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

New Issue

clawbot · 2026-08-09T03:38:13+02:00

clawbot commented

2026-08-09 03:38:13 +02:00

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 referenced this issue

2026-08-09 03:40:49 +02:00

No confirmation prompt on any destructive operation #39

clawbot referenced this issue

2026-08-09 04:24:49 +02:00

Update golangci-lint to v2.12.2 with canonical config #29

clawbot commented

2026-08-10 16:14:54 +02:00

Implementation plan:

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.
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.
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.
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).

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/secret#33

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

Threat

Definition of done

Implementation requirements

Priority

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