diff --git a/packages/scan/src/__tests__/false-positives.test.ts b/packages/scan/src/__tests__/false-positives.test.ts index 844ae0f..d23ef54 100644 --- a/packages/scan/src/__tests__/false-positives.test.ts +++ b/packages/scan/src/__tests__/false-positives.test.ts @@ -349,6 +349,33 @@ describe('a constant HTML assignment that shares its line', () => { }); }); +describe('a Go shell call whose whole argv is literal', () => { + // Verbatim from SibtainOcn/Quiesce, a local Windows CLI. A whole-repository + // scan returned exactly one finding and this was it: a shell invocation + // reading the OS version, with nothing in the argv for anyone to influence. + it('does not flag exec.Command with an all-literal argv', () => { + expect(ruleIds('ui.go', 'out, err := exec.Command("cmd", "/c", "ver").Output()')).toEqual([]); + expect(ruleIds('a.go', 'exec.Command("sh", "-c", "ls -la").Run()')).toEqual([]); + expect(ruleIds('a.go', 'exec.CommandContext(ctx, "bash", "-c", "echo \\"hi\\"").Run()')).toEqual( + [], + ); + }); + + it('still flags one the caller can reach into', () => { + // Concatenation, a formatter and a bare identifier: the three ways the + // literal stops being the whole of it. + expect(ruleIds('a.go', 'exec.Command("sh", "-c", "ping -c 1 "+host).Output()')).toContain( + 'go-shell-exec-command', + ); + expect( + ruleIds('a.go', 'exec.Command("sh", "-c", fmt.Sprintf("ping %s", host)).Output()'), + ).toContain('go-shell-exec-command'); + expect(ruleIds('a.go', 'exec.Command("sh", "-c", command).Output()')).toContain( + 'go-shell-exec-command', + ); + }); +}); + describe('findings this triage deliberately left alone', () => { // Suppressing these would be the scanner talking itself out of real classes. it('still reports a nested quantifier, a lifecycle script and a live HTML sink', () => { diff --git a/packages/scan/src/code-rules.ts b/packages/scan/src/code-rules.ts index 7fd4661..1013642 100644 --- a/packages/scan/src/code-rules.ts +++ b/packages/scan/src/code-rules.ts @@ -481,6 +481,22 @@ export const CODE_RULES: readonly CodeRule[] = [ severity: 'critical', languages: ['go'], pattern: /\bexec\.Command(?:Context)?\s*\(\s*(?:ctx\s*,\s*)?"(?:\/bin\/)?(?:sh|bash|zsh|cmd|powershell)"\s*,\s*"(?:-c|\/c)"/, + // A call whose whole argv is string literals cannot be injected into. + // `exec.Command("cmd", "/c", "ver")` reads the Windows version; there is no + // value in it for an attacker to reach, and the consequence above — that a + // shell will interpret metacharacters — describes metacharacters nobody can + // supply. gosec's G204 draws the line in the same place, and this was the + // single finding a Go project got out of a whole scan before declining the + // offer, which is an expensive way to report nothing. + // + // The guard has to end at the closing paren, so a literal followed by + // anything else still reports: `"ls " + dir` leaves a `+` before the `)`, + // `fmt.Sprintf(…)` leaves an identifier, and a bare variable leaves a name. + // `(?:[^"\\]|\\.)*` rather than `[^"]*` so an escaped quote inside a + // literal — `"echo \"hi\""` — does not end the literal early and drop the + // guard on a line it should have covered. + lineGuard: + /\bexec\.Command(?:Context)?\s*\(\s*(?:ctx\s*,\s*)?"(?:\/bin\/)?(?:sh|bash|zsh|cmd|powershell)"\s*,\s*"(?:-c|\/c)"\s*(?:,\s*"(?:[^"\\]|\\.)*")*\s*,?\s*\)/, }, { id: 'rb-backtick-interpolation',