From 0f5d884eb235e60281e18c9ed2e90f9ae883d5be 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, the version commands, encrypt and decrypt built paths from the name without checking it; import checked it only after reading the source file. vault.ValidateSecretName wraps the existing name rule and its error states the rule. Each of those commands calls it on the name as given, before building any path; a move checks both names before switching the current vault. AddSecret, GetSecretVersion and GetSecretObject use it too. The regression test copies two in-memory vaults for each rejected command and requires the exact error and an unchanged state directory. Model: opus-5-5 --- TODO.md | 10 +- internal/cli/crypto.go | 10 ++ internal/cli/path_traversal_test.go | 232 ++++++++++++++++++++++++++++ internal/cli/secrets.go | 37 ++++- internal/cli/version.go | 15 ++ internal/vault/errors.go | 9 +- internal/vault/secrets.go | 38 +++-- 7 files changed, 331 insertions(+), 20 deletions(-) create mode 100644 internal/cli/path_traversal_test.go diff --git a/TODO.md b/TODO.md index 281cbac..81a0118 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,13 @@ 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, before switching the current vault), `import`, + `version list`/`promote`/`rm`, `encrypt` and `decrypt`. The error + states the naming rule. 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 +103,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..a8d36ae --- /dev/null +++ b/internal/cli/path_traversal_test.go @@ -0,0 +1,232 @@ +package cli_test + +import ( + "maps" + "os" + "slices" + "strings" + "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 +} + +// newFsFromSnapshot returns a new in-memory filesystem holding exactly the +// directories and files recorded by snapshotStateDir. +// +//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout +func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs { + t.Helper() + + fs := afero.NewMemMapFs() + + // In sorted order every directory comes before its contents. + for _, path := range slices.Sorted(maps.Keys(tree)) { + dir, isDir := strings.CutSuffix(path, "/") + if isDir { + require.NoError(t, fs.MkdirAll(dir, secret.DirPerms)) + + continue + } + + err := afero.WriteFile(fs, path, []byte(tree[path]), secret.FilePerms) + require.NoError(t, err) + } + + return fs +} + +// requireRejectedAndUnchanged runs a command on a copy of the state +// directory recorded in before. It requires exactly the error +// vault.ValidateSecretName gives for the rejected name, so that a later +// check rejecting the name does not count, and everything under the state +// directory as it was: the error alone proves nothing, since it could come +// after the vault had already been deleted. +func requireRejectedAndUnchanged( + t *testing.T, before map[string]string, rejected string, + run func(c *cli.Instance) error, +) { + t.Helper() + + fs := newFsFromSnapshot(t, before) + + err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir)) + + require.Equal(t, before, snapshotStateDir(t, fs)) + require.ErrorIs(t, err, vault.ErrInvalidSecretName) + require.EqualError(t, err, vault.ValidateSecretName(rejected).Error()) +} + +// 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 // newTwoVaultFs uses t.Setenv +func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) { + // Creating a passphrase unlocker is slow by design, so the vaults are + // created once and each case runs on its own copy of them. + before := snapshotStateDir(t, newTwoVaultFs(t)) + + vaultDir := testStateDir + "/vaults.d/default" + require.Contains(t, before, vaultDir+"/secrets.d/x/") + require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/") + require.Equal(t, "default", before[testStateDir+"/currentvault"]) + + cmd := &cobra.Command{} + + tests := []struct { + command string + rejected string // the secret name the command must reject + 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", "../../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) + }}, + // "work" is not the current vault: a move within it must not + // select it when a name is rejected. + {"mv --force work:.. work:x", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "work:..", "work:x", true) + }}, + {"mv --force work:x work:..", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "work:x", "work:..", true) + }}, + {"mv --force default:.. work", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "default:..", "work", true) + }}, + {"mv --force default:.. work:y", "..", func(c *cli.Instance) error { + return c.MoveSecret(cmd, "default:..", "work:y", 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", "../../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, before, tt.rejected, tt.run) + }) + } +} diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index f62ecf0..6b42418 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 { @@ -737,10 +747,22 @@ func (cli *Instance) MoveSecret( destSecretName = srcSecretName } + // Check both names before selecting a vault below, so that a rejected + // move leaves the current vault as it was. + err := vault.ValidateSecretName(srcSecretName) + if err != nil { + return err + } + + err = vault.ValidateSecretName(destSecretName) + if err != nil { + return err + } + // Same vault? Use simple rename if possible (optimization) if srcVaultName == destVaultName { // Select the vault and do a simple move - err := vault.SelectVault(cli.fs, cli.stateDir, srcVaultName) + err = vault.SelectVault(cli.fs, cli.stateDir, srcVaultName) if err != nil { return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err) } @@ -757,6 +779,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 @@ -808,7 +840,8 @@ func (cli *Instance) moveSecretWithinVault( return nil } -// moveSecretCrossVault handles moving between different vaults +// moveSecretCrossVault handles moving between different vaults. Its caller, +// MoveSecret, has already checked both secret names. func (cli *Instance) moveSecretCrossVault( cmd *cobra.Command, srcVaultName, srcSecretName, 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/errors.go b/internal/vault/errors.go index 7d51e2e..ae90800 100644 --- a/internal/vault/errors.go +++ b/internal/vault/errors.go @@ -29,10 +29,11 @@ var ( // ErrNilValueBuffer indicates a nil value buffer was supplied. ErrNilValueBuffer = errors.New("value buffer is nil") - // ErrInvalidSecretName indicates a secret name that does not match - // the allowed pattern [a-z0-9.\-_/]+. Composed as - // "invalid secret name '': must match pattern [a-z0-9.\-_/]+", - // or as "invalid secret name: " by GetSecretObject. + // ErrInvalidSecretName indicates a secret name that breaks the naming + // rule: only letters, digits, '.', '-', '_' and '/'; not empty; no + // leading '.' or '/', no trailing '/', no '//', no '..' path segment. + // Composed by ValidateSecretName as + // "invalid secret name '': ". ErrInvalidSecretName = errors.New("invalid secret name") // ErrSecretExists indicates the secret already exists and --force diff --git a/internal/vault/secrets.go b/internal/vault/secrets.go index b8ab559..55ed5ae 100644 --- a/internal/vault/secrets.go +++ b/internal/vault/secrets.go @@ -79,6 +79,7 @@ func (v *Vault) ListSecrets() ([]string, error) { // - No leading or trailing slashes // - No double slashes // - No names starting with dots +// - No ".." path segments func isValidSecretName(name string) bool { if name == "" { return false @@ -110,6 +111,22 @@ 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': only letters, digits, '.', '-', '_' and '/' are allowed, "+ + "and a name must not be empty, start with '.' or '/', end with '/', "+ + "contain '//', or have '..' as a path segment", + 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 +141,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) @@ -358,8 +373,9 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) { // GetSecretObject retrieves a Secret object with metadata loaded from this vault func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) { - if !isValidSecretName(name) { - return nil, fmt.Errorf("%w: %s", ErrInvalidSecretName, name) + err := ValidateSecretName(name) + if err != nil { + return nil, err } // First check if the secret exists by checking for the metadata file @@ -628,13 +644,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