From 526a6be17bd83c0d494f3dddff832398e7b10388 Mon Sep 17 00:00:00 2001 From: sneak Date: Sat, 3 Oct 2026 12:31:35 +0000 Subject: [PATCH] Reject invalid secret names before any command builds a path (closes #33) `secret rm ..` resolved to the vault directory and deleted the whole vault; `secret rm .` and `secret rm ""` deleted every secret. rm, mv and import built paths from the name without checking it, and so did the version commands, encrypt and decrypt. vault.ValidateSecretName wraps the existing name rule and returns ErrInvalidSecretName. Each of those commands calls it on the name as given, both names for a move, before building any path. AddSecret and GetSecretVersion use it too, so the rule has one implementation. The regression test snapshots every file under the state directory of two in-memory vaults and requires it unchanged after each rejected command. Model: opus-5-5 --- TODO.md | 9 +- internal/cli/crypto.go | 10 ++ internal/cli/path_traversal_test.go | 182 ++++++++++++++++++++++++++++ internal/cli/secrets.go | 30 +++++ internal/cli/version.go | 15 +++ internal/vault/secrets.go | 30 +++-- 6 files changed, 264 insertions(+), 12 deletions(-) create mode 100644 internal/cli/path_traversal_test.go diff --git a/TODO.md b/TODO.md index 281cbac..470901c 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,12 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-03: Every command that builds a path from a secret name + checks the name first with `vault.ValidateSecretName` and touches + nothing when it is invalid: `rm`, `mv` (both names, within a vault + and between vaults), `import`, `version list`/`promote`/`rm`, + `encrypt` and `decrypt`. Before, `secret rm ..` deleted the whole + vault and `secret rm .` every secret in it. - 2026-10-02: A plain `docker build .` builds again: the size tests skip a case that needs more locked memory than the process can lock, and run every case under `script/cibuild`. The image stamps the @@ -96,8 +102,7 @@ Bring the repo into policy compliance in one commit: buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase. - Race conditions: no file locking in vault/secrets.go:142-176; non-atomic writes can leave the vault inconsistent. - - Input validation: dots in secret names risk path traversal - (vault/secrets.go:75-99); no maximum secret size (DoS). + - Input validation: no maximum secret size (DoS). - Timing attacks: bytes.Equal passphrase compare (cli/init.go: 209-216); non-constant-time public key compare (vault.go:95-100). - High priority: diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index 723b472..5a89414 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -122,6 +122,11 @@ func (cli *Instance) resolveEncryptionKey( // Encrypt encrypts data using an age secret key stored in a secret func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -191,6 +196,11 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { // Decrypt decrypts data using an age secret key stored in a secret func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { diff --git a/internal/cli/path_traversal_test.go b/internal/cli/path_traversal_test.go new file mode 100644 index 0000000..0a1eb03 --- /dev/null +++ b/internal/cli/path_traversal_test.go @@ -0,0 +1,182 @@ +package cli_test + +import ( + "os" + "testing" + + "git.eeqj.de/sneak/secret/internal/cli" + "git.eeqj.de/sneak/secret/internal/secret" + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/awnumar/memguard" + "github.com/spf13/afero" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +const ( + // testStateDir is the in-memory state directory of the test vaults. + testStateDir = "/test/state" + + // testPassphrase protects the passphrase unlocker of each test vault. + testPassphrase = "test-passphrase" + + // testVersion is a version name in the format the vault uses. + testVersion = "20260101.001" + + // missingFile is an import source that does not exist, so an import + // that opened it before checking the name would fail with another error. + missingFile = "/no/such/file" +) + +// newTwoVaultFs returns an in-memory filesystem holding the vaults "work" +// and "default", the current one. Each holds the secret "x" and a +// passphrase unlocker, so both secrets.d and unlockers.d have contents. +// +//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout +func newTwoVaultFs(t *testing.T) afero.Fs { + t.Helper() + + t.Setenv(secret.EnvMnemonic, testMnemonic) + + fs := afero.NewMemMapFs() + + for _, name := range []string{"work", "default"} { + vlt, err := vault.CreateVault(fs, testStateDir, name) + require.NoError(t, err) + + err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false) + require.NoError(t, err) + + _, err = vlt.CreatePassphraseUnlocker( + memguard.NewBufferFromBytes([]byte(testPassphrase))) + require.NoError(t, err) + } + + return fs +} + +// snapshotStateDir maps every file under the state directory to its +// contents, and every directory, written with a trailing "/", to "". Two +// snapshots are equal only if nothing in it was added, removed or changed. +func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string { + t.Helper() + + tree := map[string]string{} + + err := afero.Walk(fs, testStateDir, func( + path string, info os.FileInfo, err error, + ) error { + if err != nil { + return err + } + + if info.IsDir() { + tree[path+"/"] = "" + + return nil + } + + content, err := afero.ReadFile(fs, path) + if err != nil { + return err + } + + tree[path] = string(content) + + return nil + }) + require.NoError(t, err) + + return tree +} + +// requireRejectedAndUnchanged runs a command against the two test vaults +// and requires that it fails with vault.ErrInvalidSecretName and leaves +// everything under the state directory as it was. The error alone proves +// nothing: it could be returned after the vault had already been deleted. +func requireRejectedAndUnchanged(t *testing.T, run func(c *cli.Instance) error) { + t.Helper() + + fs := newTwoVaultFs(t) + before := snapshotStateDir(t, fs) + + vaultDir := testStateDir + "/vaults.d/default" + require.Contains(t, before, vaultDir+"/secrets.d/x/") + require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/") + + err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir)) + + require.Equal(t, before, snapshotStateDir(t, fs)) + require.ErrorIs(t, err, vault.ErrInvalidSecretName) +} + +// TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for +// https://git.eeqj.de/sneak/secret/issues/33, where `secret rm ..` deleted +// the whole vault, and `secret rm .` or `secret rm ""` every secret in it. +// Moves and imports use --force, so that only the name check stands in +// the way. +// +//nolint:paralleltest // subtests use t.Setenv via newTwoVaultFs +func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) { + cmd := &cobra.Command{} + + tests := []struct { + command string + run func(c *cli.Instance) error + }{ + {"rm ..", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, "..", false) + }}, + {"rm .", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, ".", false) + }}, + {`rm ""`, func(c *cli.Instance) error { + return c.RemoveSecret(cmd, "", false) + }}, + {"rm ../../etc", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, "../../etc", false) + }}, + {"mv --force .. x", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "..", "x", true) + }}, + {"mv --force x ..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "x", "..", true) + }}, + {"mv --force default:.. work", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "default:..", "work", true) + }}, + {"mv --force default:x work:..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "default:x", "work:..", true) + }}, + {"import --force ..", func(c *cli.Instance) error { + return c.ImportSecret(cmd, "..", missingFile, true) + }}, + {"import --force .", func(c *cli.Instance) error { + return c.ImportSecret(cmd, ".", missingFile, true) + }}, + {"import --force ../../etc", func(c *cli.Instance) error { + return c.ImportSecret(cmd, "../../etc", missingFile, true) + }}, + {"version list ..", func(c *cli.Instance) error { + return c.ListVersions(cmd, "..") + }}, + {"version promote ..", func(c *cli.Instance) error { + return c.PromoteVersion(cmd, "..", testVersion) + }}, + {"version rm ..", func(c *cli.Instance) error { + return c.RemoveVersion(cmd, "..", testVersion) + }}, + {"encrypt ..", func(c *cli.Instance) error { + return c.Encrypt("..", "", "") + }}, + {"decrypt ..", func(c *cli.Instance) error { + return c.Decrypt("..", "", "") + }}, + } + + for _, tt := range tests { + t.Run(tt.command, func(t *testing.T) { + requireRejectedAndUnchanged(t, tt.run) + }) + } +} diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index f62ecf0..3124e69 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -603,6 +603,11 @@ func printSecretsTable( func (cli *Instance) ImportSecret( cmd *cobra.Command, secretName, sourceFile string, force bool, ) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -649,6 +654,11 @@ func (cli *Instance) ImportSecret( // RemoveSecret removes a secret from the vault func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -757,6 +767,16 @@ func (cli *Instance) MoveSecret( func (cli *Instance) moveSecretWithinVault( cmd *cobra.Command, source, dest string, force bool, ) error { + err := vault.ValidateSecretName(source) + if err != nil { + return err + } + + err = vault.ValidateSecretName(dest) + if err != nil { + return err + } + currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { return err @@ -815,6 +835,16 @@ func (cli *Instance) moveSecretCrossVault( destVaultName, destSecretName string, force bool, ) error { + err := vault.ValidateSecretName(srcSecretName) + if err != nil { + return err + } + + err = vault.ValidateSecretName(destSecretName) + if err != nil { + return err + } + // Get source vault srcVault := vault.NewVault(cli.fs, cli.stateDir, srcVaultName) diff --git a/internal/cli/version.go b/internal/cli/version.go index 17f113f..feb04d9 100644 --- a/internal/cli/version.go +++ b/internal/cli/version.go @@ -112,6 +112,11 @@ func VersionCommands(cli *Instance) *cobra.Command { func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error { secret.Debug("ListVersions called", "secret_name", secretName) + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -239,6 +244,11 @@ func formatVersionTime(t *time.Time) string { func (cli *Instance) PromoteVersion( cmd *cobra.Command, secretName string, version string, ) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { @@ -282,6 +292,11 @@ func (cli *Instance) PromoteVersion( func (cli *Instance) RemoveVersion( cmd *cobra.Command, secretName string, version string, ) error { + err := vault.ValidateSecretName(secretName) + if err != nil { + return err + } + // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { diff --git a/internal/vault/secrets.go b/internal/vault/secrets.go index b8ab559..8c219c0 100644 --- a/internal/vault/secrets.go +++ b/internal/vault/secrets.go @@ -110,6 +110,20 @@ func isValidSecretName(name string) bool { return matched } +// ValidateSecretName returns an error wrapping ErrInvalidSecretName when +// name is not a valid secret name. Call it on the name exactly as the user +// gave it, before building any path from it. +func ValidateSecretName(name string) error { + if !isValidSecretName(name) { + return fmt.Errorf( + "%w '%s': must match pattern [a-z0-9.\\-_/]+", + ErrInvalidSecretName, name, + ) + } + + return nil +} + // AddSecret adds a secret to this vault func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) error { if value == nil { @@ -124,13 +138,11 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) ) // Validate secret name - if !isValidSecretName(name) { + err := ValidateSecretName(name) + if err != nil { secret.Debug("Invalid secret name provided", "secret_name", name) - return fmt.Errorf( - "%w '%s': must match pattern [a-z0-9.\\-_/]+", - ErrInvalidSecretName, name, - ) + return err } secret.Debug("Secret name validation passed", "secret_name", name) @@ -628,13 +640,11 @@ func (v *Vault) updatePreviousVersion( // version exist, and resolves an empty version to the current one. func (v *Vault) resolveSecretVersion(name, version string) (string, error) { // Validate secret name to prevent path traversal - if !isValidSecretName(name) { + err := ValidateSecretName(name) + if err != nil { secret.Debug("Invalid secret name provided", "secret_name", name) - return "", fmt.Errorf( - "%w '%s': must match pattern [a-z0-9.\\-_/]+", - ErrInvalidSecretName, name, - ) + return "", err } // Get vault directory