diff --git a/README.md b/README.md index d64a5de..83852c0 100644 --- a/README.md +++ b/README.md @@ -113,7 +113,9 @@ automatically switch to another vault if removing the current one. Adds a secret to the current vault. Reads the secret value from stdin. - `--force, -f`: Overwrite existing secret -**Secret Name Format:** `[a-z0-9\.\-\_\/]+` +**Secret Name Format:** 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. - Forward slashes (`/`) are converted to percent signs (`%`) for storage - Examples: `database/password`, `api.key`, `ssh_private_key` diff --git a/TODO.md b/TODO.md index 281cbac..5bc89cd 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 + and `README.md` state 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..7fbf6f0 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 { @@ -702,13 +712,8 @@ func (cli *Instance) MoveSecret( srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source) destVaultName, destSecretName, destQualified := ParseVaultSecretRef(dest) - // If neither is qualified, this is a simple within-vault rename - if !srcQualified && !destQualified { - return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force) - } - // Cross-vault move requires source to be qualified - if !srcQualified { + if !srcQualified && destQualified { return errCrossVaultSourceUnqualified } @@ -716,31 +721,46 @@ func (cli *Instance) MoveSecret( // Format: "work:secret default" means move to vault "default" // Format: "work:secret default:newname" means move to vault "default" // with a new name - if !destQualified { + if srcQualified && !destQualified { // Check if dest is actually a vault name vaults, err := vault.ListVaults(cli.fs, cli.stateDir) if err == nil && slices.Contains(vaults, dest) { // dest is a vault name, use source secret name destVaultName = dest destSecretName = srcSecretName - } - - // If destVaultName is still empty, dest is a secret name in source vault - if destVaultName == "" { + } else { + // dest is a secret name in source vault destVaultName = srcVaultName - destSecretName = dest } } - // If destination secret name is empty, use source secret name - if destSecretName == "" { + // If destination secret name is empty, use source secret name. A plain + // rename keeps it empty, so that the check below rejects it. + if srcQualified && destSecretName == "" { destSecretName = srcSecretName } + // Check both names, for every form of the move, 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 + } + + // If neither is qualified, this is a simple within-vault rename + if !srcQualified && !destQualified { + return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force) + } + // 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) } @@ -753,7 +773,8 @@ func (cli *Instance) MoveSecret( cmd, srcVaultName, srcSecretName, destVaultName, destSecretName, force) } -// moveSecretWithinVault handles rename within the current vault +// moveSecretWithinVault handles rename within the current vault. Its caller, +// MoveSecret, has already checked both secret names. func (cli *Instance) moveSecretWithinVault( cmd *cobra.Command, source, dest string, force bool, ) error { @@ -808,7 +829,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