diff --git a/internal/keyring/keyring.go b/internal/keyring/keyring.go index 7f116b50f..3c9f41166 100644 --- a/internal/keyring/keyring.go +++ b/internal/keyring/keyring.go @@ -3,11 +3,13 @@ // macOS and `secret-tool` (libsecret) on Linux. It stores a single secret string // per (service, account). Windows and other platforms report unsupported. // -// It shells out to the OS tools rather than taking a third-party dependency. On -// macOS the secret is passed to `security` as an argument, so it is briefly -// visible to other processes via the process list; on Linux the secret is passed -// over stdin and is not exposed in the argument vector. Callers that need to keep -// a secret out of the process list on macOS should prefer the file backend. +// It shells out to the OS tools rather than taking a third-party dependency. +// On both macOS and Linux the secret is passed over stdin, never the argument +// vector, so it is not exposed via the process list. On macOS the write goes +// through `security -i` (interactive mode), whose command parser is line-based +// with a fixed 4096-byte line buffer, so Set rejects secrets containing +// newlines or exceeding that budget rather than silently corrupting them; +// Linux's secret-tool has no such restriction. package keyring import ( @@ -63,8 +65,28 @@ func (k *Keyring) Set(service, account, secret string) error { } switch k.goos { case "darwin": + // The secret must stay out of the argument vector (visible to every + // process via `ps`), but a trailing -w prompt is not a substitute: + // security prompts with getpass(3), which reads from /dev/tty whenever + // the process has a controlling terminal (ignoring a piped stdin) and + // truncates at getpass's small fixed buffer otherwise. Instead drive + // `security -i`: interactive mode reads whole commands from stdin, so + // argv is just ["-i"] and the secret rides inside the command line. + // That parser is line-based with a fixed 4096-byte buffer, so reject + // newlines and oversized payloads up front; an overlong line would be + // split and executed as two garbage commands, silently corrupting the + // stored value. + for _, s := range []string{service, account, secret} { + if strings.ContainsAny(s, "\r\n") { + return wrap("set", errors.New("service, account, and secret must not contain newlines on macOS (security -i is line-based)")) + } + } // -U updates the item if it already exists rather than failing. - _, err := k.exec(nil, "security", "add-generic-password", "-U", "-s", service, "-a", account, "-w", secret) + line := "add-generic-password -U -s " + securityQuote(service) + " -a " + securityQuote(account) + " -w " + securityQuote(secret) + "\n" + if len(line) > securityMaxLine { + return wrap("set", fmt.Errorf("secret too large for the macOS security tool's %d-byte command line; use the file backend instead", securityMaxLine)) + } + _, err := k.exec([]byte(line), "security", "-i") return wrap("set", err) case "linux": // secret-tool reads the secret from stdin, keeping it out of the argv. @@ -140,6 +162,19 @@ func (k *Keyring) Delete(service, account string) (bool, error) { } } +// securityMaxLine is the most `security -i` can read as one command: its line +// buffer is MAX_LINE_LEN (4096) bytes in Apple's SecurityTool, one of which the +// terminating NUL consumes. +const securityMaxLine = 4095 + +// securityQuote wraps s for one argument of a `security -i` command line. The +// tool's parser (split_line in Apple's SecurityTool) treats a backslash inside +// double quotes as escaping the next character and the matching quote as the +// argument terminator, so those two characters are all that needs escaping. +func securityQuote(s string) string { + return `"` + strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(s) + `"` +} + func (k *Keyring) exec(stdin []byte, name string, args ...string) ([]byte, error) { ctx, cancel := context.WithTimeout(context.Background(), commandTimeout) defer cancel() diff --git a/internal/keyring/keyring_test.go b/internal/keyring/keyring_test.go index 01a09ebfc..430d402ed 100644 --- a/internal/keyring/keyring_test.go +++ b/internal/keyring/keyring_test.go @@ -50,6 +50,52 @@ func attrValue(args []string, attr string) string { func key(service, account string) string { return service + "\x00" + account } +// splitSecurityLine mirrors split_line in Apple's SecurityTool so the fake +// parses `security -i` command lines the way the real tool does: whitespace +// separates arguments, double or single quotes group one, and a backslash +// escapes the next character both inside and outside quotes. +func splitSecurityLine(line string) []string { + var args []string + var cur strings.Builder + inArg, escaped := false, false + quote := byte(0) + for i := 0; i < len(line); i++ { + c := line[i] + switch { + case escaped: + cur.WriteByte(c) + escaped = false + case c == '\\': + escaped = true + inArg = true + case quote != 0: + if c != quote { + cur.WriteByte(c) + continue + } + args = append(args, cur.String()) + cur.Reset() + inArg, quote = false, 0 + case c == '"' || c == '\'': + quote = c + inArg = true + case c == ' ' || c == '\t': + if inArg { + args = append(args, cur.String()) + cur.Reset() + inArg = false + } + default: + cur.WriteByte(c) + inArg = true + } + } + if inArg { + args = append(args, cur.String()) + } + return args +} + func (f *fakeKeyring) run(_ context.Context, name string, stdin []byte, args ...string) ([]byte, error) { f.lastStdin = string(stdin) f.lastArgs = append([]string{name}, args...) @@ -58,10 +104,19 @@ func (f *fakeKeyring) run(_ context.Context, name string, stdin []byte, args ... } switch f.goos { case "darwin": - svc, acct := flagValue(args, "-s"), flagValue(args, "-a") - switch args[0] { + cmdArgs := args + if args[0] == "-i" { + // Interactive mode: the command arrives as one line on stdin. + line, _, _ := strings.Cut(string(stdin), "\n") + cmdArgs = splitSecurityLine(line) + if len(cmdArgs) == 0 { + return nil, fakeExit{1} + } + } + svc, acct := flagValue(cmdArgs, "-s"), flagValue(cmdArgs, "-a") + switch cmdArgs[0] { case "add-generic-password": - f.data[key(svc, acct)] = flagValue(args, "-w") + f.data[key(svc, acct)] = flagValue(cmdArgs, "-w") return nil, nil case "find-generic-password": if v, ok := f.data[key(svc, acct)]; ok { @@ -129,6 +184,112 @@ func TestKeyringRoundTripDarwin(t *testing.T) { } } +func TestKeyringRoundTripDarwinUsesStdin(t *testing.T) { + f := newFake("darwin") + k := f.keyring() + if err := k.Set("zero", "tokens", "blob-CCC"); err != nil { + t.Fatalf("Set: %v", err) + } + // The secret must travel via stdin, never the argument vector: the write + // goes through `security -i`, whose whole command line (secret included) + // arrives on stdin while argv carries only the -i flag. + wantStdin := "add-generic-password -U -s \"zero\" -a \"tokens\" -w \"blob-CCC\"\n" + if f.lastStdin != wantStdin { + t.Fatalf("secret not sent via stdin correctly: stdin=%q, want=%q", f.lastStdin, wantStdin) + } + wantArgs := []string{"security", "-i"} + if len(f.lastArgs) != len(wantArgs) || f.lastArgs[0] != wantArgs[0] || f.lastArgs[1] != wantArgs[1] { + t.Fatalf("argv = %v, want %v", f.lastArgs, wantArgs) + } + got, ok, err := k.Get("zero", "tokens") + if err != nil || !ok || got != "blob-CCC" { + t.Fatalf("Get = %q ok=%v err=%v", got, ok, err) + } + existed, err := k.Delete("zero", "tokens") + if err != nil || !existed { + t.Fatalf("Delete: existed=%v err=%v", existed, err) + } +} + +// TestKeyringSetRejectsMultilineSecretOnDarwin guards against silently +// truncating a secret containing a newline: `security -i` reads one command +// per line, so an embedded newline would split the write into two garbage +// commands. Set must reject this before ever invoking security, not store a +// corrupted value. +func TestKeyringSetRejectsMultilineSecretOnDarwin(t *testing.T) { + for _, secret := range []string{"line1\nline2", "line1\r\nline2", "trailing\n"} { + f := newFake("darwin") + k := f.keyring() + if err := k.Set("zero", "tokens", secret); err == nil { + t.Fatalf("Set(%q) = nil error, want rejection of the embedded newline", secret) + } + if f.lastArgs != nil { + t.Fatalf("Set(%q) should be rejected before invoking security, got args=%v", secret, f.lastArgs) + } + } +} + +// TestKeyringDarwinQuotesSpecialCharacters proves the quoting survives the +// real tool's parser: the fake tokenizes stdin with a faithful mirror of +// SecurityTool's split_line, so a secret full of quotes, backslashes, and +// spaces must round-trip unchanged. +func TestKeyringDarwinQuotesSpecialCharacters(t *testing.T) { + for _, secret := range []string{ + `spa ced`, + `quo"te`, + `back\slash`, + `sin'gle`, + `mi"x'ed \" \\ end\`, + ` leading and trailing `, + } { + f := newFake("darwin") + k := f.keyring() + if err := k.Set("zero", "tokens", secret); err != nil { + t.Fatalf("Set(%q): %v", secret, err) + } + got, ok, err := k.Get("zero", "tokens") + if err != nil || !ok || got != secret { + t.Fatalf("Get after Set(%q) = %q ok=%v err=%v", secret, got, ok, err) + } + } +} + +// TestKeyringSetRejectsOversizedSecretOnDarwin guards the `security -i` line +// budget: MAX_LINE_LEN is 4096 bytes, and an overlong line would be split and +// executed as two garbage commands. A payload comfortably under the budget +// must succeed; one over it must be rejected before invoking security. +func TestKeyringSetRejectsOversizedSecretOnDarwin(t *testing.T) { + f := newFake("darwin") + k := f.keyring() + if err := k.Set("zero", "tokens", strings.Repeat("a", 4000)); err != nil { + t.Fatalf("Set(4000 bytes): %v", err) + } + f = newFake("darwin") + k = f.keyring() + if err := k.Set("zero", "tokens", strings.Repeat("a", 5000)); err == nil { + t.Fatal("Set(5000 bytes) = nil error, want rejection of the oversized line") + } + if f.lastArgs != nil { + t.Fatalf("oversized Set should be rejected before invoking security, got args=%v", f.lastArgs) + } +} + +// TestKeyringSetAllowsMultilineSecretOnLinux confirms the newline restriction +// is macOS-specific: secret-tool reads the whole stdin payload as the secret, +// with no line-based prompt to corrupt an embedded newline. +func TestKeyringSetAllowsMultilineSecretOnLinux(t *testing.T) { + f := newFake("linux") + k := f.keyring() + secret := "line1\nline2" + if err := k.Set("zero", "tokens", secret); err != nil { + t.Fatalf("Set: %v", err) + } + got, ok, err := k.Get("zero", "tokens") + if err != nil || !ok || got != secret { + t.Fatalf("Get = %q ok=%v err=%v, want %q", got, ok, err, secret) + } +} + func TestKeyringRoundTripLinuxUsesStdin(t *testing.T) { f := newFake("linux") k := f.keyring()