diff --git a/.claude/hooks/normalize-ifs.mjs b/.claude/hooks/normalize-ifs.mjs index 0f8c24f39..7dcfa721c 100644 --- a/.claude/hooks/normalize-ifs.mjs +++ b/.claude/hooks/normalize-ifs.mjs @@ -1,6 +1,8 @@ #!/usr/bin/env node // normalize-ifs.mjs — reads a shell command line from stdin and writes it -// back with every $IFS/${IFS} reference replaced by a single literal space +// back with every $IFS/${IFS} reference (plus, for the `:+`/`+` +// alternate-value form only, the equivalent reference to any other +// normally-set variable — #2558) replaced by a single literal space // (#2451). Bash's own field-splitting on an unquoted $IFS/${IFS} expansion // produces exactly this effect at execution time — `git${IFS}checkout` // looks like one token as command TEXT, but Git actually receives @@ -82,37 +84,61 @@ // a run of ALL zeros after the minus sign — matched separately here via // `-0+` (a literal minus followed by one or more zeros and nothing else). // -// `${IFS:+ }`/`${IFS+ }` (alternate-value expansion, restricted to +// `${VAR:+ }`/`${VAR+ }` (alternate-value expansion, restricted to // ALL-whitespace content — Greptile review): unlike substring, this -// operator does NOT extract from IFS's own value at all — it substitutes -// an entirely separate, attacker-chosen string `word` whenever IFS IS set -// and non-null (`:+`) or merely set (`+`), which normally means `word` is -// what actually comes out, not IFS's value. Because of that, this is -// matched ONLY when `word` consists of one or more spaces/tabs and -// NOTHING else — `${IFS:+ }` really does expand to a literal space -// (`word` itself, not derived from IFS), so it's exactly as safe to -// normalize as the whole-variable form; `${IFS:+x}` is not touched, since +// operator does NOT extract from the variable's own value at all — it +// substitutes an entirely separate, attacker-chosen string `word` whenever +// VAR IS set and non-null (`:+`) or merely set (`+`), which normally means +// `word` is what actually comes out, not VAR's value. Because of that, +// this is matched ONLY when `word` consists of one or more spaces/tabs and +// NOTHING else — `${VAR:+ }` really does expand to a literal space +// (`word` itself, not derived from VAR), so it's exactly as safe to +// normalize as the whole-variable form; `${VAR:+x}` is not touched, since // `x` is not whitespace and substituting a space for it would fabricate a // token boundary bash never produces (an earlier version of this // normalizer treated the operator as unconditionally unsafe and missed // this whitespace-content special case — Greptile review). // -// Deliberately does NOT generalize to "any `${IFS...}`": an -// earlier version tried that and was wrong (Greptile review) — -// `${IFS/pattern/replacement}` (substitutes `replacement` wherever -// `pattern` matches within IFS's value) and the bash 4.4+ `${IFS@Q}`-style -// transformation operators (e.g. `@Q` shell-quotes the value, producing -// `$' \t\n'`-shaped text) can ALSO produce arbitrary non-whitespace text, -// the same class of problem `:+`/`+` have — and unlike `:+`/`+`, there is -// no simple "restrict to all-whitespace content" fix for them, since -// `pattern`/`replacement ` are two separate, differently-shaped fields. -// Left unhandled rather than risk another incorrect generalization; a -// determined obfuscator using one of these is a known, accepted gap in -// this heuristic guard (see #2558 for tracking the closely related, -// broader problem that `:+`/`+` with all-whitespace content isn't even -// unique to the `IFS` name — `${HOME:+ }`/`${PWD:+ }`/any other normally-set -// variable works identically, which a per-variable-name normalizer like -// this one can never fully close). +// Unlike the other three replacements below, this one is NOT restricted to +// the literal name `IFS`: the operator's behavior has nothing to do with +// what VAR's own value actually is, only with whether VAR is normally set +// and non-null — true for almost any commonly-set variable (`HOME`, `PWD`, +// `PATH`, ...), not just `IFS`. A per-variable-name check could never fully +// close this class, since the variable name in the bypass isn't fixed +// (#2558) — so this one matches any bash identifier shape +// (`[A-Za-z_][A-Za-z0-9_]*`) in that position, erring toward normalizing +// (the safe direction for a guard) even for a variable that happens not to +// be set in a given shell, rather than trying to track which variables are +// actually set. +// +// Also matches bash's SPECIAL parameters in that position — `?` (exit +// status), `$` (PID), `#` (positional-parameter count), `-` (current shell +// option flags), `!` (last background PID), and a bare digit sequence +// (positional parameters, `${10:+ }` etc.) — verified directly against real +// bash (Greptile review): `${?:+ }`/`${$:+ }`/`${#:+ }`/`${-:+ }` all +// substitute the whitespace word exactly like an ordinary variable would, +// and `?`/`$`/`#`/`-` are always set in any shell (unlike `!`/digit +// parameters, which depend on whether a job has been backgrounded or +// positional arguments are present — matched anyway, erring toward +// normalizing). None of these characters overlap with the identifier +// alternative above, so a single alternation covers both without ambiguity. +// +// Deliberately does NOT generalize the other three replacements below to +// "any `${VAR...}`" — only `:+`/`+` generalizes across variable +// names, because only its substituted text is entirely independent of the +// variable's own value. `${IFS/pattern/replacement}` (substitutes +// `replacement` wherever `pattern` matches within IFS's value) and the +// bash 4.4+ `${IFS@Q}`-style transformation operators (e.g. `@Q` +// shell-quotes the value, producing `$' \t\n'`-shaped text) can ALSO +// produce arbitrary non-whitespace text, the same class of problem `:+`/`+` +// have — but unlike `:+`/`+`, there is no simple "restrict to all-whitespace +// content" fix for them (`pattern`/`replacement` are two separate, +// differently-shaped fields), AND they only produce a whitespace-only +// result by relying on IFS's own specific default value in the first +// place, so generalizing them to other variable names wouldn't even be +// meaningful the way it is for `:+`/`+`. Left unhandled rather than risk +// another incorrect generalization; a determined obfuscator using one of +// these remains a known, accepted gap in this heuristic guard. // // The bare `$IFS` form must not swallow the start of a longer variable // name — `$IFSOMETHING` references a completely different (and almost @@ -136,7 +162,7 @@ process.stdin.on('end', () => { const normalized = input .replace(/\$\{IFS\}/g, ' ') .replace(/\$\{IFS: *(?:0*[0-2]|-0*[1-3]|-0+)(?::0*[1-9]\d*)?\}/g, ' ') - .replace(/\$\{IFS:?\+[ \t]+\}/g, ' ') + .replace(/\$\{(?:[A-Za-z_][A-Za-z0-9_]*|[0-9]+|[?$!#@*-]):?\+[ \t]+\}/g, ' ') .replace(/\$IFS(?![A-Za-z0-9_])/g, ' '); process.stdout.write(normalized); }); diff --git a/docs/examples/claude-code-hooks/normalize-ifs.mjs b/docs/examples/claude-code-hooks/normalize-ifs.mjs index 0f8c24f39..7dcfa721c 100644 --- a/docs/examples/claude-code-hooks/normalize-ifs.mjs +++ b/docs/examples/claude-code-hooks/normalize-ifs.mjs @@ -1,6 +1,8 @@ #!/usr/bin/env node // normalize-ifs.mjs — reads a shell command line from stdin and writes it -// back with every $IFS/${IFS} reference replaced by a single literal space +// back with every $IFS/${IFS} reference (plus, for the `:+`/`+` +// alternate-value form only, the equivalent reference to any other +// normally-set variable — #2558) replaced by a single literal space // (#2451). Bash's own field-splitting on an unquoted $IFS/${IFS} expansion // produces exactly this effect at execution time — `git${IFS}checkout` // looks like one token as command TEXT, but Git actually receives @@ -82,37 +84,61 @@ // a run of ALL zeros after the minus sign — matched separately here via // `-0+` (a literal minus followed by one or more zeros and nothing else). // -// `${IFS:+ }`/`${IFS+ }` (alternate-value expansion, restricted to +// `${VAR:+ }`/`${VAR+ }` (alternate-value expansion, restricted to // ALL-whitespace content — Greptile review): unlike substring, this -// operator does NOT extract from IFS's own value at all — it substitutes -// an entirely separate, attacker-chosen string `word` whenever IFS IS set -// and non-null (`:+`) or merely set (`+`), which normally means `word` is -// what actually comes out, not IFS's value. Because of that, this is -// matched ONLY when `word` consists of one or more spaces/tabs and -// NOTHING else — `${IFS:+ }` really does expand to a literal space -// (`word` itself, not derived from IFS), so it's exactly as safe to -// normalize as the whole-variable form; `${IFS:+x}` is not touched, since +// operator does NOT extract from the variable's own value at all — it +// substitutes an entirely separate, attacker-chosen string `word` whenever +// VAR IS set and non-null (`:+`) or merely set (`+`), which normally means +// `word` is what actually comes out, not VAR's value. Because of that, +// this is matched ONLY when `word` consists of one or more spaces/tabs and +// NOTHING else — `${VAR:+ }` really does expand to a literal space +// (`word` itself, not derived from VAR), so it's exactly as safe to +// normalize as the whole-variable form; `${VAR:+x}` is not touched, since // `x` is not whitespace and substituting a space for it would fabricate a // token boundary bash never produces (an earlier version of this // normalizer treated the operator as unconditionally unsafe and missed // this whitespace-content special case — Greptile review). // -// Deliberately does NOT generalize to "any `${IFS...}`": an -// earlier version tried that and was wrong (Greptile review) — -// `${IFS/pattern/replacement}` (substitutes `replacement` wherever -// `pattern` matches within IFS's value) and the bash 4.4+ `${IFS@Q}`-style -// transformation operators (e.g. `@Q` shell-quotes the value, producing -// `$' \t\n'`-shaped text) can ALSO produce arbitrary non-whitespace text, -// the same class of problem `:+`/`+` have — and unlike `:+`/`+`, there is -// no simple "restrict to all-whitespace content" fix for them, since -// `pattern`/`replacement ` are two separate, differently-shaped fields. -// Left unhandled rather than risk another incorrect generalization; a -// determined obfuscator using one of these is a known, accepted gap in -// this heuristic guard (see #2558 for tracking the closely related, -// broader problem that `:+`/`+` with all-whitespace content isn't even -// unique to the `IFS` name — `${HOME:+ }`/`${PWD:+ }`/any other normally-set -// variable works identically, which a per-variable-name normalizer like -// this one can never fully close). +// Unlike the other three replacements below, this one is NOT restricted to +// the literal name `IFS`: the operator's behavior has nothing to do with +// what VAR's own value actually is, only with whether VAR is normally set +// and non-null — true for almost any commonly-set variable (`HOME`, `PWD`, +// `PATH`, ...), not just `IFS`. A per-variable-name check could never fully +// close this class, since the variable name in the bypass isn't fixed +// (#2558) — so this one matches any bash identifier shape +// (`[A-Za-z_][A-Za-z0-9_]*`) in that position, erring toward normalizing +// (the safe direction for a guard) even for a variable that happens not to +// be set in a given shell, rather than trying to track which variables are +// actually set. +// +// Also matches bash's SPECIAL parameters in that position — `?` (exit +// status), `$` (PID), `#` (positional-parameter count), `-` (current shell +// option flags), `!` (last background PID), and a bare digit sequence +// (positional parameters, `${10:+ }` etc.) — verified directly against real +// bash (Greptile review): `${?:+ }`/`${$:+ }`/`${#:+ }`/`${-:+ }` all +// substitute the whitespace word exactly like an ordinary variable would, +// and `?`/`$`/`#`/`-` are always set in any shell (unlike `!`/digit +// parameters, which depend on whether a job has been backgrounded or +// positional arguments are present — matched anyway, erring toward +// normalizing). None of these characters overlap with the identifier +// alternative above, so a single alternation covers both without ambiguity. +// +// Deliberately does NOT generalize the other three replacements below to +// "any `${VAR...}`" — only `:+`/`+` generalizes across variable +// names, because only its substituted text is entirely independent of the +// variable's own value. `${IFS/pattern/replacement}` (substitutes +// `replacement` wherever `pattern` matches within IFS's value) and the +// bash 4.4+ `${IFS@Q}`-style transformation operators (e.g. `@Q` +// shell-quotes the value, producing `$' \t\n'`-shaped text) can ALSO +// produce arbitrary non-whitespace text, the same class of problem `:+`/`+` +// have — but unlike `:+`/`+`, there is no simple "restrict to all-whitespace +// content" fix for them (`pattern`/`replacement` are two separate, +// differently-shaped fields), AND they only produce a whitespace-only +// result by relying on IFS's own specific default value in the first +// place, so generalizing them to other variable names wouldn't even be +// meaningful the way it is for `:+`/`+`. Left unhandled rather than risk +// another incorrect generalization; a determined obfuscator using one of +// these remains a known, accepted gap in this heuristic guard. // // The bare `$IFS` form must not swallow the start of a longer variable // name — `$IFSOMETHING` references a completely different (and almost @@ -136,7 +162,7 @@ process.stdin.on('end', () => { const normalized = input .replace(/\$\{IFS\}/g, ' ') .replace(/\$\{IFS: *(?:0*[0-2]|-0*[1-3]|-0+)(?::0*[1-9]\d*)?\}/g, ' ') - .replace(/\$\{IFS:?\+[ \t]+\}/g, ' ') + .replace(/\$\{(?:[A-Za-z_][A-Za-z0-9_]*|[0-9]+|[?$!#@*-]):?\+[ \t]+\}/g, ' ') .replace(/\$IFS(?![A-Za-z0-9_])/g, ' '); process.stdout.write(normalized); }); diff --git a/tests/unit/hook-guard-git-ifs-bypass.test.ts b/tests/unit/hook-guard-git-ifs-bypass.test.ts index c99d0ed1f..af68306b5 100644 --- a/tests/unit/hook-guard-git-ifs-bypass.test.ts +++ b/tests/unit/hook-guard-git-ifs-bypass.test.ts @@ -207,6 +207,56 @@ describe('guard-git.sh IFS whitespace-expansion bypass (#2451)', () => { expect(isDenied('git${IFS+ }reset')).toBe(true); }); + it('still blocks git reset via a whitespace-only alternate-value expansion on a non-IFS, normally-set variable (#2558)', () => { + // ${HOME:+ } works identically to ${IFS:+ } — the operator substitutes + // the literal "word" whenever the named variable is set and non-null, + // regardless of what that variable's own value actually is. HOME is + // normally set in any real shell, so this produces the same token + // boundary as the IFS-specific form; a per-variable-name check could + // never fully close this class, since the variable name isn't fixed. + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${HOME:+ }reset')).toBe(true); + }); + + it('still blocks git reset via the bare (colon-less) whitespace-only alternate-value form on a non-IFS variable', () => { + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${PWD+ }reset')).toBe(true); + }); + + it('still blocks git reset via a whitespace-only alternate-value expansion on the $? special parameter (Greptile review)', () => { + // ${?:+ } is always-set (bash's exit-status special parameter is never + // unset), so this substitutes the literal space "word" exactly like an + // ordinary variable would -- verified directly against real bash. + // Neither special parameters nor bare digit sequences (positional + // parameters) match a bash identifier shape, so they need their own + // branch in the alternation, not just the ordinary-variable one above. + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${?:+ }reset')).toBe(true); + }); + + it('still blocks git reset via a whitespace-only alternate-value expansion on the $$ (PID) special parameter', () => { + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${$:+ }reset')).toBe(true); + }); + + it('still blocks git reset via a whitespace-only alternate-value expansion on the $# (arg count) special parameter', () => { + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${#:+ }reset')).toBe(true); + }); + + it('still blocks git reset via a whitespace-only alternate-value expansion on the $- (shell options) special parameter', () => { + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${-:+ }reset')).toBe(true); + }); + + it('does not invent a token boundary from a non-whitespace alternate-value expansion on a non-IFS variable', () => { + // ${SOME_VAR:+x} substitutes the literal "x", not whitespace — the + // all-whitespace-content restriction applies regardless of which + // variable is named. + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('echo git${SOME_VAR:+x}reset')).toBe(false); + }); + it('does not invent a token boundary from an empty IFS alternate-value expansion', () => { // ${IFS:+} (nothing between + and }) substitutes an EMPTY string when // IFS is set and non-null — an unquoted empty expansion contributes