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