diff --git a/runtimevar/hashivault/hashivault.go b/runtimevar/hashivault/hashivault.go index d1539f9574..9048ee3b2d 100644 --- a/runtimevar/hashivault/hashivault.go +++ b/runtimevar/hashivault/hashivault.go @@ -42,6 +42,7 @@ import ( "os" "path" "strconv" + "strings" "sync" "time" @@ -271,11 +272,15 @@ func newWatcher(client *api.Client, secretPath string, decoder *runtimevar.Decod mount = "secret" } - var fullPath string + var prefix string if engineVersion == 2 { - fullPath = path.Join(mount, "data", secretPath) + prefix = path.Join(mount, "data") } else { - fullPath = path.Join(mount, secretPath) + prefix = mount + } + fullPath, err := vaultPath(prefix, secretPath) + if err != nil { + return nil, err } return &watcher{ @@ -288,6 +293,20 @@ func newWatcher(client *api.Client, secretPath string, decoder *runtimevar.Decod }, nil } +// vaultPath joins prefix and secretPath into a Vault API path, and verifies +// that the result is actually inside prefix. path.Join cleans ".." segments, +// so a secretPath such as "../../sys/health" would otherwise silently make +// the request escape the intended mount -- something Vault's own +// path-prefix-scoped ACL policies can't defend against, since as far as +// Vault is concerned it's simply the path the client asked for. +func vaultPath(prefix, secretPath string) (string, error) { + p := path.Join(prefix, secretPath) + if p != prefix && !strings.HasPrefix(p, prefix+"/") { + return "", fmt.Errorf("hashivault: secret path %q escapes the %q mount", secretPath, prefix) + } + return p, nil +} + type state struct { val any raw *api.Secret diff --git a/runtimevar/hashivault/hashivault_test.go b/runtimevar/hashivault/hashivault_test.go index 077b14536b..849ba85f3e 100644 --- a/runtimevar/hashivault/hashivault_test.go +++ b/runtimevar/hashivault/hashivault_test.go @@ -262,6 +262,50 @@ func TestEngineVersionPaths(t *testing.T) { } } +func TestSecretPathEscape(t *testing.T) { + ctx := context.Background() + client, err := Dial(ctx, &Config{ + Token: "fake", + APIConfig: api.Config{ + Address: "http://localhost:8200", + }, + }) + if err != nil { + t.Fatal(err) + } + + tests := []struct { + SecretPath string + WantPath string // empty means an error is expected + }{ + {"tenant-a/config", "secret/data/tenant-a/config"}, + {"tenant-a/../tenant-b/config", "secret/data/tenant-b/config"}, // stays within the mount, allowed + {"../../sys/health", ""}, + {"a/../../../auth/token/create", ""}, + {"../secret/data/other-tenant", ""}, + } + + for _, test := range tests { + w, err := newWatcher(client, test.SecretPath, runtimevar.StringDecoder, nil) + if test.WantPath == "" { + if err == nil { + t.Errorf("SecretPath=%q: expected an error rejecting the escape, got none (path=%q)", + test.SecretPath, w.(*watcher).path) + } + continue + } + if err != nil { + t.Errorf("SecretPath=%q: newWatcher failed: %v", test.SecretPath, err) + continue + } + watcher := w.(*watcher) + if watcher.path != test.WantPath { + t.Errorf("SecretPath=%q: got path %q, want %q", test.SecretPath, watcher.path, test.WantPath) + } + w.Close() + } +} + func TestInvalidEngineVersion(t *testing.T) { ctx := context.Background() client, err := Dial(ctx, &Config{