sneak/secret
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Critical: secret rm .. deletes the entire vault; rm/mv/import skip name validation #33

New Issue
Closed
opened 2026-08-09 03:38:13 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-09 03:38:13 +02:00
Collaborator
Copy Link
Copy Source

Found during the 1.0 security survey. This was not on the old TODO list — it is strictly worse than the "dots in secret names risk path traversal" item that was, because it is an unvalidated destructive path rather than a read.

Threat

internal/vault/secrets.go:82-111 defines isValidSecretName, and it is sound: it rejects "", leading ., .. components, leading and trailing /, //, and anything outside ^[a-zA-Z0-9\.\-\_\/]+$. AddSecret (:127), GetSecretObject (:358) and resolveSecretVersion (:625) all call it.

The destructive commands do not.

internal/cli/secrets.go:651-689, RemoveSecret:

encodedName := strings.ReplaceAll(secretName, "/", "%")
secretDir := filepath.Join(vaultDir, "secrets.d", encodedName)
...
err = cli.fs.RemoveAll(secretDir)

The / to % encoding is what normally collapses a name into a single path component and makes traversal structurally impossible. A name of .. contains no /, so it passes through the encoding unchanged, and filepath.Join then cleans the result:

  • secret rm .. produces filepath.Join(vaultDir, "secrets.d", "..") which cleans to vaultDir, so RemoveAll deletes the entire vault — every secret, every version, every unlocker, the vault metadata, and longterm.age.
  • secret rm . cleans to secrets.d, deleting every secret in the vault while leaving the unlockers.

cobra.ExactArgs(1) at internal/cli/secrets.go:192 does not filter either value. There is no confirmation prompt and no --force requirement on secret rm (see the separate destructive-confirmation issue), so a single argument destroys the vault with no second chance and no backup.

The same unvalidated pattern is in moveSecretWithinVault (internal/cli/secrets.go:770-771, :782-783), which does RemoveAll(destDir) at :795 and Rename at :801 — so secret mv .. anything renames the vault directory out from under itself. moveSecretCrossVault (:812) and the file-import path have the same gap.

Realistic triggers, none of which require a malicious actor:

  • A shell mishap where a variable is unset: secret rm "$PREFIX/.." with PREFIX empty.
  • Any script interpolating a name from data into secret rm.
  • A plain typo, given .. is two adjacent keys.

Because unlockers and longterm.age are deleted along with the secrets, recovery after secret rm .. requires the BIP39 mnemonic. A user who never wrote the mnemonic down — which the tool permits, since passphrase unlockers work without it — has permanently lost every secret in that vault.

Definition of done

  • isValidSecretName (or an exported equivalent) is called at the top of every command path that resolves a user-supplied name to a filesystem path, before any path construction: RemoveSecret, MoveSecret / moveSecretWithinVault / moveSecretCrossVault (both source and destination), and ImportSecret.
  • The validator is applied in exactly one place per entry point, and rejects before any RemoveAll, Rename, or MkdirAll is reached.
  • secret rm .., secret rm ., secret rm ../../etc, secret mv .. x, secret mv x .., and the import equivalents all fail with a clear validation error and a non-zero exit, leaving the vault byte-for-byte unchanged.
  • Regression tests prove the vault is untouched afterward — not merely that the command returned an error. Assert that the vault directory, its secrets.d entries, and unlockers.d all still exist with the same contents. A test that only checks the error message would pass even if the deletion had already happened.
  • Tests cover both RemoveSecret and both move paths, and live alongside the existing traversal tests in internal/vault/path_traversal_test.go / internal/vault/secrets_name_test.go or a CLI-level equivalent.
  • make check green. TODO.md updated in the same commit.

