From b4ed320ffdd2a81cd4b743734584040da5c865ab Mon Sep 17 00:00:00 2001 From: parveshsaini Date: Sat, 11 Jul 2026 20:31:27 +0530 Subject: [PATCH] fix(permissions): match glob wildcards across path separators Signed-off-by: parveshsaini --- pkg/permissions/permissions.go | 84 +++++++++++++++++++++++++---- pkg/permissions/permissions_test.go | 25 +++++++++ 2 files changed, 99 insertions(+), 10 deletions(-) diff --git a/pkg/permissions/permissions.go b/pkg/permissions/permissions.go index 38af9b57c2..139a10aaa9 100644 --- a/pkg/permissions/permissions.go +++ b/pkg/permissions/permissions.go @@ -4,7 +4,7 @@ package permissions import ( "fmt" - "path/filepath" + "regexp" "strings" "github.com/docker/docker-agent/pkg/config/latest" @@ -239,17 +239,18 @@ func argToString(v any) string { } // matchGlob checks if a value matches a glob pattern. -// Supports glob-style patterns using filepath.Match semantics: -// - "*" matches any sequence of characters within a path segment +// Supports glob-style patterns: +// - "*" matches any sequence of characters (including path separators and spaces) // - "?" matches any single character // - "[...]" matches character classes // // Matching is case-insensitive. // -// Note: filepath.Match's "*" stops at path separators, but for argument -// matching we want "*" to match any characters including spaces. -// We handle trailing wildcards specially to support patterns like "sudo*" -// matching "sudo rm -rf /". +// Note: filepath.Match's "*"/"?" stop at path separators ("/", or "\" on +// Windows), but argument values (shell commands, file paths, URLs) routinely +// contain "/", so matching must not depend on it. We therefore translate the +// glob to an anchored regexp instead of using filepath.Match. Trailing +// wildcards keep a plain prefix-match fast path (e.g. "sudo*"). func matchGlob(pattern, value string) bool { // Normalize both to lowercase for case-insensitive matching pattern = strings.ToLower(pattern) @@ -266,7 +267,70 @@ func matchGlob(pattern, value string) bool { } } - // Try glob pattern match (also handles exact matches) - matched, err := filepath.Match(pattern, value) - return err == nil && matched + // Full glob match (also handles exact matches). Unlike filepath.Match, the + // translated "*"/"?" span any character, so patterns match values that + // contain path separators. + re, err := globToRegexp(pattern) + if err != nil { + return false + } + return re.MatchString(value) +} + +// globToRegexp translates a filepath.Match-style glob into an anchored regular +// expression. Unlike filepath.Match, the resulting "*" and "?" match any +// character, including path separators — argument values such as shell commands, +// file paths and URLs routinely contain "/", and permission patterns must match +// them regardless. Character classes ("[...]"), whose bracket syntax glob and +// regexp share, are copied through verbatim; every other character is matched literally. +func globToRegexp(pattern string) (*regexp.Regexp, error) { + var b strings.Builder + b.WriteString(`\A`) + for i := 0; i < len(pattern); { + switch c := pattern[i]; c { + case '*': + b.WriteString(`.*`) + i++ + case '?': + b.WriteByte('.') + i++ + case '[': + if end := classEnd(pattern, i); end > 0 { + b.WriteString(pattern[i : end+1]) + i = end + 1 + continue + } + // Unterminated class: match "[" literally. + b.WriteString(regexp.QuoteMeta("[")) + i++ + default: + b.WriteString(regexp.QuoteMeta(string(c))) + i++ + } + } + b.WriteString(`\z`) + return regexp.Compile(b.String()) +} + +// classEnd returns the index of the "]" closing the character class opened at +// start, or -1 if the class is unterminated. A "]" in the first position +// (optionally after a leading "^" negation) is treated as a literal member, +// matching glob semantics. +func classEnd(pattern string, start int) int { + i := start + 1 + if i < len(pattern) && pattern[i] == '^' { + i++ + } + if i < len(pattern) && pattern[i] == ']' { + i++ + } + for ; i < len(pattern); i++ { + switch pattern[i] { + case '\\': + i++ + case ']': + return i + } + } + return -1 } diff --git a/pkg/permissions/permissions_test.go b/pkg/permissions/permissions_test.go index 2a648e11f9..9e1cfb476b 100644 --- a/pkg/permissions/permissions_test.go +++ b/pkg/permissions/permissions_test.go @@ -544,6 +544,12 @@ func TestMatchGlob(t *testing.T) { {"mcp:*", "mcp:github:create_issue", true}, // * matches everything including : {"mcp:github:*", "mcp:github:create_issue", true}, {"mcp:github:create_*", "mcp:github:create_issue", true}, + + // "*" and "?" span path separators. Argument values (shell commands, + // paths, URLs) routinely contain "/", so wildcards must not stop at it. + {"*rm -rf*", "rm -rf /", true}, + {"*/etc/*", "cat /etc/passwd", true}, + {"a?c", "a/c", true}, } for _, tt := range tests { @@ -555,6 +561,25 @@ func TestMatchGlob(t *testing.T) { } } +// TestDenyGlobCrossesPathSeparator guards against a deny rule failing open. +// +// A deny pattern with a non-trailing wildcard (e.g. "*rm -rf*") must match the +// same command whether or not the argument value contains a path separator. +// Previously it fell through to filepath.Match, whose "*" stops at "/", so the +// rule fired for "rm -rf tmp" but silently returned Ask for "rm -rf /". +func TestDenyGlobCrossesPathSeparator(t *testing.T) { + t.Parallel() + + checker := NewCheckerFromRules(nil, nil, []string{"shell:cmd=*rm -rf*"}) + + assert.Equal(t, Deny, checker.CheckWithArgs("shell", + map[string]any{"cmd": "rm -rf tmp"}), + "deny rule should block 'rm -rf tmp'") + assert.Equal(t, Deny, checker.CheckWithArgs("shell", + map[string]any{"cmd": "rm -rf /"}), + "deny rule must also block 'rm -rf /'") +} + func TestArgToString(t *testing.T) { t.Parallel()