[matt-dz]

Summary

A script that stays under MaxScriptBytes (5 MiB) but nests shell constructs very deeply could crash the entire embedding process (e.g. the Private Action Runner) with an uncatchable Go runtime fatal error: stack overflow. The overflow happens in one of three recursive paths — the mvdan recursive-descent parser, rshell's recursive validateNode walk, or the recursive executor — all of which consume a Go stack frame per nesting level. Because a runtime-fatal throw is not a panic, the recover() guard in Runner.Run cannot intercept it, and the crash occurs before any command, path, mode, or timeout policy is consulted.

This affects every agent version and needs no allowed commands, elevation, or remediation mode.

Fix

Introduce MaxNestingDepth (5000), enforced in two complementary places:

• Pre-parse lexical guard (checkScriptNestingDepth, called from ParseScript): a quoting-aware context-stack scanner that bounds the nesting of bracketing/substitution constructs ((, $(, ${, backtick, {) and the compound keyword commands (if/elif, while/until/for/select, case) before the parser descends into them. The context stack keeps keyword-looking words in quoted strings (e.g. "waiting for work to do") literal, while still counting keywords nested inside command substitutions such as "$(if …)".
• Post-parse AST guard (checkNestingDepth, called from the top of validateNode, so both Run and Check are covered): an iterative depth check that bounds pipeline and logical-list depth, which the parser builds iteratively (so the lexical guard doesn't see them) but the later walk and execution recurse over. It reads a node's direct children via a depth-one syntax.Walk, so it never recurses and cannot itself overflow.

Both guards report a clear error (script nesting exceeds maximum depth of 5000; …) instead of crashing. 5000 is far below the overflow threshold (hundreds of thousands of levels) and far above any realistic script.

Tests

interp/nesting_vuln_hunt_test.go:

• Rejects moderately-deep scripts (just over the limit, so they parse/run cleanly on main) across all nesting shapes — fails on main, passes here.
• Allows realistic scripts, including long pipelines, deep-but-ordinary nesting, and large scripts full of keyword-looking words / brackets inside quotes (false-positive regressions).
• Pins the lexical guard's quoting/escape/comment handling.
• A re-exec subprocess test reproduces the real exploit depth and asserts the child exits cleanly rather than dying with a stack overflow.

Full go test -race ./... passes except the pre-existing arm64 TestCheckRemoteNoHostIO/stat-canary failure (fixed separately by #684).

Docs updated in README.md and SHELL_FEATURES.md.

🤖 Generated with Claude Code

[julesmcrt]

Reviewed a09b8ee9b253a5a1c51f1a734e90e9e87ad9940d.

The iterative AST guard and its Run/Check integration are sound for the reviewed paths. Focused race tests and non-Docker scenarios pass, but the pre-parse half of the guard does not bound what it claims to bound.

P1 — the lexical guard can undercount real parser nesting

interp/nesting.go:88–246 and applyKeyword treat text as structural syntax even where the parser treats it as literal data:

• Quoted heredoc bodies: closing parentheses inside a <<'EOF' body pop the scanner's command-substitution frames, while the parser keeps those frames open.
• Argument-position keywords: in echo fi fi …, the fi words pop scanner if frames even though they are ordinary arguments, not closing keywords.

The diagnostic repeated batches of 4,000 genuine nested constructs, then literal text that clears the scanner's tracked frames. With 15 batches, checkScriptNestingDepth accepts scripts with 60,000 actual nesting levels, and the parser successfully builds ASTs that only the post-parse guard then rejects. Both the heredoc and the argument-keyword construction were confirmed. The new pre-parse guard therefore does not enforce its claimed depth limit.

A separate bounded subprocess diagnostic set the goroutine stack limit to 8 MiB (debug.SetMaxStack) and called ParseScript on the heredoc construction. It terminated with fatal error: stack overflow inside the mvdan parser recursion, before ParseScript returned — so neither the post-parse guard nor the recover() in Runner.Run can protect that stage.

Scope of evidence: the crash was reproduced with an explicitly reduced stack limit, not the default 1 GiB limit; no default-limit threshold was measured and no measured production-threshold exploit is claimed here. The confirmed security defect is the unbounded discrepancy between the lexical guard's counter and actual parser recursion, which leaves the parser-side protection incomplete.

Fix: Make pre-parse depth tracking follow the parser's lexical contexts — heredoc bodies (quoted delimiters: fully literal; unquoted: still count $(/${/backtick, but not keywords or bare brackets) and command-position gating for closers — or enforce the depth limit inside the parser itself. Add regressions proving both constructions are rejected before recursive parsing.

P2 — the same lexical mismatch rejects valid shallow scripts

Confirmed cases that the mvdan parser accepts but the new scanner rejects with script nesting exceeds maximum depth of 5000:

• A quoted heredoc body containing 5,100 literal opening parentheses.
• 5,001 separate echo if commands: the argument if is counted as an opening keyword even though the script has no nested if statements.

Add accepted regressions for literal heredoc content and keyword-looking arguments when fixing the context tracking.

Requesting changes for the pre-parse nesting bypass. All diagnostics were temporary (removed afterward); worktree clean at the reviewed head. Validation was on macOS arm64; Docker/bash comparison and Linux/Windows runtime checks were not run. One separately probed case esac construction was syntactically invalid and is not included as a finding.

[matt-dz]

Thanks @julesmcrt — this was a correct and valuable catch. Fixed in the latest push by making the pre-parse lexical guard follow the parser's lexical contexts instead of treating all bytes as structural syntax.

P1 — lexical guard could undercount real parser nesting
checkScriptNestingDepth is now a quoting-/context-aware state machine over a frame stack whose top selects the mode:

• Single quotes and quoted heredoc bodies are fully literal; double quotes and unquoted heredoc bodies are literal except for $(, ${ and backquotes (which still open a nested command context, so deep $( inside an expanding heredoc is still counted).
• Reserved words and brace command groups are recognised only in command position, so argument-position fi/done/esac/} no longer pop tracked frames.

So neither construction clears the counter anymore. Both of your reproductions are now rejected before the recursive parser runs — see TestLexicalGuardTracksParserContexts (heredoc_hidden_nesting, argument_keyword_nesting, plus sub_in_unquoted_heredoc to pin the expanding-body case).

P2 — same mismatch rejected valid shallow scripts
The context tracking also fixes the false positives. Added accepted regressions in TestNestingGuardAllowsRealisticScripts: literal_parens_in_quoted_heredoc (5,100 literal (), keyword_looking_arguments (5,001 × echo if), closer_looking_arguments (echo fi done esac), brace_looking_arguments (echo }), function_definitions, and normal_heredoc_with_expansion.

Extra fix found while validating under -race: an empty () is now treated as a function-definition parameter list so the body group { … ; } is still counted (f() { … } nesting was otherwise missed).

Validation: full go test -race ./... passes (except the pre-existing arm64 TestCheckRemoteNoHostIO/stat-canary, fixed separately by #684), and the lexical guard was run over all 3,441 checked-in scenario scripts with zero false positives. The post-parse iterative AST guard is unchanged.

Re-requesting review.

[julesmcrt]

Reviewed d27453d9.

The round-1 findings are fixed: nesting hidden behind quoted-heredoc bodies and argument-position keywords is now rejected before the recursive parser runs, and the false rejections (literal parens in quoted heredocs, keyword-looking arguments, function definitions, expanding heredocs) are gone. The scenario sweep (3,446 checked-in scripts) shows no false positives, the post-parse iterative AST guard is unchanged, and the focused -race suite and go vet are clean.

However, the rewritten state machine still undercounts real parser nesting through three new vectors, each reproducing the same failure the pre-parse guard exists to prevent.

P1 — remaining undercount vectors in the lexical guard

1. << inside arithmetic opens a phantom heredoc body (interp/nesting.go:339-342, interp/nesting.go:420-431). << in $(( a << b )) (or a (( … )) arithmetic command) is the shift operator, but the scanner reads it as a heredoc operator and takes b as the delimiter word. At the next newline a heredoc-body frame opens that can never close, and in heredoc-body mode only $(, ${ and backquotes open counted frames — so everything after the arithmetic is uncounted:

"echo $((1 << 2))\n" + strings.Repeat("(", 6000) + "x" + strings.Repeat(")", 6000)
"$((1<<9))\n" + strings.Repeat("(", 6000) + "x" + strings.Repeat(")", 6000) + "\n"

passes checkScriptNestingDepth, parses to a 6,000-deep AST, and at 60,000 depth (~120 KB, well under MaxScriptBytes) a child with debug.SetMaxStack(8<<20) dies with fatal error: stack overflow inside ParseScript, before the post-parse guard can run. The nested-if variant behaves the same. The outcome depends on whether a later line exactly equals the phantom delimiter word — with delimiter 9, no later 9 line means the body never closes and the bypass holds, while a line exactly 9 closes it and counting resumes (useful as a paired regression; an attacker just avoids the delimiter line). A real cat <<9 … 9 heredoc is correctly rejected, so the arithmetic misparse is the distinguishing factor. The doc comment at interp/nesting.go:112-114 ("can only over-skip and never undercount") is a counterexample to itself and should be corrected with the fix.

2. ) after a case pattern leaves the scanner out of command position (interp/nesting.go:318-322). In real shell the case body begins a command list; the scanner sets cmdPos = false. Openers that are recognized only in command position — case and { … } — are therefore never counted at the start of a case body:

strings.Repeat("case a in x) case b in y) ", 6000) + ":" + strings.Repeat(";; esac;; esac", 6000)
strings.Repeat("case a in x) { ", 6000) + ":" + strings.Repeat("; };; esac", 6000)

Both pass the guard and crash the 8 MiB-stack child at 60,000 depth. (Subshell-shaped case bodies are caught because (/$( are counted regardless of position; if/while bodies are caught only incidentally, because their own then/do restores command position for the next level.)

3. CRLF line endings keep the scanner's heredoc body open forever (interp/nesting.go:182-188). The delimiter-line compare does not strip a trailing \r; the mvdan parser is CRLF-aware and closes the heredoc at E\r\n, while the scanner sees E\r ≠ E and treats the whole rest of the script as heredoc text:

"cat <<E\r\nE\r\n" + strings.Repeat("(", 6000) + "x" + strings.Repeat(")", 6000) + "\n"

Same result: guard accepts, parser builds a 6,000-deep AST, 60,000 depth crashes the 8 MiB child.

As before, the stack-overflow reproductions use an explicitly reduced 8 MiB stack limit rather than the default 1 GiB, so no default-threshold exploit is claimed; the defect is that the guard accepts scripts whose actual parser nesting is unbounded, leaving the parser-side protection incomplete for these shapes.

Suggested fixes: mark frames opened by $((/(( as arithmetic and skip redirect handling while the topmost counted frame is arithmetic (a $(( $(x) + 1<<2 )) stays correct because the command substitution frame is on top there); set cmdPos = true when ) is seen with kindCase on top; strip a trailing \r from the delimiter and body lines before comparing. Alternatively, as discussed last round, enforce the limit inside the parser itself rather than a hand-rolled approximation — a fix for one of these three will predictably be followed by another lexical edge case.

P2 — continuation words set command position unconditionally (interp/nesting.go:536-537)

then/do/else/in set cmdPos = true without checking the enclosing compound, so a keyword-looking argument after them is counted as an opener. strings.Repeat("if true; then in if; :; fi\n", 6000) is valid bash (an ordinary command in with argument if) but is rejected at 6,000 repetitions. Gating the continuation words on the matching frame kind (kindIf/kindLoop/kindCase on top) closes this without affecting the round-1 regressions.

Requesting changes for the three undercount vectors; the P2 is minor and could ride along in the same fix. Repros were run in bounded 8 MiB-stack subprocesses only; diagnostics were temporary and removed, worktree clean at d27453d9. Validation was on macOS arm64; Docker-based scenario comparisons and Linux/Windows runtime checks were not run.

[matt-dz]

Thanks for round 2, julesmcrt. All three P1 vectors and the P2 are fixed in 035a4e3. The branch also now merges main (after #695) to clear the README conflict.

P1.1: << inside arithmetic opened a phantom heredoc. Arithmetic is now its own scanner mode: $((…)), ((…)), $[…], let arguments, and the a[…] index of prefix assignments and declare/local/export/… arguments. In that mode << is a shift and words are plain names. This follows mvdan's rules: (( is always arithmetic, name[ is an index only in assignment positions (echo a[1<<2] is still a heredoc), and let x && y is a list while let x&&y is one expression. ${…} words get the same treatment (${x//<</y} was the same bug), and $((…)) inside an unquoted heredoc body is covered too. The "can only over-skip" doc comment is gone, replaced by the actual invariant.

P1.2: ) after a case pattern left the scanner out of command position. Case statements now go through head, pattern list and body states. A pattern's ) starts a command, ;;/;&/;;& start the next pattern list, and a leading ( and extglob groups are handled. So case and { at the start of a body are counted, and keyword-looking patterns (if|while)) are not.

P1.3: CRLF heredoc delimiters. A trailing \r is removed before the delimiter compare, matching the parser.

P2: continuation words. then/else restore command position only when an if/elif frame is on top, and do only when a loop frame is. in is never a command-position word, so if true; then in if; :; fi ×6000 is accepted.

While fixing these I found and fixed more vectors of the same kind:

• Command position now survives !, time [-p], function NAME, coproc NAME { … }, and backslash-newline.
• $'\'' is read as one ANSI-C string instead of opening a phantom quote.

Tests (interp/nesting_vuln_hunt_test.go):

• TestLexicalGuardRejectsBypassShapes: 23 shapes, including your exact repros, all rejected by ParseScript before the parser runs. All 23 fail against d27453d's scanner. Shallow variants of the same shapes are accepted.
• TestLexicalBypassDoesNotCrashParser: your reproduction, a re-exec child with debug.SetMaxStack(8<<20) parsing each shape at depth 60,000.
• TestLexicalGuardAllowsKeywordLookingWords: false-positive regressions, including the P2 case, keyword words in for lists and case patterns, shift operators, literal parens in ${…}, and CRLF heredocs.
• A throwaway randomized check (not committed) mixed nesting past the limit with tricky fragments. Over 300 iterations it found no accepted deep script and no false positives on shallow ones.

go test -race ./... passes. The one failure, TestPSPentestLargePIDListDoesNotHang, also fails on clean main on this host: its PIDs are above 1,000,000, which the test assumes don't exist.

Not done: moving the limit into the parser itself. I agree a hand-rolled approximation could have further lexical edge cases. Doing it properly means forking or vendoring mvdan.cc/sh/v3/syntax with a depth counter, which is a bigger call for Matthew to make. I've raised it with him rather than doing it in this PR.

[julesmcrt]

Reviewed 035a4e33.

All four items from the previous review are fixed, and I could not find a way back in. Verifying each against the original repros:

1. Arithmetic << phantom heredoc — fixed. echo $((1 << 2)) / $((1<<9)) / (( a << b )) prefixes followed by 6,000-deep parens are now rejected by the pre-parse guard, and the 60,000-depth variants are rejected before the parser descends in a child capped at an 8 MiB stack (no stack overflow). Arithmetic is now its own frame kind, so << is consumed as a shift operator and never reaches the heredoc scanner. Command substitution nested inside arithmetic ($(( $(x) + 1<<2 ))) still counts normally, and heredocs inside arithmetic-containing heredoc bodies are handled.

2. Case-body command position — fixed. Both the case-in-case and {-in-case-body repros at 6,000 and 60,000 depth are rejected pre-parse. The pattern's ) restores command position, so openers at the start of a case body are counted; a ) belonging to a subshell inside a case body is not confused with the pattern closer.

3. CRLF heredoc undercount — fixed. cat <<E\r\nE\r\n + deep parens is rejected at both depths; the delimiter compare now strips the trailing \r exactly where the parser closes the heredoc. \r in quoted heredoc bodies and <<- with CRLF are still accepted.

4. Continuation-word false rejection — fixed in both directions. if true; then in if; :; fi repeated 6,000 times is accepted again, while the round-1 keyword-hidden-nesting detections (argument-position keywords, nesting hidden in heredoc bodies) still reject, and the keyword-looking-argument cases (for-lists, case patterns, echo then else do) all stay accepted. then/else/do are now gated on the matching compound on top of the stack, and in only applies in a case head.

The added regression tests are the real thing, not shallow variants: TestLexicalBypassDoesNotCrashParser re-execs a child with debug.SetMaxStack(8<<20) and feeds every bypass shape at depth 60,000, failing on any stack-overflow output, and TestLexicalGuardRejectsBypassShapes/TestLexicalGuardAllowsKeywordLookingWords cover the vectors plus shallow valid twins.

I also probed the new state the fix introduces for fresh undercounts — arithmetic spanning lines, backquotes inside arithmetic, heredocs declared after arithmetic, $(( vs (( in and out of command position, nested $(( 1 + … at 60,000 deep, let/$[…]/a[1<<2]= index arithmetic, extglob-style pattern groups, and \r inside quoted heredoc bodies — about forty shapes in total. No new undercounts and no new false rejections. The only probes the guard accepts are ones the parser genuinely handles shallowly (e.g. an unescaped-backtick run cannot nest, so it stays at depth 1 — confirmed against the parser at 8 MiB). One minor note: the TestCheckScriptNestingDepthQuotingContexts positive control changed from a raw "(" run to "( "-separated — that's correct, since an unspaced leading (( now (correctly) lexes as arithmetic and costs two bytes for its opening frame; every following ( is still counted, so it's calibration, not a weakening.

The merge from main (#700/#703/#704) doesn't touch interp/nesting.go; the fix delta is confined to the scanner rewrite and its tests.

Full sweep at this head: the nesting/lexical/deep guard tests, TestShellScenarios, a focused -race run on interp, and go vet ./... are all green. The only thing I could not run locally is the Docker-based bash comparison suite (RSHELL_BASH_TEST=1), which isn't available on this host — worth a CI run before merge if it's not already in the pipeline.

Nice work — the frame-kind state machine is a big step up in fidelity over the previous scanner, and the test suite now pins both directions (reject the deep bypass, accept the shallow twin) for every shape.