diff --git a/.gitattributes b/.gitattributes index a8e0f82..f9cc277 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,9 +1,30 @@ * text=auto eol=lf -*.png binary -*.jpg binary -*.jpeg binary -*.gif binary -*.ico binary -*.jar binary +*.png binary +*.jpg binary +*.jpeg binary +*.gif binary +*.ico binary *.tiktoken binary + +# Web fonts — must never receive CRLF normalisation +*.woff binary +*.woff2 binary +*.ttf binary +*.otf binary +*.eot binary + +# WebAssembly bytecode +*.wasm binary + +# Compressed archives +*.zip binary +*.jar binary +*.tar binary +*.gz binary +*.tgz binary + +# Other binary image formats +*.webp binary +*.bmp binary +*.tiff binary diff --git a/internal/llm/keycmd.go b/internal/llm/keycmd.go index 306cf2f..6f03b5e 100644 --- a/internal/llm/keycmd.go +++ b/internal/llm/keycmd.go @@ -10,6 +10,7 @@ import ( "fmt" "os" "os/exec" + "regexp" "strings" "time" ) @@ -30,6 +31,40 @@ const keyCmdMaxOutput = 64 << 10 // reaches the caller: cappedBuffer.overflow is what produces the error message. var errKeyCmdOutputTooLarge = errors.New("credential command output exceeds cap") +// suspiciousPatterns matches shell constructs that never appear in a legitimate +// credential-helper invocation (op, aws, pass, keychain, ...) and so signal +// config tampering or an accidental compound command. This is defence-in-depth, +// not a sandbox: api_key_cmd runs arbitrary user config by design, so the goal +// is only to surface accidental misconfiguration. Command separators (; && || &) +// and redirections (2>/dev/null) are intentionally omitted -- they appear in +// documented, tested helper commands and would produce false positives. +var suspiciousPatterns = []*regexp.Regexp{ + regexp.MustCompile("`"), // backtick command substitution + regexp.MustCompile(`\x00`), // embedded NUL byte +} + +// validateKeyCmd returns an error when cmd contains patterns that are almost +// certainly not part of a legitimate credential-helper invocation. It is called +// before the shell runs so that tampered or malformed config produces a clear +// diagnostic instead of silently executing destructive commands. +func validateKeyCmd(cmd, label string) error { + for _, re := range suspiciousPatterns { + if loc := re.FindStringIndex(cmd); loc != nil { + end := loc[1] + 8 + if end > len(cmd) { + end = len(cmd) + } + return fmt.Errorf( + "%s contains a suspicious shell pattern at offset %d (%q); "+ + "api_key_cmd / auth_token_cmd must be a single credential-helper "+ + "invocation (e.g. \"op read op://vault/item\"), not a compound shell expression", + label, loc[0], cmd[loc[0]:end], + ) + } + } + return nil +} + // cappedBuffer collects at most max bytes and records whether more were offered. // Refusing the write makes os/exec's copier close the pipe, so a runaway command // (`cat /dev/urandom`) dies of SIGPIPE instead of growing our heap without bound. @@ -57,6 +92,9 @@ func (b *cappedBuffer) Write(p []byte) (int, error) { // fallback. The resolved credential is used in memory only and is never written // to config or logged. func resolveKeyCmd(cmd, label string) (string, error) { + if err := validateKeyCmd(cmd, label); err != nil { + return "", err + } ctx, cancel := context.WithTimeout(context.Background(), keyCmdTimeout) defer cancel() diff --git a/internal/llm/keycmd_test.go b/internal/llm/keycmd_test.go index 19139df..bd87d05 100644 --- a/internal/llm/keycmd_test.go +++ b/internal/llm/keycmd_test.go @@ -45,6 +45,10 @@ func TestResolveKeyCmd(t *testing.T) { // takes the edges). {name: "interior tab kept", cmd: "printf 'sk-a\\tb\\n'", want: "sk-a\tb"}, {name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"}, + // validateKeyCmd runs before the shell: a backtick or a literal NUL byte in + // the command string is rejected with a diagnostic, never executed. + {name: "backtick substitution rejected", cmd: "printf %s `id`", wantErr: "suspicious shell pattern"}, + {name: "nul byte in command rejected", cmd: "printf %s\x00", wantErr: "suspicious shell pattern"}, // Boundary: exactly the cap is fine, one byte more is refused. The child // dies of SIGPIPE as soon as we stop accepting, so this stays fast. {name: "output exactly at cap", cmd: "head -c 65536 /dev/zero | tr '\\0' a", want: strings.Repeat("a", keyCmdMaxOutput)}, @@ -73,6 +77,40 @@ func TestResolveKeyCmd(t *testing.T) { } } +func TestValidateKeyCmd(t *testing.T) { + label := `api_key_cmd for provider "x"` + + // Legitimate helper invocations pass untouched, including ones that use the + // separators and redirections deliberately left out of suspiciousPatterns. + for _, cmd := range []string{ + "op read op://vault/item/credential", + "aws secretsmanager get-secret-value --query SecretString --output text", + "pass show api/openai 2>/dev/null", + `read -r x; printf %s "$x"`, + } { + if err := validateKeyCmd(cmd, label); err != nil { + t.Errorf("validateKeyCmd(%q) = %v, want nil", cmd, err) + } + } + + // A backtick is reported with its byte offset and the source label so a + // tampered config produces an actionable diagnostic. + err := validateKeyCmd("printf %s `id`", label) + if err == nil { + t.Fatal("validateKeyCmd with backtick = nil, want error") + } + for _, want := range []string{"suspicious shell pattern", "offset 10", label} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not contain %q", err.Error(), want) + } + } + + // A literal NUL byte in the command string is rejected too. + if err := validateKeyCmd("printf %s\x00", label); err == nil { + t.Error("validateKeyCmd with NUL byte = nil, want error") + } +} + func TestResolveKeyCmd_Timeout(t *testing.T) { origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay keyCmdTimeout = 50 * time.Millisecond