Implementation requirements

  • Do not duplicate the validation regex. isValidSecretName is currently unexported in internal/vault; export it or add a thin exported wrapper and call that from internal/cli. Two copies of this rule will drift, and the drift will be silent.
  • Validate the raw user input, before the / to % encoding, matching how AddSecret already does it. Validating post-encoding would accept .. since the encoding does not alter it.
  • For the move paths, validate both operands. A valid source with a .. destination is just as destructive.
  • Do not "fix" this by calling filepath.Clean and checking the prefix. Prefix checks on cleaned paths are a well-known source of bypasses, and the allowlist validator already exists and is correct — use it.
  • Do not add a confirmation prompt as the fix. Prompting is tracked separately and is defense in depth; a name that can escape its directory must be rejected outright regardless of any prompt.

Priority

Top of the 1.0.0 milestone. Unbounded, unrecoverable data loss in a tool whose entire purpose is not losing this data.

Found during the 1.0 security survey. **This was not on the old TODO list** — it is strictly worse than the "dots in secret names risk path traversal" item that was, because it is an unvalidated *destructive* path rather than a read. ## Threat `internal/vault/secrets.go:82-111` defines `isValidSecretName`, and it is sound: it rejects `""`, leading `.`, `..` components, leading and trailing `/`, `//`, and anything outside `^[a-zA-Z0-9\.\-\_\/]+$`. `AddSecret` (`:127`), `GetSecretObject` (`:358`) and `resolveSecretVersion` (`:625`) all call it. The destructive commands do not. `internal/cli/secrets.go:651-689`, `RemoveSecret`: ```go encodedName := strings.ReplaceAll(secretName, "/", "%") secretDir := filepath.Join(vaultDir, "secrets.d", encodedName) ... err = cli.fs.RemoveAll(secretDir) ``` The `/` to `%` encoding is what normally collapses a name into a single path component and makes traversal structurally impossible. A name of `..` contains no `/`, so it passes through the encoding unchanged, and `filepath.Join` then *cleans* the result: - `secret rm ..` produces `filepath.Join(vaultDir, "secrets.d", "..")` which cleans to **`vaultDir`**, so `RemoveAll` deletes the **entire vault** — every secret, every version, every unlocker, the vault metadata, and `longterm.age`. - `secret rm .` cleans to `secrets.d`, deleting **every secret in the vault** while leaving the unlockers. `cobra.ExactArgs(1)` at `internal/cli/secrets.go:192` does not filter either value. There is no confirmation prompt and no `--force` requirement on `secret rm` (see the separate destructive-confirmation issue), so a single argument destroys the vault with no second chance and no backup. The same unvalidated pattern is in `moveSecretWithinVault` (`internal/cli/secrets.go:770-771`, `:782-783`), which does `RemoveAll(destDir)` at `:795` and `Rename` at `:801` — so `secret mv .. anything` renames the vault directory out from under itself. `moveSecretCrossVault` (`:812`) and the file-import path have the same gap. Realistic triggers, none of which require a malicious actor: - A shell mishap where a variable is unset: `secret rm "$PREFIX/.."` with `PREFIX` empty. - Any script interpolating a name from data into `secret rm`. - A plain typo, given `..` is two adjacent keys. Because unlockers and `longterm.age` are deleted along with the secrets, recovery after `secret rm ..` requires the BIP39 mnemonic. A user who never wrote the mnemonic down — which the tool permits, since passphrase unlockers work without it — has permanently lost every secret in that vault. ## Definition of done - `isValidSecretName` (or an exported equivalent) is called at the top of **every** command path that resolves a user-supplied name to a filesystem path, before any path construction: `RemoveSecret`, `MoveSecret` / `moveSecretWithinVault` / `moveSecretCrossVault` (both source and destination), and `ImportSecret`. - The validator is applied in exactly one place per entry point, and rejects before any `RemoveAll`, `Rename`, or `MkdirAll` is reached. - `secret rm ..`, `secret rm .`, `secret rm ../../etc`, `secret mv .. x`, `secret mv x ..`, and the import equivalents all fail with a clear validation error and a non-zero exit, leaving the vault byte-for-byte unchanged. - Regression tests prove the vault is untouched afterward — not merely that the command returned an error. Assert that the vault directory, its `secrets.d` entries, and `unlockers.d` all still exist with the same contents. A test that only checks the error message would pass even if the deletion had already happened. - Tests cover both `RemoveSecret` and both move paths, and live alongside the existing traversal tests in `internal/vault/path_traversal_test.go` / `internal/vault/secrets_name_test.go` or a CLI-level equivalent. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - Do not duplicate the validation regex. `isValidSecretName` is currently unexported in `internal/vault`; export it or add a thin exported wrapper and call that from `internal/cli`. Two copies of this rule will drift, and the drift will be silent. - Validate the **raw user input**, before the `/` to `%` encoding, matching how `AddSecret` already does it. Validating post-encoding would accept `..` since the encoding does not alter it. - For the move paths, validate **both** operands. A valid source with a `..` destination is just as destructive. - Do not "fix" this by calling `filepath.Clean` and checking the prefix. Prefix checks on cleaned paths are a well-known source of bypasses, and the allowlist validator already exists and is correct — use it. - Do not add a confirmation prompt as the fix. Prompting is tracked separately and is defense in depth; a name that can escape its directory must be rejected outright regardless of any prompt. ## Priority Top of the 1.0.0 milestone. Unbounded, unrecoverable data loss in a tool whose entire purpose is not losing this data.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:38:13 +02:00
clawbot commented 2026-08-10 16:14:54 +02:00
Author
Collaborator
Copy Link
Copy Source

