From 8fadf378e045a91c89a30f0494fd3632d3ec14a3 Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Wed, 16 Oct 2024 19:19:58 -0500 Subject: [PATCH] i/p/requestrules,o/i/apparmorprompting: move request matching logic into requestrules Signed-off-by: Oliver Calder --- .../prompting/requestrules/export_test.go | 10 ++ .../prompting/requestrules/requestrules.go | 37 ++++- .../requestrules/requestrules_test.go | 146 +++++++++++++++++- .../ifacestate/apparmorprompting/prompting.go | 65 +++----- 4 files changed, 203 insertions(+), 55 deletions(-) diff --git a/interfaces/prompting/requestrules/export_test.go b/interfaces/prompting/requestrules/export_test.go index 1f27b0803ae..b70a0fafdac 100644 --- a/interfaces/prompting/requestrules/export_test.go +++ b/interfaces/prompting/requestrules/export_test.go @@ -21,6 +21,8 @@ package requestrules import ( "time" + + "github.com/snapcore/snapd/testutil" ) var JoinInternalErrors = joinInternalErrors @@ -30,3 +32,11 @@ type RulesDBJSON rulesDBJSON func (rule *Rule) Validate(currTime time.Time) (expired bool, err error) { return rule.validate(currTime) } + +func (rdb *RuleDB) IsPathPermAllowed(user uint32, snap string, iface string, path string, permission string) (bool, error) { + return rdb.isPathPermAllowed(user, snap, iface, path, permission) +} + +func MockIsPathPermAllowed(f func(rdb *RuleDB, user uint32, snap string, iface string, path string, permission string) (bool, error)) func() { + return testutil.Mock(&isPathPermAllowedByRuleDB, f) +} diff --git a/interfaces/prompting/requestrules/requestrules.go b/interfaces/prompting/requestrules/requestrules.go index 3caa1087063..1c617aac5dc 100644 --- a/interfaces/prompting/requestrules/requestrules.go +++ b/interfaces/prompting/requestrules/requestrules.go @@ -735,10 +735,43 @@ func (rdb *RuleDB) makeNewRule(user uint32, snap string, iface string, constrain return &newRule, nil } -// IsPathAllowed checks whether the given path with the given permission is +// IsRequestAllowed checks whether a request with the given parameters is +// allowed or denied by existing rules. +// +// If any of the given permissions are allowed, they are returned as +// allowedPerms. If any permissions are denied, then returns anyDenied as true. +// If any of the given permissions were not matched by an existing rule, then +// they are returned as outstandingPerms. If an error occurred, returns it. +func (rdb *RuleDB) IsRequestAllowed(user uint32, snap string, iface string, path string, permissions []string) (allowedPerms []string, anyDenied bool, outstandingPerms []string, err error) { + allowedPerms = make([]string, 0, len(permissions)) + outstandingPerms = make([]string, 0, len(permissions)) + var errs []error + for _, perm := range permissions { + allowed, err := isPathPermAllowedByRuleDB(rdb, user, snap, iface, path, perm) + switch { + case err == nil: + if allowed { + allowedPerms = append(allowedPerms, perm) + } else { + anyDenied = true + } + case errors.Is(err, prompting_errors.ErrNoMatchingRule): + outstandingPerms = append(outstandingPerms, perm) + default: + errs = append(errs, err) + } + } + return allowedPerms, anyDenied, outstandingPerms, strutil.JoinErrors(errs...) +} + +var isPathPermAllowedByRuleDB = func(rdb *RuleDB, user uint32, snap string, iface string, path string, permission string) (bool, error) { + return rdb.isPathPermAllowed(user, snap, iface, path, permission) +} + +// isPathPermAllowed checks whether the given path with the given permission is // allowed or denied by existing rules for the given user, snap, and interface. // If no rule applies, returns prompting_errors.ErrNoMatchingRule. -func (rdb *RuleDB) IsPathAllowed(user uint32, snap string, iface string, path string, permission string) (bool, error) { +func (rdb *RuleDB) isPathPermAllowed(user uint32, snap string, iface string, path string, permission string) (bool, error) { rdb.mutex.RLock() defer rdb.mutex.RUnlock() permissionMap, ok := rdb.permissionDBForUserSnapInterfacePermission(user, snap, iface, permission) diff --git a/interfaces/prompting/requestrules/requestrules_test.go b/interfaces/prompting/requestrules/requestrules_test.go index 1dd589ce74d..c04e8020607 100644 --- a/interfaces/prompting/requestrules/requestrules_test.go +++ b/interfaces/prompting/requestrules/requestrules_test.go @@ -968,7 +968,141 @@ func (s *requestrulesSuite) TestAddRulePartiallyExpired(c *C) { c.Check(exists, Equals, true) } -func (s *requestrulesSuite) TestIsPathAllowedSimple(c *C) { +type isPathPermAllowedResult struct { + allowed bool + err error +} + +func (s *requestrulesSuite) TestIsRequestAllowed(c *C) { + rdb, err := requestrules.New(nil) + c.Assert(err, IsNil) + c.Assert(rdb, NotNil) + + user := uint32(1234) + snap := "firefox" + iface := "powerful" + path := "/path/to/something" + + for _, testCase := range []struct { + requestedPerms []string + permReturns map[string]isPathPermAllowedResult + allowedPerms []string + anyDenied bool + outstandingPerms []string + errStr string + }{ + { + requestedPerms: []string{"foo", "bar", "baz"}, + permReturns: map[string]isPathPermAllowedResult{ + "foo": {true, nil}, + "bar": {true, nil}, + "baz": {true, nil}, + }, + allowedPerms: []string{"foo", "bar", "baz"}, + anyDenied: false, + outstandingPerms: []string{}, + errStr: "", + }, + { + requestedPerms: []string{"foo", "bar", "baz"}, + permReturns: map[string]isPathPermAllowedResult{ + "foo": {true, prompting_errors.ErrNoMatchingRule}, + "bar": {false, prompting_errors.ErrNoMatchingRule}, + "baz": {true, prompting_errors.ErrNoMatchingRule}, + }, + allowedPerms: []string{}, + anyDenied: false, + outstandingPerms: []string{"foo", "bar", "baz"}, + errStr: "", + }, + { + requestedPerms: []string{"foo", "bar", "baz"}, + permReturns: map[string]isPathPermAllowedResult{ + "foo": {true, prompting_errors.ErrNoMatchingRule}, + "bar": {true, nil}, + "baz": {false, prompting_errors.ErrNoMatchingRule}, + }, + allowedPerms: []string{"bar"}, + anyDenied: false, + outstandingPerms: []string{"foo", "baz"}, + errStr: "", + }, + { + requestedPerms: []string{"foo", "bar", "baz"}, + permReturns: map[string]isPathPermAllowedResult{ + "foo": {false, nil}, + "bar": {true, nil}, + "baz": {false, prompting_errors.ErrNoMatchingRule}, + }, + allowedPerms: []string{"bar"}, + anyDenied: true, + outstandingPerms: []string{"baz"}, + errStr: "", + }, + { + requestedPerms: []string{"foo", "bar"}, + permReturns: map[string]isPathPermAllowedResult{ + "foo": {true, nil}, + "bar": {true, nil}, + "baz": {true, fmt.Errorf("baz")}, + }, + allowedPerms: []string{"foo", "bar"}, + anyDenied: false, + outstandingPerms: []string{}, + errStr: "", + }, + { + requestedPerms: []string{"foo", "bar", "baz"}, + permReturns: map[string]isPathPermAllowedResult{ + "foo": {true, fmt.Errorf("foo")}, + "bar": {true, nil}, + "baz": {true, nil}, + }, + allowedPerms: []string{"bar", "baz"}, + anyDenied: false, + outstandingPerms: []string{}, + errStr: "foo", + }, + { + requestedPerms: []string{"foo", "bar", "baz", "qux", "fizz", "buzz"}, + permReturns: map[string]isPathPermAllowedResult{ + "foo": {true, fmt.Errorf("foo")}, + "bar": {true, nil}, + "baz": {true, prompting_errors.ErrNoMatchingRule}, + "qux": {false, fmt.Errorf("qux")}, + "fizz": {false, nil}, + "buzz": {false, prompting_errors.ErrNoMatchingRule}, + }, + allowedPerms: []string{"bar"}, + anyDenied: true, + outstandingPerms: []string{"baz", "buzz"}, + errStr: "foo\nqux", + }, + } { + restore := requestrules.MockIsPathPermAllowed(func(r *requestrules.RuleDB, u uint32, s string, i string, p string, perm string) (bool, error) { + c.Assert(r, Equals, rdb) + c.Assert(u, Equals, user) + c.Assert(s, Equals, snap) + c.Assert(i, Equals, iface) + c.Assert(p, Equals, path) + result := testCase.permReturns[perm] + return result.allowed, result.err + }) + defer restore() + + allowedPerms, anyDenied, outstandingPerms, err := rdb.IsRequestAllowed(user, snap, iface, path, testCase.requestedPerms) + c.Check(allowedPerms, DeepEquals, testCase.allowedPerms) + c.Check(anyDenied, Equals, testCase.anyDenied) + c.Check(outstandingPerms, DeepEquals, testCase.outstandingPerms) + if testCase.errStr == "" { + c.Check(err, IsNil) + } else { + c.Check(err, ErrorMatches, testCase.errStr) + } + } +} + +func (s *requestrulesSuite) TestIsPathPermAllowedSimple(c *C) { // Target user := s.defaultUser snap := "firefox" @@ -1051,7 +1185,7 @@ func (s *requestrulesSuite) TestIsPathAllowedSimple(c *C) { s.checkNewNoticesSimple(c, nil) } - allowed, err := rdb.IsPathAllowed(user, snap, iface, path, permission) + allowed, err := rdb.IsPathPermAllowed(user, snap, iface, path, permission) c.Check(err, Equals, testCase.err) c.Check(allowed, Equals, testCase.allowed) // Check that no notices were recorded when checking @@ -1065,7 +1199,7 @@ func (s *requestrulesSuite) TestIsPathAllowedSimple(c *C) { } } -func (s *requestrulesSuite) TestIsPathAllowedPrecedence(c *C) { +func (s *requestrulesSuite) TestIsPathPermAllowedPrecedence(c *C) { // Target user := s.defaultUser snap := "firefox" @@ -1122,13 +1256,13 @@ func (s *requestrulesSuite) TestIsPathAllowedPrecedence(c *C) { mostRecentOutcome, err := ruleContents.Outcome.AsBool() c.Check(err, IsNil) - allowed, err := rdb.IsPathAllowed(user, snap, iface, path, permission) + allowed, err := rdb.IsPathPermAllowed(user, snap, iface, path, permission) c.Check(err, IsNil) c.Check(allowed, Equals, mostRecentOutcome, Commentf("most recent: %+v", ruleContents)) } } -func (s *requestrulesSuite) TestIsPathAllowedExpiration(c *C) { +func (s *requestrulesSuite) TestIsPathPermAllowedExpiration(c *C) { // Target user := s.defaultUser snap := "firefox" @@ -1191,7 +1325,7 @@ func (s *requestrulesSuite) TestIsPathAllowedExpiration(c *C) { c.Check(err, IsNil) // Check that the outcome of the most specific unexpired rule has precedence - allowed, err := rdb.IsPathAllowed(user, snap, iface, path, permission) + allowed, err := rdb.IsPathPermAllowed(user, snap, iface, path, permission) c.Check(err, IsNil) c.Check(allowed, Equals, expectedOutcome, Commentf("last unexpired: %+v", rule)) diff --git a/overlord/ifacestate/apparmorprompting/prompting.go b/overlord/ifacestate/apparmorprompting/prompting.go index 0e9813b7dcf..f45a51fc758 100644 --- a/overlord/ifacestate/apparmorprompting/prompting.go +++ b/overlord/ifacestate/apparmorprompting/prompting.go @@ -20,7 +20,6 @@ package apparmorprompting import ( - "errors" "fmt" "sync" @@ -212,54 +211,26 @@ func (m *InterfacesRequestsManager) handleListenerReq(req *listener.Request) err return requestReply(req, nil) } - outstandingPerms := make([]string, 0, len(permissions)) - satisfiedPerms := make([]string, 0, len(permissions)) - // we're done with early checks, serious business starts now, and we can // take the lock m.lock.Lock() defer m.lock.Unlock() - matchedDenyRule := false - for _, perm := range permissions { - // TODO: move this for-loop to a helper in requestrules - if yesNo, err := m.rules.IsPathAllowed(userID, snap, iface, path, perm); err == nil { - if !yesNo { - matchedDenyRule = true - } else { - satisfiedPerms = append(satisfiedPerms, perm) - } - } else { - if !errors.Is(err, prompting_errors.ErrNoMatchingRule) { - logger.Noticef("error while checking request against existing rules: %v", err) - } - // No matching rule found - outstandingPerms = append(outstandingPerms, perm) + allowedPerms, matchedDenyRule, outstandingPerms, err := m.rules.IsRequestAllowed(userID, snap, iface, path, permissions) + if err != nil || matchedDenyRule || len(outstandingPerms) == 0 { + switch { + case err != nil: + logger.Noticef("error while checking request against existing rules: %v", err) + case matchedDenyRule: + logger.Debugf("request denied by existing rule: %+v", req) + case len(outstandingPerms) == 0: + logger.Debugf("request allowed by existing rule: %+v", req) } - } - if matchedDenyRule { - logger.Debugf("request denied by existing rule: %+v", req) - - // Respond with this information by allowing any requested permissions - // which were explicitly allowed by existing rules (there may be no - // such permissions) and let the listener deny all permissions which - // were not explicitly included in the allowed permissions. - allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, satisfiedPerms) - // Error should not occur, but if it does, allowedPermission is set to - // empty, leaving it to the listener to default deny all permissions. - return requestReply(req, allowedPermission) - } - - if len(outstandingPerms) == 0 { - logger.Debugf("request allowed by existing rule: %+v", req) - - // We don't want to just send back req.Permission() here, since that - // could include unrecognized permissions which were discarded, and - // were not matched by an existing rule. So only respond with the - // permissions which were matched and allowed, the listener will - // deny any permissions which were not explicitly included in the - // allowed permissions. - allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, satisfiedPerms) + // Allow any requested permissions which were explicitly allowed by + // existing rules (there may be no such permissions) and let the + // listener deny all permissions which were not explicitly included in + // the allowed permissions. + allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, allowedPerms) // Error should not occur, but if it does, allowedPermission is set to // empty, leaving it to the listener to default deny all permissions. return requestReply(req, allowedPermission) @@ -280,18 +251,18 @@ func (m *InterfacesRequestsManager) handleListenerReq(req *listener.Request) err // We weren't able to create a new prompt, so respond with the best // information we have, which is to allow any permissions which were // allowed by existing rules, and let the listener deny the rest. - allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, satisfiedPerms) + allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, allowedPerms) // Error should not occur, but if it does, allowedPermission is set to // empty, leaving it to the listener to default deny all permissions. return requestReply(req, allowedPermission) } + if merged { logger.Debugf("new prompt merged with identical existing prompt: %+v", newPrompt) - return nil + } else { + logger.Debugf("adding prompt to internal storage: %+v", newPrompt) } - logger.Debugf("adding prompt to internal storage: %+v", newPrompt) - return nil }