diff --git a/docs/api/v2/components/schemas/PromptingPromptConstraintsHome.yaml b/docs/api/v2/components/schemas/PromptingPromptConstraintsHome.yaml index e569245d294..b22a926bad2 100644 --- a/docs/api/v2/components/schemas/PromptingPromptConstraintsHome.yaml +++ b/docs/api/v2/components/schemas/PromptingPromptConstraintsHome.yaml @@ -10,7 +10,13 @@ required: properties: path: type: string - description: The path for which access is requested. + description: | + The path for which access is requested. + + Any characters which are considered special in reply path patterns will + be escaped by a backslash ('\') character. That way, the "path" field in the prompt + can be used directly as the "path-pattern" field in a reply and exactly + match the requested path. example: /home/ubuntu/Downloads/image.png requested-permissions: type: array diff --git a/interfaces/prompting/patterns/patterns_test.go b/interfaces/prompting/patterns/patterns_test.go index 3cf6d5bd21e..2222c5012ab 100644 --- a/interfaces/prompting/patterns/patterns_test.go +++ b/interfaces/prompting/patterns/patterns_test.go @@ -36,6 +36,42 @@ type patternsSuite struct{} var _ = Suite(&patternsSuite{}) +func (s *patternsSuite) TestEscapeLiteralPathMatchesOriginalPath(c *C) { + for _, testCase := range []struct { + original string + expected string + }{ + {`/foo/bar`, `/foo/bar`}, + {`/foo*bar`, `/foo\*bar`}, + {`/foo?bar`, `/foo\?bar`}, + {`/foo\bar`, `/foo\\bar`}, + {`/foo(bar,baz)`, `/foo(bar,baz)`}, // () are not special, so not escaped + {`/foo[bar,baz]`, `/foo\[bar,baz\]`}, + {`/foo{bar,baz}`, `/foo\{bar,baz\}`}, + {`/foo\*bar`, `/foo\\\*bar`}, + {`/foo\?bar`, `/foo\\\?bar`}, + {`/foo\\bar`, `/foo\\\\bar`}, + {`/foo\(bar,baz\)`, `/foo\\(bar,baz\\)`}, // () are not special, so not escaped + {`/foo\[bar,baz\]`, `/foo\\\[bar,baz\\\]`}, + {`/foo\{bar,baz\}`, `/foo\\\{bar,baz\\\}`}, + {`/foo*?()[]{}'",\`, `/foo\*\?()\[\]\{\}'",\\`}, + {`/foo/bar/[アニメ][ゲーム動画].mkv`, `/foo/bar/\[アニメ\]\[ゲーム動画\].mkv`}, + } { + result := patterns.EscapeLiteralPath(testCase.original) + c.Check(result, Equals, testCase.expected) + + // Check that a path pattern equal to the escaped path does indeed + // match the original path + expectedPathPattern, err := patterns.ParsePathPattern(testCase.expected) + c.Assert(err, IsNil, Commentf("testCase: %+v", testCase)) + c.Assert(expectedPathPattern, NotNil, Commentf("testCase: %+v", testCase)) + c.Check(expectedPathPattern.String(), Equals, testCase.expected, Commentf("testCase: %+v", testCase)) + matches, err := expectedPathPattern.Match(testCase.original) + c.Check(err, IsNil, Commentf("testCase: %+v", testCase)) + c.Check(matches, Equals, true, Commentf("testCase: %+v", testCase)) + } +} + func (s *patternsSuite) TestParsePathPatternHappy(c *C) { for _, pattern := range []string{ "/", diff --git a/interfaces/prompting/patterns/variant.go b/interfaces/prompting/patterns/variant.go index 75bbf4f2b39..aa113705682 100644 --- a/interfaces/prompting/patterns/variant.go +++ b/interfaces/prompting/patterns/variant.go @@ -179,14 +179,21 @@ func (c component) componentRegex() string { var escapeFinder = regexp.MustCompile(`\\(.)`) -// unescapeLiteral removes any `\` characters which are used to escape another -// character. Note that escaped `\` characters are not removed, since they are +// unescapeLiteral removes any '\' characters which are used to escape another +// character. Note that escaped '\' characters are not removed, since they are // not acting as an escape character in those instances. That is, `\\` is // reduced to `\`. func unescapeLiteral(literal string) string { return escapeFinder.ReplaceAllString(literal, "${1}") } +var escaper = regexp.MustCompile(`([\*\?\[\]\{\}\\])`) + +// EscapeLiteralPath escapes any special characters from the given path. +func EscapeLiteralPath(path string) string { + return escaper.ReplaceAllString(path, `\${1}`) +} + type PatternVariant struct { variant string components []component diff --git a/interfaces/prompting/requestprompts/requestprompts.go b/interfaces/prompting/requestprompts/requestprompts.go index b03a0bbccec..a86e9b35d23 100644 --- a/interfaces/prompting/requestprompts/requestprompts.go +++ b/interfaces/prompting/requestprompts/requestprompts.go @@ -37,6 +37,7 @@ import ( "github.com/snapcore/snapd/interfaces/prompting" prompting_errors "github.com/snapcore/snapd/interfaces/prompting/errors" "github.com/snapcore/snapd/interfaces/prompting/internal/maxidmmap" + "github.com/snapcore/snapd/interfaces/prompting/patterns" "github.com/snapcore/snapd/logger" "github.com/snapcore/snapd/osutil" "github.com/snapcore/snapd/strutil" @@ -225,7 +226,7 @@ func (pc *promptConstraints) marshalForInterface(iface string) ([]byte, error) { switch iface { case "home": constraintsJSON := &promptConstraintsJSONHome{ - Path: pc.path, + Path: pc.EscapedPath(), RequestedPermissions: pc.outstandingPermissions, AvailablePermissions: pc.availablePermissions, } @@ -274,7 +275,7 @@ func (pc *promptConstraints) equals(other *promptConstraints) bool { // then affectedByRule is false, and no changes are made to the prompt // constraints. func (pc *promptConstraints) applyRuleConstraints(constraints *prompting.RuleConstraints) (affectedByRule, respond bool, deniedPermissions []string, err error) { - pathMatched, err := constraints.Match(pc.path) + pathMatched, err := constraints.Match(pc.Path()) if err != nil { // Should not occur, only error is if path pattern is malformed, // which would have thrown an error while parsing, not now. @@ -341,11 +342,24 @@ func (pc *promptConstraints) buildResponse(deniedPermissions []string) []string } // Path returns the path associated with the request to which the receiving -// prompt constraints apply. +// prompt constraints apply. This is the literal path, without special path +// pattern characters escaped. This should be used when matching patterns +// against prompts. func (pc *promptConstraints) Path() string { return pc.path } +// EscapedPath returns the path associated with prompt constraints, with any +// special path pattern characters escaped by a '\' character. This should be +// used in order to create a path pattern which matches the requested path. +// Thus, it should also be used when marshalling prompt constraints to send to +// a prompting client, as clients should be able to reply using the exact path +// they received in the prompt as the path pattern and have that reply apply to +// the requested path. +func (pc *promptConstraints) EscapedPath() string { + return patterns.EscapeLiteralPath(pc.path) +} + // OutstandingPermissions returns the outstanding unsatisfied permissions // associated with the prompt. func (pc *promptConstraints) OutstandingPermissions() []string { diff --git a/interfaces/prompting/requestprompts/requestprompts_test.go b/interfaces/prompting/requestprompts/requestprompts_test.go index 862415ffff2..771b7af28c8 100644 --- a/interfaces/prompting/requestprompts/requestprompts_test.go +++ b/interfaces/prompting/requestprompts/requestprompts_test.go @@ -503,6 +503,7 @@ func (s *requestpromptsSuite) TestAddOrMergeNonMerges(c *C) { c.Check(prompt1.Cgroup, Equals, metadata.Cgroup) c.Check(prompt1.Interface, Equals, metadata.Interface) c.Check(prompt1.Constraints.Path(), Equals, path) + c.Check(prompt1.Constraints.EscapedPath(), Equals, path) c.Check(prompt1.Constraints.OutstandingPermissions(), DeepEquals, permissions) c.Assert(prompt1.Requests(), HasLen, 1) c.Check(prompt1.Requests()[0].Key, Equals, "fake:1") @@ -540,6 +541,7 @@ func (s *requestpromptsSuite) TestAddOrMergeNonMerges(c *C) { c.Check(prompt2.Cgroup, Equals, metadata.Cgroup) c.Check(prompt2.Interface, Equals, metadata.Interface) c.Check(prompt2.Constraints.Path(), Equals, path) + c.Check(prompt2.Constraints.EscapedPath(), Equals, path) c.Check(prompt2.Constraints.OutstandingPermissions(), DeepEquals, permissions) // Request was added to the requests list @@ -582,6 +584,7 @@ func (s *requestpromptsSuite) TestAddOrMergeNonMerges(c *C) { c.Check(prompt3.Cgroup, Equals, metadata.Cgroup) c.Check(prompt3.Interface, Equals, metadata.Interface) c.Check(prompt3.Constraints.Path(), Equals, path) + c.Check(prompt3.Constraints.EscapedPath(), Equals, path) c.Check(prompt3.Constraints.OutstandingPermissions(), DeepEquals, permissions) c.Assert(prompt3.Requests(), HasLen, 1) c.Check(prompt3.Requests()[0].Key, Equals, "fake:3") @@ -624,6 +627,7 @@ func (s *requestpromptsSuite) TestAddOrMergeNonMerges(c *C) { c.Check(prompt4.Cgroup, Equals, metadata.Cgroup) c.Check(prompt4.Interface, Equals, metadata.Interface) c.Check(prompt4.Constraints.Path(), Equals, path) + c.Check(prompt4.Constraints.EscapedPath(), Equals, path) c.Check(prompt4.Constraints.OutstandingPermissions(), DeepEquals, permissions) c.Assert(prompt4.Requests(), HasLen, 1) c.Check(prompt4.Requests()[0].Key, Equals, "fake:4") @@ -668,6 +672,7 @@ func (s *requestpromptsSuite) TestAddOrMergeNonMerges(c *C) { c.Check(prompt5.Cgroup, Equals, metadata.Cgroup) c.Check(prompt5.Interface, Equals, metadata.Interface) c.Check(prompt5.Constraints.Path(), Equals, path) + c.Check(prompt5.Constraints.EscapedPath(), Equals, path) c.Check(prompt5.Constraints.OutstandingPermissions(), DeepEquals, permissions) c.Assert(prompt5.Requests(), HasLen, 1) c.Check(prompt5.Requests()[0].Key, Equals, "fake:5") @@ -715,6 +720,7 @@ func (s *requestpromptsSuite) TestAddOrMergeNonMerges(c *C) { c.Check(prompt6.Cgroup, Equals, metadata.Cgroup) c.Check(prompt6.Interface, Equals, metadata.Interface) c.Check(prompt6.Constraints.Path(), Equals, path) + c.Check(prompt6.Constraints.EscapedPath(), Equals, path) c.Check(prompt6.Constraints.OutstandingPermissions(), DeepEquals, permissions) c.Assert(prompt6.Requests(), HasLen, 1) c.Check(prompt6.Requests()[0].Key, Equals, "fake:6") @@ -819,6 +825,7 @@ func (s *requestpromptsSuite) TestAddOrMergeMerges(c *C) { c.Check(prompt1.Cgroup, Equals, metadata.Cgroup) c.Check(prompt1.Interface, Equals, metadata.Interface) c.Check(prompt1.Constraints.Path(), Equals, path) + c.Check(prompt1.Constraints.EscapedPath(), Equals, path) c.Check(prompt1.Constraints.OutstandingPermissions(), DeepEquals, permissions) stored, err = pdb.Prompts(metadata.User, clientActivity) @@ -853,6 +860,63 @@ func (s *requestpromptsSuite) TestAddOrMergeMerges(c *C) { s.checkWrittenRequestMap(c, expectedMap) } +func (s *requestpromptsSuite) TestAddOrMergeEscapesPath(c *C) { + // Mock timer so we don't get irrelevant timeouts during the test + restore := requestprompts.MockTimeAfterFunc(func(d time.Duration, f func()) timeutil.Timer { + return testtime.AfterFunc(d, f) + }) + defer restore() + + pdb, err := requestprompts.New(s.defaultNotifyPrompt) + c.Assert(err, IsNil) + defer pdb.Close() + + metadata := &prompting.Metadata{ + User: s.defaultUser, + Snap: "nextcloud", + PID: 1234, + Cgroup: "some/cgroup/path", + Interface: "home", + } + permissions := []string{"read", "write", "execute"} + + for i, testCase := range []struct { + originalPath string + escapedPath string + }{ + // Normal path + {`/foo/bar`, `/foo/bar`}, + {`/foo(bar,baz)`, `/foo(bar,baz)`}, // () are not special, so not escaped + // Paths with individual special characters + {`/foo*bar`, `/foo\*bar`}, + {`/foo?bar`, `/foo\?bar`}, + {`/foo\bar`, `/foo\\bar`}, + {`/foo[bar,baz]`, `/foo\[bar,baz\]`}, + {`/foo{bar,baz}`, `/foo\{bar,baz\}`}, + // Paths with special characters preceded by a literal '\' (as decoy) + {`/foo\*bar`, `/foo\\\*bar`}, + {`/foo\?bar`, `/foo\\\?bar`}, + {`/foo\\bar`, `/foo\\\\bar`}, + {`/foo\(bar,baz\)`, `/foo\\(bar,baz\\)`}, // () are not special, so not escaped + {`/foo\[bar,baz\]`, `/foo\\\[bar,baz\\\]`}, + {`/foo\{bar,baz\}`, `/foo\\\{bar,baz\\\}`}, + // Path with all the special characters and some not so special characters + {`/foo*?()[]{}'",\`, `/foo\*\?()\[\]\{\}'",\\`}, + // Path with square brackets and unicode characters + {`/foo/bar/[アニメ][ゲーム動画].mkv`, `/foo/bar/\[アニメ\]\[ゲーム動画\].mkv`}, + } { + req := &prompting.Request{Key: fmt.Sprintf("fake:%d", i)} + + prompt, merged, err := pdb.AddOrMerge(metadata, testCase.originalPath, permissions, permissions, req) + c.Assert(err, IsNil) + c.Assert(prompt, NotNil) + c.Assert(merged, Equals, false) + + c.Check(prompt.Constraints.Path(), Equals, testCase.originalPath) + c.Check(prompt.Constraints.EscapedPath(), Equals, testCase.escapedPath) + } +} + func (s *requestpromptsSuite) TestAddOrMergeDuplicateRequests(c *C) { // Mock timer so we don't get irrelevant timeouts during the test restore := requestprompts.MockTimeAfterFunc(func(d time.Duration, f func()) timeutil.Timer { @@ -1400,6 +1464,33 @@ func (s *requestpromptsSuite) TestReplyErrors(c *C) { } func (s *requestpromptsSuite) TestHandleNewRule(c *C) { + for _, testCase := range []struct { + requestedPath string + replyPattern string + }{ + {`/home/test/Documents/foo.txt`, `/home/test/Documents/foo.txt`}, + {`/home/test/Documents/foo.txt`, `/home/test/Documents/**`}, + {`/foo*bar`, `/foo\*bar`}, + {`/foo*bar`, `/foo*bar`}, // if we happen to reply with a globstar, it matches literal '*' + {`/foo*bar`, `/foo*`}, + {`/foo*bar`, `/foo*\**`}, + {`/foo?bar`, `/foo\?bar`}, + {`/foo?bar`, `/foo\??ar`}, + {`/foo?bar`, `/foo?bar`}, // if we reply with a ?, it matches literal '?' + {`/foo(bar,baz)`, `/foo(bar,baz)`}, // technically, '(' and ')' are not special + {`/foo(bar,baz)`, `/foo\(bar,baz\)`}, // but they can be escaped all the same + {`/foo[bar]`, `/foo\[bar\]`}, + {`/foo{bar,baz}`, `/foo\{bar,baz\}`}, + {`/foo{bar,baz}`, `/foo{xyz,\{bar\,baz\}}`}, + {`/foo*?()[]{}'",\`, `/foo\*\?()\[\]\{\}'",\\`}, + {`/foo/bar/[アニメ][ゲーム動画].mkv`, `/foo/bar/\[アニメ\]\[ゲーム動画\].mkv`}, + } { + s.SetUpTest(c) + s.testHandleNewRule(c, testCase.requestedPath, testCase.replyPattern) + } +} + +func (s *requestpromptsSuite) testHandleNewRule(c *C, requestedPath, replyPattern string) { // Mock timer so we don't get irrelevant timeouts during the test restore := requestprompts.MockTimeAfterFunc(func(d time.Duration, f func()) timeutil.Timer { return testtime.AfterFunc(d, f) @@ -1417,29 +1508,28 @@ func (s *requestpromptsSuite) TestHandleNewRule(c *C) { Cgroup: "some-cgroup-path", Interface: "home", } - path := "/home/test/Documents/foo.txt" permissions1 := []string{"read", "write", "execute"} req1, replyChan1 := newRequestWithReplyChan("fake:12") - prompt1, merged, err := pdb.AddOrMerge(metadata, path, permissions1, permissions1, req1) + prompt1, merged, err := pdb.AddOrMerge(metadata, requestedPath, permissions1, permissions1, req1) c.Assert(err, IsNil) c.Check(merged, Equals, false) permissions2 := []string{"read", "write"} req2, replyChan2 := newRequestWithReplyChan("fake:34") - prompt2, merged, err := pdb.AddOrMerge(metadata, path, permissions2, permissions2, req2) + prompt2, merged, err := pdb.AddOrMerge(metadata, requestedPath, permissions2, permissions2, req2) c.Assert(err, IsNil) c.Check(merged, Equals, false) permissions3 := []string{"read"} req3, replyChan3 := newRequestWithReplyChan("fake:56") - prompt3, merged, err := pdb.AddOrMerge(metadata, path, permissions3, permissions3, req3) + prompt3, merged, err := pdb.AddOrMerge(metadata, requestedPath, permissions3, permissions3, req3) c.Assert(err, IsNil) c.Check(merged, Equals, false) permissions4 := []string{"open"} req4 := &prompting.Request{Key: "fake:78"} // Reply should not occur, so panic if called - prompt4, merged, err := pdb.AddOrMerge(metadata, path, permissions4, permissions4, req4) + prompt4, merged, err := pdb.AddOrMerge(metadata, requestedPath, permissions4, permissions4, req4) c.Assert(err, IsNil) c.Check(merged, Equals, false) @@ -1457,7 +1547,7 @@ func (s *requestpromptsSuite) TestHandleNewRule(c *C) { c.Assert(err, IsNil) c.Assert(stored, HasLen, 4) - pathPattern, err := patterns.ParsePathPattern("/home/test/Documents/**") + pathPattern, err := patterns.ParsePathPattern(replyPattern) c.Assert(err, IsNil) constraints := &prompting.RuleConstraints{ InterfaceSpecific: &prompting.InterfaceSpecificConstraintsHome{ @@ -1476,7 +1566,7 @@ func (s *requestpromptsSuite) TestHandleNewRule(c *C) { satisfied, err := pdb.HandleNewRule(metadata, constraints) c.Assert(err, IsNil) - c.Check(satisfied, HasLen, 2) + c.Check(satisfied, HasLen, 2, Commentf("requestedPath: %q, replyPattern: %q", requestedPath, replyPattern)) c.Check(promptIDListContains(satisfied, prompt1.ID), Equals, true) c.Check(promptIDListContains(satisfied, prompt3.ID), Equals, true) @@ -1543,6 +1633,29 @@ func promptIDListContains(haystack []prompting.IDType, needle prompting.IDType) } func (s *requestpromptsSuite) TestHandleNewRuleNonMatches(c *C) { + for _, testCase := range []struct { + requestedPath string + matchingPattern string + nonMatchingPattern string + }{ + {`/home/test/Documents/foo.txt`, `/home/test/Documents/foo.txt`, `/home/test/Documents/bar.txt`}, + {`/home/test/Documents/foo.txt`, `/home/test/Documents/foo.txt`, `/home/test/Pictures/**`}, + {`/foo*bar`, `/foo\*bar`, `/foobar`}, + {`/foo*bar`, `/foo\*bar`, `/foo\*\*bar`}, + {`/foo*bar`, `/foo\*bar`, `/foo\*`}, + {`/foo?bar`, `/foo\?bar`, `/fooxbar`}, + {`/foo?bar`, `/foo\?bar`, `/foo?\?bar`}, + {`/foo{bar,baz}`, `/foo\{bar,baz\}`, `/foo{bar,baz}`}, + {`/foo*?()[]{}'",\`, `/foo\*\?()\[\]\{\}'",\\`, `/foo*?()\[\]{}'",\\`}, // () are not special so do not need to be escaped + {`/foo*?()[]{}'",\`, `/foo\*\?\(\)\[\]\{\}'",\\`, `/foo*?()\[\]{}'",\\`}, + {`/foo/bar/[アニメ][ゲーム動画].mkv`, `/foo/bar/\[アニメ\]\[ゲーム動画\].mkv`, `/foo/bar/アニメゲーム動画.mkv`}, + } { + s.SetUpTest(c) + s.testHandleNewRuleNonMatches(c, testCase.requestedPath, testCase.matchingPattern, testCase.nonMatchingPattern) + } +} + +func (s *requestpromptsSuite) testHandleNewRuleNonMatches(c *C, requestedPath, matchingPattern, nonMatchingPattern string) { // Mock timer so we don't get irrelevant timeouts during the test restore := requestprompts.MockTimeAfterFunc(func(d time.Duration, f func()) timeutil.Timer { return testtime.AfterFunc(d, f) @@ -1563,10 +1676,9 @@ func (s *requestpromptsSuite) TestHandleNewRuleNonMatches(c *C) { Cgroup: "0::/user.slice/user-1000.slice/user@1000.service/app.slice/some-cgroup.scope", Interface: iface, } - path := "/home/test/Documents/foo.txt" permissions := []string{"read"} req, replyChan := newRequestWithReplyChan("fake:1") - prompt, merged, err := pdb.AddOrMerge(metadata, path, permissions, permissions, req) + prompt, merged, err := pdb.AddOrMerge(metadata, requestedPath, permissions, permissions, req) c.Assert(err, IsNil) c.Check(merged, Equals, false) @@ -1576,7 +1688,7 @@ func (s *requestpromptsSuite) TestHandleNewRuleNonMatches(c *C) { metadata.PID = 0 metadata.Cgroup = "" - pathPattern, err := patterns.ParsePathPattern("/home/test/Documents/**") + pathPattern, err := patterns.ParsePathPattern(matchingPattern) c.Assert(err, IsNil) constraints := &prompting.RuleConstraints{ InterfaceSpecific: &prompting.InterfaceSpecificConstraintsHome{ @@ -1599,7 +1711,7 @@ func (s *requestpromptsSuite) TestHandleNewRuleNonMatches(c *C) { otherUser := user + 1 otherSnap := "ldx" otherInterface := "system-files" - otherPattern, err := patterns.ParsePathPattern("/home/test/Pictures/**.png") + otherPattern, err := patterns.ParsePathPattern(nonMatchingPattern) c.Assert(err, IsNil) otherConstraints := &prompting.RuleConstraints{ InterfaceSpecific: &prompting.InterfaceSpecificConstraintsHome{ @@ -2233,6 +2345,20 @@ func (s *requestpromptsSuite) TestPromptMarshalJSON(c *C) { outstandingPerms: []string{"access"}, expected: `{"id":"0000000000000003","timestamp":"2024-08-14T09:47:03.350324989-05:00","snap":"protonmail-bridge","pid":1248,"cgroup":"0::/user.slice/user-1000.slice/user@1000.service/app.slice/some-cgroup.scope","interface":"audio-record","constraints":{"requested-permissions":["access"],"available-permissions":["access"]}}`, }, + { + // Path with special characters + metadata: &prompting.Metadata{ + User: s.defaultUser, + Snap: "firefox", + PID: 1234, + Cgroup: "0::/user.slice/user-1000.slice/user@1000.service/app.slice/some-cgroup.scope", + Interface: "home", + }, + path: `/home/test/foo*?()[]{}'",\`, + requestedPerms: []string{"write"}, + outstandingPerms: []string{"write"}, + expected: `{"id":"0000000000000004","timestamp":"2024-08-14T09:47:03.350324989-05:00","snap":"firefox","pid":1234,"cgroup":"0::/user.slice/user-1000.slice/user@1000.service/app.slice/some-cgroup.scope","interface":"home","constraints":{"path":"/home/test/foo\\*\\?()\\[\\]\\{\\}'\",\\\\","requested-permissions":["write"],"available-permissions":["read","write","execute"]}}`, + }, } { fakeRequest := &prompting.Request{Key: fmt.Sprintf("fake:%d", reqCount)} reqCount++ diff --git a/overlord/ifacestate/apparmorprompting/prompting.go b/overlord/ifacestate/apparmorprompting/prompting.go index b710245b0c0..35d119844d9 100644 --- a/overlord/ifacestate/apparmorprompting/prompting.go +++ b/overlord/ifacestate/apparmorprompting/prompting.go @@ -469,7 +469,9 @@ func (m *InterfacesRequestsManager) HandleReply(userID uint32, promptID promptin } if !matches { return nil, &prompting_errors.RequestedPathNotMatchedError{ - Requested: prompt.Constraints.Path(), + // XXX: it's a bit complicated, as we really send EscapedPath() but + // we check whether it matches against Path() + Requested: prompt.Constraints.EscapedPath(), Replied: constraints.PathPattern().String(), } } diff --git a/overlord/ifacestate/apparmorprompting/prompting_test.go b/overlord/ifacestate/apparmorprompting/prompting_test.go index 825f2a75972..3d0aa3a96d8 100644 --- a/overlord/ifacestate/apparmorprompting/prompting_test.go +++ b/overlord/ifacestate/apparmorprompting/prompting_test.go @@ -442,6 +442,89 @@ func waitForReply(replyChan chan []string) ([]string, error) { } } +func (s *apparmorpromptingSuite) TestHandleReplyUnusualPaths(c *C) { + _, reqChan, restore := apparmorprompting.MockListener() + defer restore() + + mgr, err := apparmorprompting.New(s.st) + c.Assert(err, IsNil) + + const clientActivity = true + + for i, testCase := range []struct { + originalPath string + escapedPath string + pathJSON string + }{ + // Normal path + {`/foo/bar`, `/foo/bar`, `"/foo/bar"`}, + {`/foo(bar,baz)`, `/foo(bar,baz)`, `"/foo(bar,baz)"`}, // () are not special, so not escaped + // Paths with individual special characters + {`/foo*bar`, `/foo\*bar`, `"/foo\\*bar"`}, + {`/foo?bar`, `/foo\?bar`, `"/foo\\?bar"`}, + {`/foo\bar`, `/foo\\bar`, `"/foo\\\\bar"`}, + {`/foo[bar,baz]`, `/foo\[bar,baz\]`, `"/foo\\[bar,baz\\]"`}, + {`/foo{bar,baz}`, `/foo\{bar,baz\}`, `"/foo\\{bar,baz\\}"`}, + // Paths with special characters preceded by a literal '\' (as decoy) + {`/foo\*bar`, `/foo\\\*bar`, `"/foo\\\\\\*bar"`}, + {`/foo\?bar`, `/foo\\\?bar`, `"/foo\\\\\\?bar"`}, + {`/foo\\bar`, `/foo\\\\bar`, `"/foo\\\\\\\\bar"`}, + {`/foo\(bar,baz\)`, `/foo\\(bar,baz\\)`, `"/foo\\\\(bar,baz\\\\)"`}, // () are not special, so not escaped + {`/foo\[bar,baz\]`, `/foo\\\[bar,baz\\\]`, `"/foo\\\\\\[bar,baz\\\\\\]"`}, + {`/foo\{bar,baz\}`, `/foo\\\{bar,baz\\\}`, `"/foo\\\\\\{bar,baz\\\\\\}"`}, + // Path with all the special characters and some not so special characters + {`/foo*?()[]{}'",\`, `/foo\*\?()\[\]\{\}'",\\`, `"/foo\\*\\?()\\[\\]\\{\\}'\",\\\\"`}, + // Path with square brackets and unicode characters + {`/foo/bar/[アニメ][ゲーム動画].mkv`, `/foo/bar/\[アニメ\]\[ゲーム動画\].mkv`, `"/foo/bar/\\[アニメ\\]\\[ゲーム動画\\].mkv"`}, + } { + key := fmt.Sprintf("fake:%d", i) + req, replyChan := requestWithReplyChan(&prompting.Request{Key: key, Path: testCase.originalPath}) + _, prompt := s.simulateRequest(c, reqChan, mgr, req, false) + + // Validate the paths presented by the prompt constraints + c.Assert(prompt.Constraints.Path(), Equals, testCase.originalPath, Commentf("testCase: %+v", testCase)) + c.Assert(prompt.Constraints.EscapedPath(), Equals, testCase.escapedPath, Commentf("testCase: %+v", testCase)) + + // Marshal the prompt to json so we can check the marshalled path + marshalled, err := prompt.MarshalJSON() + c.Assert(err, IsNil) + c.Check(string(marshalled), testutil.Contains, fmt.Sprintf(`"path":%s`, testCase.pathJSON), Commentf("testCase: %+v", testCase)) + + // Reply to the request with a path pattern equal to the prompt path as + // it was marshalled into JSON. + constraintsJSON := prompting.ConstraintsJSON{ + "path-pattern": json.RawMessage(testCase.pathJSON), + "permissions": json.RawMessage(`["read"]`), + } + + // First, validate that the escaped json pattern will be parsed as + // expected. + constraints, err := prompting.UnmarshalReplyConstraints("home", prompting.OutcomeAllow, prompting.LifespanSingle, "", constraintsJSON) + c.Assert(err, IsNil, Commentf("testCase: %+v", testCase)) + c.Check(constraints.PathPattern().String(), Equals, testCase.escapedPath, Commentf("testCase: %+v", testCase)) + // Next, check that the parsed path pattern matches the original path. + matches, err := constraints.PathPattern().Match(testCase.originalPath) + c.Assert(err, IsNil, Commentf("testCase: %+v", testCase)) + c.Check(matches, Equals, true, Commentf("testCase: %+v", testCase)) + matches, err = constraints.PathPattern().Match(prompt.Constraints.Path()) + c.Assert(err, IsNil, Commentf("testCase: %+v", testCase)) + c.Check(matches, Equals, true, Commentf("testCase: %+v", testCase)) + + // Now actually send reply + satisfied, err := mgr.HandleReply(s.defaultUser, prompt.ID, constraintsJSON, prompting.OutcomeAllow, prompting.LifespanSingle, "", clientActivity) + c.Check(err, IsNil, Commentf("testCase: %+v", testCase)) + c.Check(satisfied, HasLen, 0) + + // Simulate the listener receiving the response + allowedPermissions, err := waitForReply(replyChan) + c.Assert(err, IsNil) + expectedPerms := []string{"read"} + c.Check(allowedPermissions, DeepEquals, expectedPerms) + } + + c.Assert(mgr.Stop(), IsNil) +} + func (s *apparmorpromptingSuite) TestHandleReplyErrors(c *C) { _, reqChan, restore := apparmorprompting.MockListener() defer restore() diff --git a/tests/main/apparmor-prompting-integration-tests/special_characters.json b/tests/main/apparmor-prompting-integration-tests/special_characters.json new file mode 100644 index 00000000000..d19ff292c4c --- /dev/null +++ b/tests/main/apparmor-prompting-integration-tests/special_characters.json @@ -0,0 +1,28 @@ +{ + "version": 1, + "prompt-filter": { + "snap": "prompting-client", + "interface": "home", + "constraints": { + "path": "$BASE_PATH/.*" + } + }, + "prompts": [ + { + "prompt-filter": { + "constraints": { + "path": "${BASE_PATH}/\\\\\\[アニメ\\\\\\]\\\\\\[ゲーム動画\\\\\\].mkv", + "requested-permissions": [ "read" ] + } + }, + "reply": { + "action": "allow", + "lifespan": "single", + "constraints": { + "path-pattern": "${BASE_PATH}/\\[アニメ\\]\\[ゲーム動画\\].mkv", + "permissions": [ "read" ] + } + } + } + ] +} diff --git a/tests/main/apparmor-prompting-integration-tests/special_characters.sh b/tests/main/apparmor-prompting-integration-tests/special_characters.sh new file mode 100644 index 00000000000..2fab5fdd536 --- /dev/null +++ b/tests/main/apparmor-prompting-integration-tests/special_characters.sh @@ -0,0 +1,49 @@ +#!/usr/bin/sh + +# Test prompting for filepaths which contain special characters. + +TEST_DIR="$1" +TIMEOUT="$2" +if [ -z "$TIMEOUT" ] ; then + TIMEOUT=10 +fi + +FIRST_CONTENT="a file with square brackets and unicode" +SECOND_CONTENT="a file with all the special characters" + +# Prompt sequence prompt-filters are regular expressions, and they're stored as +# json, so we annoyingly have to escape special characters twice in just the +# prompt-filter path, with extra complexity for literal '\' characters. +echo "Prepare the files to be read" +echo "$FIRST_CONTENT" | tee "${TEST_DIR}/[アニメ][ゲーム動画].mkv" +echo "$SECOND_CONTENT" | tee "${TEST_DIR}/foo*?()[]{}\\" + +echo "Attempt to read the first file" +FIRST_OUTPUT="$(snap run --shell prompting-client.scripted -c "cat ${TEST_DIR}/'[アニメ][ゲーム動画].mkv'")" + +echo "Skip reading the second file as there's an issue with the prompting-client.scripted parsing the sequence" +# TODO: actually do the second read +#echo "Attempt to read the second file" +#SECOND_OUTPUT="$(snap run --shell prompting-client.scripted -c "cat ${TEST_DIR}/'foo*?()[]{}\\'")" + +# Wait for the client to write its result and exit +timeout "$TIMEOUT" sh -c "while pgrep -f 'prompting-client.scripted.*${TEST_DIR}' > /dev/null; do sleep 0.1; done" + +CLIENT_OUTPUT="$(cat "${TEST_DIR}/result")" + +if [ "$CLIENT_OUTPUT" != "success" ] ; then + echo "test failed" + echo "output='$CLIENT_OUTPUT'" + exit 1 +fi + +if [ "$FIRST_OUTPUT" != "$FIRST_CONTENT" ] ; then + echo "test script failed" + exit 1 +fi + +# TODO: actually check the second output +#if [ "$SECOND_OUTPUT" != "$SECOND_CONTENT" ] ; then +# echo "test script failed" +# exit 1 +#fi diff --git a/tests/main/apparmor-prompting-integration-tests/task.yaml b/tests/main/apparmor-prompting-integration-tests/task.yaml index a37c455b66e..28bedd9d18e 100644 --- a/tests/main/apparmor-prompting-integration-tests/task.yaml +++ b/tests/main/apparmor-prompting-integration-tests/task.yaml @@ -28,6 +28,7 @@ environment: VARIANT/download_file_safer: download_file_safer VARIANT/read_single_allow: read_single_allow VARIANT/read_single_deny: read_single_deny + VARIANT/special_characters: special_characters VARIANT/timespan_allow: timespan_allow VARIANT/timespan_deny: timespan_deny VARIANT/write_read_multiple_actioned_by_other_pid_allow_deny: write_read_multiple_actioned_by_other_pid_allow_deny