Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/scan/src/__tests__/false-positives.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
16 changes: 16 additions & 0 deletions packages/scan/src/code-rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Loading