From 16284ba78fba953d86f14c8fc301bdedfbcb03fe Mon Sep 17 00:00:00 2001 From: "exe.dev user" Date: Sat, 8 Aug 2026 23:27:11 +0000 Subject: [PATCH] fix(security): strip PM_CLI_BRIDGE_PASSWORD from child process env mail watch --exec built its child environment with append(os.Environ(), ...), so the Bridge password supplied via PM_CLI_BRIDGE_PASSWORD was inherited by the user-supplied command and by everything that command shelled out to. Any third-party triage script could read the mail credential. The env-var credential path and this spawn site were each fine on their own; the exposure only exists once both are present, which is first true in 0.2.6. It is therefore introduced by the unreleased version rather than pre-existing, and is fixed before the release goes out. Adds config.ScrubSecrets, which removes pm-cli credential variables from an environment slice, and routes the exec site through it. The secret list lives next to the variable it names, so a future credential variable is covered by adding one entry. Matching on the exact variable name rather than a prefix keeps unrelated names such as PM_CLI_BRIDGE_PASSWORD_BACKUP intact; bare entries with no "=" are dropped, since that is the safe direction. Tested with a real child process: the scrubbed environment exposes nothing, while the unscrubbed one reproduces the leak. Also documents the environment-variable credential path in SECURITY.md, which described only the keyring. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 5 ++ SECURITY.md | 18 ++++ internal/cli/mail.go | 6 +- internal/config/config.go | 41 +++++++++ internal/config/scrub_secrets_test.go | 120 ++++++++++++++++++++++++++ 5 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 internal/config/scrub_secrets_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 543ad02..f9c17b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,11 @@ Targeted for the 0.2.6 release. - The COPY no-match check is gated on the server advertising UIDPLUS, since COPYUID is only guaranteed there; without the guard every successful copy on a non-UIDPLUS server would have been reported as a failure (#17). +- `mail watch --exec` no longer passes `PM_CLI_BRIDGE_PASSWORD` to the command it + runs. The child environment was built from `os.Environ()`, so a Bridge password + supplied via the environment variable added in this release was inherited by the + user-supplied command and by anything it shelled out to. Child environments are now + built through `config.ScrubSecrets`. ### Changed - `mail list --unread` now returns up to `--limit` unread messages. Previously the diff --git a/SECURITY.md b/SECURITY.md index a63fa81..aa276f8 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -28,6 +28,24 @@ If you discover a security vulnerability in pm-cli, please report it responsibly - Config file permissions are set to `0600` (owner read/write only) - Config directory permissions are set to `0700` +#### Environment variable credentials + +`PM_CLI_BRIDGE_PASSWORD` supplies the Bridge password directly, for headless +environments with no secret service available. When set and non-empty it takes +precedence over the keyring. + +This trades some protection for portability, and the tradeoff should be a +deliberate choice: + +- The value is readable by any process running as the same user, and appears in + `/proc//environ` on Linux. +- It may be captured by shell history or process listings depending on how it is set. +- pm-cli removes it from the environment of child processes it spawns (see + `config.ScrubSecrets`), so `mail watch --exec` commands do not inherit it. + +Prefer the system keyring on interactive machines. Where the variable is required, +inject it from a secrets manager rather than a shell profile. + ### Network Security - All connections to Proton Bridge use TLS encryption diff --git a/internal/cli/mail.go b/internal/cli/mail.go index a5398ec..8edaf52 100644 --- a/internal/cli/mail.go +++ b/internal/cli/mail.go @@ -1943,7 +1943,11 @@ func (c *MailWatchCmd) executeCommand(ctx *Context, msg imap.MessageSummary) { ctx.Formatter.Verbosef("Executing: %s", cmdStr) cmd := exec.Command("sh", "-c", cmdStr) - cmd.Env = append(os.Environ(), + // Build the child environment from ScrubSecrets, never os.Environ() + // directly: a Bridge password supplied via PM_CLI_BRIDGE_PASSWORD would + // otherwise be inherited by this user-supplied command and by everything + // it shells out to. + cmd.Env = append(config.ScrubSecrets(os.Environ()), fmt.Sprintf("PM_MSG_SEQ=%d", msg.SeqNum), fmt.Sprintf("PM_MSG_UID=%d", msg.UID), "PM_MSG_FROM="+safetext.SanitizeHeaderValue(msg.From), diff --git a/internal/config/config.go b/internal/config/config.go index 374ebcd..ded71d2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" "github.com/zalando/go-keyring" @@ -163,6 +164,46 @@ func DeletePassword(email string) error { return keyring.Delete(AppName, email) } +// secretEnvVars lists environment variables that carry pm-cli credentials and +// must never be handed to a child process. +var secretEnvVars = []string{EnvBridgePassword} + +// ScrubSecrets returns env with every pm-cli credential variable removed. +// +// Any code spawning a child process must build its environment from this +// rather than from os.Environ() directly. Without it, a Bridge password +// supplied via EnvBridgePassword is inherited by user-supplied commands (for +// example `mail watch --exec`), handing the mail credential to arbitrary +// third-party scripts. +// +// The input slice is not modified. +func ScrubSecrets(env []string) []string { + out := make([]string, 0, len(env)) + for _, kv := range env { + if isSecretEnv(kv) { + continue + } + out = append(out, kv) + } + return out +} + +// isSecretEnv reports whether a "KEY=VALUE" entry names a credential variable. +// A bare "KEY" with no "=" is matched too, since Go permits such entries and +// dropping them is the safe direction. +func isSecretEnv(kv string) bool { + name := kv + if i := strings.IndexByte(kv, '='); i >= 0 { + name = kv[:i] + } + for _, secret := range secretEnvVars { + if name == secret { + return true + } + } + return false +} + func Exists() bool { path, err := ConfigPath() if err != nil { diff --git a/internal/config/scrub_secrets_test.go b/internal/config/scrub_secrets_test.go new file mode 100644 index 0000000..f1849c4 --- /dev/null +++ b/internal/config/scrub_secrets_test.go @@ -0,0 +1,120 @@ +package config + +import ( + "os" + "os/exec" + "strings" + "testing" +) + +func TestScrubSecretsRemovesBridgePassword(t *testing.T) { + env := []string{ + "PATH=/usr/bin", + EnvBridgePassword + "=super-secret", + "HOME=/home/user", + } + + got := ScrubSecrets(env) + + for _, kv := range got { + if strings.HasPrefix(kv, EnvBridgePassword+"=") { + t.Fatalf("ScrubSecrets left the credential in place: %q", kv) + } + if strings.Contains(kv, "super-secret") { + t.Fatalf("ScrubSecrets leaked the secret value: %q", kv) + } + } + if len(got) != 2 { + t.Errorf("expected 2 surviving entries, got %d: %v", len(got), got) + } +} + +func TestScrubSecretsKeepsUnrelatedVars(t *testing.T) { + env := []string{"PATH=/usr/bin", "PM_MSG_FROM=a@b.test", "LANG=C"} + + got := ScrubSecrets(env) + + if len(got) != len(env) { + t.Fatalf("unrelated vars were dropped: %v", got) + } + for i := range env { + if got[i] != env[i] { + t.Errorf("entry %d changed: got %q, want %q", i, got[i], env[i]) + } + } +} + +// TestScrubSecretsDoesNotMatchPrefixes guards against dropping variables that +// merely start with the secret's name. +func TestScrubSecretsDoesNotMatchPrefixes(t *testing.T) { + similar := EnvBridgePassword + "_BACKUP=keep-me" + got := ScrubSecrets([]string{similar}) + + if len(got) != 1 || got[0] != similar { + t.Errorf("a similarly-named variable was dropped: %v", got) + } +} + +// TestScrubSecretsBareName covers an entry with no "=", which Go permits. +func TestScrubSecretsBareName(t *testing.T) { + got := ScrubSecrets([]string{EnvBridgePassword, "PATH=/usr/bin"}) + + for _, kv := range got { + if kv == EnvBridgePassword { + t.Error("bare credential name survived scrubbing") + } + } +} + +func TestScrubSecretsDoesNotMutateInput(t *testing.T) { + env := []string{"PATH=/usr/bin", EnvBridgePassword + "=secret"} + _ = ScrubSecrets(env) + + if len(env) != 2 || env[1] != EnvBridgePassword+"=secret" { + t.Errorf("input slice was modified: %v", env) + } +} + +func TestScrubSecretsEmpty(t *testing.T) { + if got := ScrubSecrets(nil); len(got) != 0 { + t.Errorf("expected empty result, got %v", got) + } +} + +// TestScrubbedEnvironNotVisibleToChild is the end-to-end guard for the actual +// leak: a real child process must not be able to read the Bridge password. +func TestScrubbedEnvironNotVisibleToChild(t *testing.T) { + t.Setenv(EnvBridgePassword, "bridge-secret-value") + + // Mirrors MailWatchCmd.executeCommand's environment construction. + cmd := exec.Command("sh", "-c", `printf '%s' "$`+EnvBridgePassword+`"`) + cmd.Env = append(ScrubSecrets(os.Environ()), "PM_MSG_SEQ=1") + + out, err := cmd.Output() + if err != nil { + t.Fatalf("child failed: %v", err) + } + if len(out) != 0 { + t.Errorf("child could read the Bridge password: %q", out) + } +} + +// TestUnscrubbedEnvironWouldLeak pins the regression this fix prevents: using +// os.Environ() directly does expose the credential. If this ever stops +// leaking, the scrubbing above is no longer load-bearing and the test should +// be revisited rather than deleted. +func TestUnscrubbedEnvironWouldLeak(t *testing.T) { + t.Setenv(EnvBridgePassword, "bridge-secret-value") + + cmd := exec.Command("sh", "-c", `printf '%s' "$`+EnvBridgePassword+`"`) + cmd.Env = os.Environ() + + out, err := cmd.Output() + if err != nil { + t.Fatalf("child failed: %v", err) + } + if string(out) != "bridge-secret-value" { + t.Skipf("environment did not propagate as expected (got %q); "+ + "the scrubbed-child test above is the load-bearing assertion", out) + } +}