From bf1fe507ea9ededaaf47069bd9bb4ea3be2603a4 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Fri, 25 Sep 2026 21:31:35 +0200 Subject: [PATCH 1/3] danger: harden classifier (env-dump builtins, fork bombs, quote desync, allowlist bypass) - Gate builtin env dumps (set, set -o, export -p, declare/typeset -p) as system_write - Generalize fork-bomb detection beyond the ':' name (RE2-safe, code-checked match) - Fix extractSubstitutions double-quote escape desync hiding substitution bodies - Run isRawBlocked before allowlist match in ActionForCommand - Friction prompt re-prompts once on reflex answers instead of silent deny - Fix dead writePrefixes comment; remove duplicate openssl egress branch - Pin dynamic-subst write verbs, clobber redirects, and deep-nesting termination with tests --- internal/danger/approver.go | 23 ++- .../danger/approver_friction_wave_test.go | 49 +++++++ internal/danger/classifier.go | 101 ++++++++++++- .../danger/classifier_hardening_wave_test.go | 135 ++++++++++++++++++ 4 files changed, 298 insertions(+), 10 deletions(-) create mode 100644 internal/danger/approver_friction_wave_test.go create mode 100644 internal/danger/classifier_hardening_wave_test.go diff --git a/internal/danger/approver.go b/internal/danger/approver.go index 12bc0275..8f88b801 100644 --- a/internal/danger/approver.go +++ b/internal/danger/approver.go @@ -364,13 +364,30 @@ func (a *TTYApprover) promptLocked(cls RiskClass, cmd, description string) error } line = strings.TrimSpace(strings.ToLower(line)) - // In friction mode, only the full word "approve" is accepted. + // In friction mode, only the full word "approve" is accepted. A short + // reflex answer ("a", "y", or a bare Enter — muscle memory from the + // non-friction prompt) gets ONE explicit re-prompt instead of a silent + // denial; an explicit denial input still denies immediately. if friction { - if line == "approve" { + switch line { + case "approve": a.recordApproval(cls) return nil + case "d", "deny", "n", "no": + return fmt.Errorf("operation denied by user (friction mode): %s", cmd) + default: + fmt.Fprint(os.Stderr, " Friction mode: type 'approve' (full word) to proceed, or 'd' to deny: ") + line2, err := a.readTTYLine(tty, reader) + if err != nil { + return fmt.Errorf("approval prompt error: %w", err) + } + line2 = strings.TrimSpace(strings.ToLower(line2)) + if line2 == "approve" { + a.recordApproval(cls) + return nil + } + return fmt.Errorf("operation denied by user (friction mode): %s", cmd) } - return fmt.Errorf("operation denied by user (friction mode): %s", cmd) } switch line { diff --git a/internal/danger/approver_friction_wave_test.go b/internal/danger/approver_friction_wave_test.go new file mode 100644 index 00000000..a9960e30 --- /dev/null +++ b/internal/danger/approver_friction_wave_test.go @@ -0,0 +1,49 @@ +package danger + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// In friction mode a reflex short answer ("a", "y", or bare Enter) must +// re-prompt once with explicit instructions instead of silently denying; +// an explicit denial input still denies immediately. +func TestFrictionReflexAnswerReprompts(t *testing.T) { + script := filepath.Join(t.TempDir(), "tty-script") + if err := os.WriteFile(script, []byte("a\napprove\n"), 0o600); err != nil { + t.Fatal(err) + } + a := NewTTYApprover(&DangerousConfig{NonInteractive: strPtr("deny")}) + a.TTYPath = script + a.FrictionThreshold = 3 + a.FrictionWindow = time.Minute + a.pauseFn = func(time.Duration) {} + for i := 0; i < 3; i++ { + a.recordApproval(CodeExecution) + } + if err := a.PromptCommand(CodeExecution, "echo hi", "test"); err != nil { + t.Fatalf("reflex 'a' should re-prompt, then 'approve' should succeed: %v", err) + } +} + +func TestFrictionExplicitDenialStillDenies(t *testing.T) { + script := filepath.Join(t.TempDir(), "tty-script") + if err := os.WriteFile(script, []byte("d\n"), 0o600); err != nil { + t.Fatal(err) + } + a := NewTTYApprover(&DangerousConfig{NonInteractive: strPtr("deny")}) + a.TTYPath = script + a.FrictionThreshold = 3 + a.FrictionWindow = time.Minute + a.pauseFn = func(time.Duration) {} + for i := 0; i < 3; i++ { + a.recordApproval(CodeExecution) + } + err := a.PromptCommand(CodeExecution, "echo hi", "test") + if err == nil || !strings.Contains(err.Error(), "denied") { + t.Fatalf("explicit 'd' must deny immediately, got: %v", err) + } +} diff --git a/internal/danger/classifier.go b/internal/danger/classifier.go index 6956ebc9..7813724e 100644 --- a/internal/danger/classifier.go +++ b/internal/danger/classifier.go @@ -882,6 +882,11 @@ func (c *DangerousConfig) ActionForCommand(cmd string) Action { if cmd == "" { return Allow } + // The raw-blocked floor runs before every list check: even an exact + // allowlist entry must not re-arm a fork bomb or other blocked shape. + if isRawBlocked(cmd) { + return Deny + } // Allowlist has highest priority — exact match after trimming both sides. for _, pattern := range c.Allowlist { if cmd == strings.TrimSpace(pattern) { @@ -1106,9 +1111,9 @@ func tokenize(input string) []string { return tokens } -// ── Safe command prefixes ────────────────────────────────────────────── -// (Unused — classification falls through to Safe by default. Kept as -// documentation of what's considered read-only.) +// ── Write command prefixes ───────────────────────────────────────────── +// Not consulted for default fall-through (classification defaults to Safe +// when nothing matches); these gate verbs that write to the filesystem. var writePrefixes = map[string]bool{ // echo is deliberately absent: without a redirect it only prints, and @@ -1339,6 +1344,7 @@ var safeCommands = map[string]bool{ // common modern read-only CLIs (ls/find/cat/ps/df/du/diff/hex viewers) "fd": true, "fdfind": true, "eza": true, "exa": true, "lsd": true, "htop": true, "btop": true, "glances": true, "pstree": true, "procs": true, + "top": true, "duf": true, "dust": true, "delta": true, "hexyl": true, "glow": true, // Language toolchains: compile / format / lint. Same bar as go build // and cargo test — workspace output is reversible. A system-path @@ -1737,6 +1743,13 @@ func classifyStage(tokens []string, pipedInto bool) RiskClass { if isEnvironmentDump(tokens) { return SystemWrite } + // Shell-builtin dumps: bare `set`, `export -p`, `declare -p`, and + // `typeset -p` print the full environment / all shell variables, + // including secrets not covered by redaction patterns. Same threat + // as `env` / `printenv` → system_write. + if builtinEnvDump(tokens) { + return SystemWrite + } cmdTokens, floor := unwrapWrappers(tokens) cls := floor if len(cmdTokens) > 0 { @@ -1844,6 +1857,42 @@ func isScriptEvalInterpreter(name string) bool { return false } +// builtinEnvDump reports whether tokens are a shell-builtin invocation +// that prints the environment or all shell variables: bare `set`, +// `set -o`, and `export`/`declare`/`typeset` run in `-p` (print) mode +// with no assignments. Setting variables or options (`export FOO=bar`, +// `set -e`, `declare -i x=5`) is not a dump. +func builtinEnvDump(tokens []string) bool { + if len(tokens) == 0 { + return false + } + switch commandName(tokens[0]) { + case "set": + if len(tokens) == 1 { + return true + } + // `set -o` prints all options; `set -o errexit` sets one. + return len(tokens) == 2 && tokens[1] == "-o" + case "export", "declare", "typeset": + sawPrint := false + for _, t := range tokens[1:] { + if strings.HasPrefix(t, "-") { + if strings.Contains(t, "p") { + sawPrint = true + } + continue + } + if isAssignment(t) { + continue + } + // A name operand in print mode is a targeted query, not a dump. + return false + } + return sawPrint + } + return false +} + // isEnvironmentDump reports whether tokens represent a bare `env` or // `printenv` invocation whose only effect is to dump the process environment. // `env FOO=bar cmd ...` is NOT a dump (the real command is classified @@ -2034,6 +2083,17 @@ func extractSubstitutions(cmd string) (string, []string) { i := 0 for i < len(cmd) { + // Inside double quotes a backslash escapes the next character: + // `\"` is a literal quote that must NOT toggle the double-quote + // state (same for \\, \$, \`). Without this, a single escaped + // quote desyncs the quote state and a later single-quoted span + // can hide a substitution body from extraction. + if cmd[i] == '\\' && inDouble && i+1 < len(cmd) && + (cmd[i+1] == '"' || cmd[i+1] == '\\' || cmd[i+1] == '$' || cmd[i+1] == '`') { + out.WriteString(cmd[i : i+2]) + i += 2 + continue + } // Double quotes toggle expansion context: inside them a `'` is data, // not a quote span (so an apostrophe in a double-quoted argument // cannot open a bogus single-quote span and hide later $()/backtick @@ -2268,6 +2328,28 @@ func isKnownCommandName(name string) bool { // Canonical `:(){ :|:& };:` and spaced `: () { : | : & } ; :` still match. var rawForkBombRe = regexp.MustCompile(`(^|[;&|\s]):\s*(?:\(\s*\)\s*)?\{[^}]*[|&][^}]*\}\s*;?\s*:`) +// namedForkBombShapeRe matches the outer shape of a function-definition +// fork bomb: `name(){ body-with-pipe-or-amp };name` (or with spaces / +// extra separators). The definition must start at a real command position +// — start of input or right after `;`, `&`, `|`, or a newline — not in +// argument position after another command's name (e.g. `echo bomb(){…}`). +// Backreferences are unsupported in RE2, so the name-equality and +// recursive-spawn checks are done in code over the captured groups. +var namedForkBombShapeRe = regexp.MustCompile(`(?s)(?:^|[;&|\n])(\w+)\s*(?:\(\s*\)\s*)?\{([^}]*)\}\s*;?\s*(\w+)(?:\s|$)`) + +// isNamedForkBomb reports whether a shape match is a genuine +// self-recursing fork bomb: the function defined, the function invoked +// after the body, and at least two self-references inside the body (a +// real bomb spawns itself more than once; a body calling it once with +// other work is not self-sustaining). +func isNamedForkBomb(m []string) bool { + defName, body, tailName := m[1], m[2], m[3] + if defName != tailName { + return false + } + return strings.Count(body, defName) >= 2 +} + // isRawBlocked checks the raw command string for patterns that are // blocked regardless of tokenization artifacts. func isRawBlocked(cmd string) bool { @@ -2275,7 +2357,15 @@ func isRawBlocked(cmd string) bool { if cmd == ":(){ :|:& };:" { return true } - return rawForkBombRe.MatchString(cmd) + if rawForkBombRe.MatchString(cmd) { + return true + } + for _, m := range namedForkBombShapeRe.FindAllStringSubmatch(cmd, -1) { + if isNamedForkBomb(m) { + return true + } + } + return false } // splitSegments splits token sequences on command separators. @@ -3549,9 +3639,6 @@ func isNetworkEgress(first string, tokens []string) bool { // gh subcommands inherently contact the GitHub API — the same class as // git's remote-contacting subcommands. Only meta invocations (help, // completion, version queries) stay local and fall through to Safe. - if first == "openssl" { - return opensslContactsRemote(tokens) - } if first == "gh" { skipNext := false for _, tok := range tokens[1:] { diff --git a/internal/danger/classifier_hardening_wave_test.go b/internal/danger/classifier_hardening_wave_test.go new file mode 100644 index 00000000..683a190e --- /dev/null +++ b/internal/danger/classifier_hardening_wave_test.go @@ -0,0 +1,135 @@ +package danger + +import ( + "strings" + "testing" + "time" +) + +// Hardening wave driven by the adversarial review of the danger classifier: +// env-dump builtins, dynamic substitutions in write verbs, clobber redirects, +// generalized fork bombs, double-quote escape desync, recursion depth, and +// the allowlist bypassing the raw-blocked floor. + +func TestHardeningEnvDumpBuiltins(t *testing.T) { + dumps := []string{ + "set", + "export -p", + "declare -p", + "typeset -p", + "set -o", // prints shell options; still a dump-style builtin invocation + } + for _, cmd := range dumps { + if got := Classify(cmd); got != SystemWrite { + t.Errorf("Classify(%q) = %v, want system_write (env/option dump)", cmd, got) + } + } + // Setting variables/options must stay un-escalated. + keeps := []string{"export FOO=bar", "set -e", "declare -i x=5"} + for _, cmd := range keeps { + if got := Classify(cmd); got == SystemWrite { + t.Errorf("Classify(%q) = system_write, want un-escalated", cmd) + } + } +} + +func TestHardeningDynamicSubstWriteVerb(t *testing.T) { + cases := []string{ + "chmod $(cat m) /tmp/x", + "ln -sf $(cat t) ~/.zshenv", + "cp $(cat src) /etc/cron.d/x", + "tee /etc/hosts < $(cat f)", + } + for _, cmd := range cases { + if got := Classify(cmd); Rank(got) < Rank(Unknown) { + t.Errorf("Classify(%q) = %v, want fail-closed (unknown or worse)", cmd, got) + } + } +} + +func TestHardeningClobberRedirects(t *testing.T) { + cases := []string{ + "printf x >| /etc/hosts", + "echo hi >| ~/.zshenv", + } + for _, cmd := range cases { + if got := Classify(cmd); Rank(got) < Rank(SystemWrite) { + t.Errorf("Classify(%q) = %v, want system_write (clobber redirect)", cmd, got) + } + } +} + +func TestHardeningGeneralizedForkBomb(t *testing.T) { + cases := []string{ + ":(){ :|:& };:", + "bomb(){ bomb|bomb& };bomb", + "bomb () { bomb | bomb & } ; bomb", + "f(){ f&f& };f", + } + for _, cmd := range cases { + if got := Classify(cmd); got != Blocked { + t.Errorf("Classify(%q) = %v, want blocked", cmd, got) + } + } + // Innocent lookalikes stay unblocked. + innocent := []string{ + "echo :{a}:", + "bomb(){ echo hi };bomb", + "echo bomb(){ bomb|bomb& };bomb", + } + for _, cmd := range innocent { + if got := Classify(cmd); got == Blocked { + t.Errorf("Classify(%q) = blocked, want allowed shape", cmd) + } + } +} + +func TestHardeningDoubleQuoteEscapeDesync(t *testing.T) { + // Inside double quotes \" is an escaped literal quote — it must not + // toggle the double-quote state, or a later single-quote span can hide + // a substitution body from extraction. + cmd := `arg "b \"c\" '$(touch /tmp/pwned)'"` + _, subs := extractSubstitutions(cmd) + found := false + for _, s := range subs { + if strings.Contains(s, "touch /tmp/pwned") { + found = true + } + } + if !found { + t.Fatalf("extractSubstitutions(%q) missed the substitution body; subs = %q", cmd, subs) + } +} + +func TestHardeningNestedSubstitutionDepthCap(t *testing.T) { + deep := strings.Repeat("$(", 5000) + "echo hi" + strings.Repeat(")", 5000) + done := make(chan RiskClass, 1) + go func() { done <- Classify(deep) }() + select { + case <-time.After(10 * time.Second): + t.Fatal("Classify did not terminate on deeply nested substitutions") + case got := <-done: + _ = got // any class is fine; termination is the point + } +} + +func TestHardeningTopMonitorSafe(t *testing.T) { + for _, cmd := range []string{"top", "top -b -n 1"} { + if got := Classify(cmd); got != Safe { + t.Errorf("Classify(%q) = %v, want safe (read-only process monitor)", cmd, got) + } + } +} + +func TestHardeningAllowlistDoesNotBypassRawBlocked(t *testing.T) { + cfg := &DangerousConfig{ + Allowlist: []string{":(){ :|:& };:"}, + DefaultAction: func() *string { + s := "prompt" + return &s + }(), + } + if got := cfg.ActionForCommand(":(){ :|:& };:"); got == Allow { + t.Fatal("allowlisted fork bomb must not return Allow; the raw-blocked floor must run first") + } +} From dbcf7cad3856c9b977f49ef138e30792dede95b4 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Fri, 25 Sep 2026 21:40:58 +0200 Subject: [PATCH 2/3] danger: close declare -x dump bypass; fork-bomb requires spawn - Flag-only declare/typeset -x prints all exported vars -> system_write; assignments or name operands keep the declaration semantics - isNamedForkBomb requires a pipe/ampersand in the body so plain recursive functions are not blocked --- internal/danger/classifier.go | 19 +++++++- internal/danger/classifier_review_fix_test.go | 43 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 internal/danger/classifier_review_fix_test.go diff --git a/internal/danger/classifier.go b/internal/danger/classifier.go index 7813724e..ba9fc17c 100644 --- a/internal/danger/classifier.go +++ b/internal/danger/classifier.go @@ -1875,20 +1875,31 @@ func builtinEnvDump(tokens []string) bool { return len(tokens) == 2 && tokens[1] == "-o" case "export", "declare", "typeset": sawPrint := false + sawExport := false + flagOnly := true for _, t := range tokens[1:] { if strings.HasPrefix(t, "-") { if strings.Contains(t, "p") { sawPrint = true } + if strings.Contains(t, "x") { + sawExport = true + } continue } if isAssignment(t) { + // `declare -x FOO=bar` declares, not dumps. + flagOnly = false continue } // A name operand in print mode is a targeted query, not a dump. return false } - return sawPrint + // Flag-only `-p` prints all variables; flag-only `-x` on + // declare/typeset prints all exported variables (bash/zsh both). + // Either is a full-environment dump; any assignment makes it a + // declaration instead. + return flagOnly && (sawPrint || sawExport) } return false } @@ -2347,6 +2358,12 @@ func isNamedForkBomb(m []string) bool { if defName != tailName { return false } + // A fork bomb spawns itself concurrently: the body must contain a + // pipe or ampersand at all. Without one the worst case is a plain + // recursive function (a benign pattern), never unbounded spawning. + if !strings.ContainsAny(body, "|&") { + return false + } return strings.Count(body, defName) >= 2 } diff --git a/internal/danger/classifier_review_fix_test.go b/internal/danger/classifier_review_fix_test.go new file mode 100644 index 00000000..3361b771 --- /dev/null +++ b/internal/danger/classifier_review_fix_test.go @@ -0,0 +1,43 @@ +package danger + +import "testing" + +// Review follow-ups from the adversarial diff pass. + +func TestReviewDeclareXDump(t *testing.T) { + // Flag-only declare/typeset -x prints all exported variables. + for _, cmd := range []string{"declare -x", "typeset -x"} { + if got := Classify(cmd); got != SystemWrite { + t.Errorf("Classify(%q) = %v, want system_write (exported-var dump)", cmd, got) + } + } + // Declaring/exporting variables stays un-escalated. + for _, cmd := range []string{"declare -x FOO=bar", "typeset -i x=5", "declare -x FOO"} { + if got := Classify(cmd); got == SystemWrite { + t.Errorf("Classify(%q) = system_write, want un-escalated", cmd) + } + } +} + +func TestReviewForkBombRequiresSpawn(t *testing.T) { + // A function body that references the name twice without any + // pipe/ampersand spawn is not a fork bomb. + for _, cmd := range []string{ + "x(){ echo x; echo x }; x", + "hello(){ echo hello; echo hello }; hello", + } { + if got := Classify(cmd); got == Blocked { + t.Errorf("Classify(%q) = blocked, want allowed (no spawn in body)", cmd) + } + } + // Real bombs with named functions stay blocked. + for _, cmd := range []string{ + "bomb(){ bomb|bomb& };bomb", + "f(){ f&f& };f", + "bomb(){ bomb & bomb }; bomb", + } { + if got := Classify(cmd); got != Blocked { + t.Errorf("Classify(%q) = %v, want blocked", cmd, got) + } + } +} From 617258390a2aeab2ba12f56c6539f01398e31dc0 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Fri, 25 Sep 2026 22:07:20 +0200 Subject: [PATCH 3/3] danger: cap substitution reclassification depth at 64 levels (fail closed as Unknown past cap) --- internal/danger/classifier.go | 29 ++++++++++++++++--- .../danger/classifier_hardening_wave_test.go | 11 +++++-- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/internal/danger/classifier.go b/internal/danger/classifier.go index ba9fc17c..4f6542bb 100644 --- a/internal/danger/classifier.go +++ b/internal/danger/classifier.go @@ -1400,7 +1400,15 @@ var safeCommands = map[string]bool{ // and classifies each (see classifyPipeline/classifyStage). Every extracted // sub-expression is re-classified through Classify so nested commands cannot // hide one level deeper; the worst class across the whole tree is returned. -func Classify(cmd string) RiskClass { +// maxSubstDepth caps recursive re-classification of nested command +// substitutions. Real commands nest a handful deep; hundreds of levels +// are hostile input, and classifying each level re-normalizes the whole +// remaining string (quadratic). Past the cap the classifier fails closed +// instead of burning time. +const maxSubstDepth = 64 + +// classifyAtDepth is Classify with a nesting-depth budget. +func classifyAtDepth(cmd string, depth int) RiskClass { cmd = strings.TrimSpace(cmd) if cmd == "" { return Safe @@ -1411,19 +1419,32 @@ func Classify(cmd string) RiskClass { return Blocked } + if depth > maxSubstDepth { + return Unknown + } + main, subs := normalize(cmd) worst := classifyOne(main) for _, s := range subs { // Substitutions are themselves commands the shell will run. - // Re-enter Classify (not classifyOne) so nested substitutions - // inside them also normalise. - if r := Classify(s); Rank(r) > Rank(worst) { + // Re-enter the depth-tracked classifier (not classifyOne) so + // nested substitutions inside them also normalise. + if r := classifyAtDepth(s, depth+1); Rank(r) > Rank(worst) { worst = r } } return worst } +// Classify returns the worst risk class found in cmd after normalisation +// (shell evasion tricks, substitutions, wrappers, basenames) and token +// classification (see classifyOne). Every extracted sub-expression is +// re-classified recursively, bounded by maxSubstDepth; deeper nesting +// fails closed as Unknown. +func Classify(cmd string) RiskClass { + return classifyAtDepth(cmd, 0) +} + // classifyOne runs the existing token-level pipeline against an already- // normalised command string. func classifyOne(cmd string) RiskClass { diff --git a/internal/danger/classifier_hardening_wave_test.go b/internal/danger/classifier_hardening_wave_test.go index 683a190e..0927e035 100644 --- a/internal/danger/classifier_hardening_wave_test.go +++ b/internal/danger/classifier_hardening_wave_test.go @@ -106,10 +106,17 @@ func TestHardeningNestedSubstitutionDepthCap(t *testing.T) { done := make(chan RiskClass, 1) go func() { done <- Classify(deep) }() select { - case <-time.After(10 * time.Second): + case <-time.After(5 * time.Second): t.Fatal("Classify did not terminate on deeply nested substitutions") case got := <-done: - _ = got // any class is fine; termination is the point + // Past the substitution-depth cap the classifier fails closed. + if Rank(got) < Rank(Unknown) { + t.Errorf("Classify(deep nesting) = %v, want unknown (depth cap fail-closed)", got) + } + } + // Reasonable nesting still classifies normally. + if got := Classify("echo $(echo $(echo hi))"); got == Unknown { + t.Error("Classify(3-deep nesting) = unknown, want normal classification") } }