Implementation plan:

  1. internal/vault/secrets.go: add exported ValidateSecretName(name string) error, a thin wrapper over the existing unexported isValidSecretName, returning the wrapped ErrInvalidSecretName with the message AddSecret already composes. Route AddSecret and resolveSecretVersion through it so the rule has exactly one implementation and no second regex.

  2. internal/cli/secrets.go: call vault.ValidateSecretName on the raw argument, before the /->% encoding and before any filepath.Join, at the top of:

    • RemoveSecret (before DirExists/RemoveAll)
    • ImportSecret (before the source file is even read)
    • moveSecretWithinVault — both source and dest
    • moveSecretCrossVault — both srcSecretName and destSecretName, after MoveSecret has resolved the defaulted destination name

    One call site per entry point. No filepath.Clean/prefix checking, no confirmation prompt as a substitute.

  3. Regression tests, CLI-level (the gap is in internal/cli), on an in-memory filesystem: snapshot the whole vault tree (paths + file contents, including secrets.d and unlockers.d) before the call, assert the command errors with ErrInvalidSecretName, then assert the snapshot is unchanged byte-for-byte. Cases: rm .., rm ., rm ../../etc, mv .. x, mv x .., and the import equivalent.

  4. TODO.md updated in the same commit. Gate on make check plus script/cibuild (the containerized build, since the host memlock limit aborts the memguard test locally).

Implementation plan: 1. `internal/vault/secrets.go`: add exported `ValidateSecretName(name string) error`, a thin wrapper over the existing unexported `isValidSecretName`, returning the wrapped `ErrInvalidSecretName` with the message `AddSecret` already composes. Route `AddSecret` and `resolveSecretVersion` through it so the rule has exactly one implementation and no second regex. 2. `internal/cli/secrets.go`: call `vault.ValidateSecretName` on the **raw** argument, before the `/`->`%` encoding and before any `filepath.Join`, at the top of: - `RemoveSecret` (before `DirExists`/`RemoveAll`) - `ImportSecret` (before the source file is even read) - `moveSecretWithinVault` — both `source` and `dest` - `moveSecretCrossVault` — both `srcSecretName` and `destSecretName`, after `MoveSecret` has resolved the defaulted destination name One call site per entry point. No `filepath.Clean`/prefix checking, no confirmation prompt as a substitute. 3. Regression tests, CLI-level (the gap is in `internal/cli`), on an in-memory filesystem: snapshot the whole vault tree (paths + file contents, including `secrets.d` and `unlockers.d`) before the call, assert the command errors with `ErrInvalidSecretName`, then assert the snapshot is unchanged byte-for-byte. Cases: `rm ..`, `rm .`, `rm ../../etc`, `mv .. x`, `mv x ..`, and the import equivalent. 4. `TODO.md` updated in the same commit. Gate on `make check` plus `script/cibuild` (the containerized build, since the host `memlock` limit aborts the memguard test locally).
clawbot added the critical label 2026-10-03 14:20:40 +02:00
clawbot commented 2026-10-03 14:20:41 +02:00
Author
Collaborator
Copy Link
Copy Source

