diff --git a/hooks/lib/loop-common.sh b/hooks/lib/loop-common.sh index ec4497bb..3e54e6ef 100755 --- a/hooks/lib/loop-common.sh +++ b/hooks/lib/loop-common.sh @@ -1190,15 +1190,21 @@ is_cancel_authorized() { # Normalize and validate source path. # - # Canonicalize the user-provided path so a symlinked prefix in the caller's - # command (e.g. /Users/x vs /private/Users/x on macOS, or /var vs - # /private/var) matches canonical_loop_dir resolved via resolve_project_root. + # Use canonicalize_path_prefix (NOT canonicalize_path): we need to resolve + # symlinks in the parent directory so a symlinked project prefix matches + # canonical_loop_dir, but we MUST NOT dereference a symlink at the leaf. + # Otherwise a symlink like /tmp/alias -> /state.md would canonicalize + # to /state.md and pass the check, but `mv` would then operate on + # the link path itself, escaping the loop directory and/or corrupting + # loop state. The on-disk symlink rejection below (src_original check) + # still fires because it probes the real state.md under canonical_loop_dir. + # # Re-lowercase after canonicalization because realpath on case-insensitive # filesystems may restore the original casing of path components, which # would diverge from the already-lowercased expected_* values. src=$(_normalize_path "$src") local src_canonical - src_canonical="$(canonicalize_path "$src")" + src_canonical="$(canonicalize_path_prefix "$src")" src_canonical="${src_canonical:-$src}" src_canonical=$(echo "$src_canonical" | tr '[:upper:]' '[:lower:]') local expected_src_state="${loop_dir_lower}state.md" @@ -1208,11 +1214,14 @@ is_cancel_authorized() { return 5 fi - # Normalize and validate destination path (same canonicalize+lowercase - # transformation as source; see src comment above for rationale). + # Normalize and validate destination path. Uses canonicalize_path_prefix + # for the same reason as src: a symlink alias pointing at the real + # cancel-state.md must NOT pass authorization, because `mv` onto a + # symlink replaces the link rather than creating /cancel-state.md, + # corrupting loop state and moving state.md outside the loop dir. dest=$(_normalize_path "$dest") local dest_canonical - dest_canonical="$(canonicalize_path "$dest")" + dest_canonical="$(canonicalize_path_prefix "$dest")" dest_canonical="${dest_canonical:-$dest}" dest_canonical=$(echo "$dest_canonical" | tr '[:upper:]' '[:lower:]') local expected_dest="${loop_dir_lower}cancel-state.md" diff --git a/hooks/lib/project-root.sh b/hooks/lib/project-root.sh index 3887788b..cb23403a 100644 --- a/hooks/lib/project-root.sh +++ b/hooks/lib/project-root.sh @@ -52,6 +52,49 @@ resolve_project_root() { printf '%s\n' "${canonical:-$root}" } +# canonicalize_path_prefix +# +# Resolves symlinks ONLY in the parent directory and reattaches the +# original basename verbatim. This is the right helper for comparing +# user-supplied filenames against an expected path inside a known +# directory: a symlink at /tmp/alias pointing at /real/loop/state.md +# MUST NOT canonicalize to /real/loop/state.md for comparison purposes, +# because `mv` operates on the link path itself. Resolving only the +# parent still lets a symlinked project prefix (e.g. /var vs /private/var +# on macOS) match a canonical expected path. +# +# If realpath on the parent fails, falls back to returning the input +# path unchanged (prefix cannot be canonicalized -> caller's comparison +# will correctly fail against a canonical expected path). +# +# Empty input prints nothing and returns 0. +# +canonicalize_path_prefix() { + local path="$1" + if [[ -z "$path" ]]; then + return 0 + fi + + local parent base parent_real + parent=$(dirname -- "$path") + base=$(basename -- "$path") + + if parent_real=$(realpath "$parent" 2>/dev/null) && [[ -n "$parent_real" ]]; then + printf '%s/%s\n' "${parent_real%/}" "$base" + return 0 + fi + + if command -v python3 >/dev/null 2>&1; then + parent_real=$(python3 -c 'import os,sys;print(os.path.realpath(sys.argv[1]))' "$parent" 2>/dev/null || true) + if [[ -n "$parent_real" ]]; then + printf '%s/%s\n' "${parent_real%/}" "$base" + return 0 + fi + fi + + printf '%s\n' "$path" +} + # canonicalize_path # # Prints the realpath of the input path. If the path itself does not @@ -60,6 +103,11 @@ resolve_project_root() { # If realpath is unavailable and python3 is missing, prints the input # path verbatim. # +# SECURITY NOTE: This helper dereferences symlinks at the leaf when +# the leaf exists. Do NOT use it to authorize a user-supplied path +# against an expected filename -- use canonicalize_path_prefix instead, +# which only resolves the parent. +# # Empty input prints nothing and returns 0. # canonicalize_path() { diff --git a/hooks/loop-read-validator.sh b/hooks/loop-read-validator.sh index 5a27fc4f..b812288a 100755 --- a/hooks/loop-read-validator.sh +++ b/hooks/loop-read-validator.sh @@ -304,10 +304,12 @@ fi CORRECT_PATH="$ACTIVE_LOOP_DIR/$CLAUDE_FILENAME" -# Compare canonical (symlink-resolved) forms -- see loop-write-validator.sh -# for the rationale; the same reasoning applies to read paths. -_READ_FILE_REAL=$(canonicalize_path "$FILE_PATH") -_READ_CORRECT_REAL=$(canonicalize_path "$CORRECT_PATH") +# Compare prefix-canonical forms -- see loop-write-validator.sh for the +# rationale; the same reasoning applies to read paths. A planted symlink +# at the leaf would otherwise let a Read follow the link outside the loop +# dir and still pass this validator. +_READ_FILE_REAL=$(canonicalize_path_prefix "$FILE_PATH") +_READ_CORRECT_REAL=$(canonicalize_path_prefix "$CORRECT_PATH") if [[ "${_READ_FILE_REAL:-$FILE_PATH}" != "${_READ_CORRECT_REAL:-$CORRECT_PATH}" ]]; then FALLBACK="# Wrong Directory Path diff --git a/hooks/loop-write-validator.sh b/hooks/loop-write-validator.sh index 9f46c102..eac0744c 100755 --- a/hooks/loop-write-validator.sh +++ b/hooks/loop-write-validator.sh @@ -331,13 +331,15 @@ fi CORRECT_PATH="$ACTIVE_LOOP_DIR/$CLAUDE_FILENAME" -# Compare canonical (symlink-resolved) forms so the check is not fooled by -# equivalent paths expressed in different prefix forms (e.g. /var/... vs -# /private/var/... on macOS). A raw string compare would mis-handle a -# symlinked project prefix whenever one side was canonicalized upstream -# (e.g. by resolve_project_root) and the other was not. -_WRITE_FILE_REAL=$(canonicalize_path "$FILE_PATH") -_WRITE_CORRECT_REAL=$(canonicalize_path "$CORRECT_PATH") +# Compare prefix-canonical forms so the check is not fooled by equivalent +# paths expressed in different ancestor forms (e.g. /var/... vs /private/var/... +# on macOS) -- without dereferencing the leaf. Using full realpath here +# would let a planted symlink at / pointing outside +# the loop dir approve a write through the link, escalating Claude's write +# reach beyond the loop dir. canonicalize_path_prefix resolves the parent +# directory only; the basename is compared verbatim. +_WRITE_FILE_REAL=$(canonicalize_path_prefix "$FILE_PATH") +_WRITE_CORRECT_REAL=$(canonicalize_path_prefix "$CORRECT_PATH") if [[ "${_WRITE_FILE_REAL:-$FILE_PATH}" != "${_WRITE_CORRECT_REAL:-$CORRECT_PATH}" ]]; then FALLBACK="# Wrong Directory Path diff --git a/tests/test-cancel-signal-file.sh b/tests/test-cancel-signal-file.sh index 7d0ca8b9..420c5d5d 100755 --- a/tests/test-cancel-signal-file.sh +++ b/tests/test-cancel-signal-file.sh @@ -1373,6 +1373,51 @@ fi rm -rf "$SYMLINK_ROOT" 2>/dev/null || true +echo "HELPER TEST 9: is_cancel_authorized rejects destination symlink alias" +# Regression test for a P1 security issue: if the destination argument is a +# symlink that points at /cancel-state.md, canonicalizing the full +# path (leaf dereferenced) would let the alias pass authorization. `mv` +# would then operate on the link path itself, corrupting loop state and +# leaking state.md contents outside the loop dir. The fix resolves symlinks +# only in the parent directory and preserves the basename verbatim. +setup_test_loop "helper-9" +touch "$LOOP_DIR/.cancel-requested" +# Create the target file so the symlink would resolve if the prefix-only +# canonicalizer were relaxed back to full canonicalization. +touch "$LOOP_DIR/cancel-state.md" +ln -sfn "$LOOP_DIR/cancel-state.md" "$TEST_DIR/dest-alias" + +COMMAND_LOWER="mv ${LOOP_DIR}/state.md ${TEST_DIR}/dest-alias" +COMMAND_LOWER=$(to_lower "$COMMAND_LOWER") + +if is_cancel_authorized "$LOOP_DIR" "$COMMAND_LOWER"; then + fail "helper dest symlink alias" "returns non-zero (rejected)" "returns 0 (authorized)" +else + pass "is_cancel_authorized rejects destination symlink alias" +fi +rm -f "$TEST_DIR/dest-alias" "$LOOP_DIR/cancel-state.md" + +echo "HELPER TEST 10: is_cancel_authorized rejects source symlink alias" +# Regression test for a P1 security issue: if the source argument is a +# symlink aliasing /state.md, dereferencing the leaf would let it +# pass authorization. The on-disk symlink check (src_original) below +# would still catch this specific case because it probes the real path, +# but we defend in depth: the path comparison must reject the alias on +# its own. +setup_test_loop "helper-10" +touch "$LOOP_DIR/.cancel-requested" +ln -sfn "$LOOP_DIR/state.md" "$TEST_DIR/src-alias" + +COMMAND_LOWER="mv ${TEST_DIR}/src-alias ${LOOP_DIR}/cancel-state.md" +COMMAND_LOWER=$(to_lower "$COMMAND_LOWER") + +if is_cancel_authorized "$LOOP_DIR" "$COMMAND_LOWER"; then + fail "helper src symlink alias" "returns non-zero (rejected)" "returns 0 (authorized)" +else + pass "is_cancel_authorized rejects source symlink alias" +fi +rm -f "$TEST_DIR/src-alias" + # ======================================== # Summary # ========================================