Labelled critical: secret rm .. deletes the whole vault (and secret rm . every secret in it) in one command with no prompt, leaving the user only the mnemonic to recover from.

Model: opus-5-5

Labelled critical: `secret rm ..` deletes the whole vault (and `secret rm .` every secret in it) in one command with no prompt, leaving the user only the mnemonic to recover from. Model: opus-5-5
clawbot commented 2026-10-03 14:34:24 +02:00
Author
Collaborator
Copy Link
Copy Source

Fixed in #65. rm, mv (both names, within a vault and between vaults) and import now check the secret name before building any path. So do version list/promote/rm, encrypt and decrypt, which had the same gap.

The audit found two larger gaps, not fixed in that PR:

  1. The version argument goes into the path unchecked in secret version rm and secret version promote, and in secret get --version, where it is only read. The "cannot remove the current version" check does not stop it, because the argument never equals the current version's name. With an existing secret x:

    • secret version rm x "" or secret version rm x . deletes every version of x, the current one included.
    • secret version rm x .. deletes x.
    • secret version rm x ../../.. deletes the whole vault. Each further .. climbs one level: all vaults, then the state directory, then (on Linux, with the default state directory) ~/.config and the home directory.

    This is as severe as this issue, and an empty shell variable is enough to trigger it. No rule for version names exists to reuse. One plain fix: accept only a version that is listed in the secret's versions directory.

  2. Vault names. The vault name check accepts . and ... Two commands skip it entirely: secret vault import, and the vault part of a move between vaults (secret mv vault:name ...). With such names:

    • vault import writes a public key, metadata and an unlocker into the state directory or any existing directory.
    • A move can copy a secret into any other location that holds a vault.
    • vault create . puts a vault inside vaults.d, whose secrets.d and unlockers.d then show up as vaults.

    No data is lost on these paths: a move deletes its source only after copying it, and replaces an existing secret only with --force.

Model: opus-5-5

Fixed in https://git.eeqj.de/sneak/secret/pulls/65. `rm`, `mv` (both names, within a vault and between vaults) and `import` now check the secret name before building any path. So do `version list`/`promote`/`rm`, `encrypt` and `decrypt`, which had the same gap. The audit found two larger gaps, not fixed in that PR: 1. **The version argument goes into the path unchecked** in `secret version rm` and `secret version promote`, and in `secret get --version`, where it is only read. The "cannot remove the current version" check does not stop it, because the argument never equals the current version's name. With an existing secret `x`: - `secret version rm x ""` or `secret version rm x .` deletes every version of `x`, the current one included. - `secret version rm x ..` deletes `x`. - `secret version rm x ../../..` deletes the whole vault. Each further `..` climbs one level: all vaults, then the state directory, then (on Linux, with the default state directory) `~/.config` and the home directory. This is as severe as this issue, and an empty shell variable is enough to trigger it. No rule for version names exists to reuse. One plain fix: accept only a version that is listed in the secret's `versions` directory. 2. **Vault names.** The vault name check accepts `.` and `..`. Two commands skip it entirely: `secret vault import`, and the vault part of a move between vaults (`secret mv vault:name ...`). With such names: - `vault import` writes a public key, metadata and an unlocker into the state directory or any existing directory. - A move can copy a secret into any other location that holds a vault. - `vault create .` puts a vault inside `vaults.d`, whose `secrets.d` and `unlockers.d` then show up as vaults. No data is lost on these paths: a move deletes its source only after copying it, and replaces an existing secret only with `--force`. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels critical
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/secret#33