i/builtin: prohibit trailing '@' in common-files filepaths #14696
Conversation
For the `common-files` interface and those which use
`commonFilesInterface` (such as `system-files` and `personal-files`),
these append a trailing `{,/,/**}` to the end of each path.
The `@` character is allowed in filepaths specified in these plugs,
since filepaths like `/dev/foo@bar` may be required. However, if an `@`
character occurs at the end of the filepath (e.g. `/foo/bar@`), then the
resulting filepath in the rule is `/foo/bar@{,/,/**}`. AppArmor treats
`@{FOO}` as a variable, so having `@{,/,/**}` in the final rule snippet
is problematic.
This commit adds a check to `common_files.go` to ensure that filepaths
do not have a trailing `@`.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14696 +/- ##
==========================================
+ Coverage 78.95% 78.96% +0.01%
==========================================
Files 1084 1085 +1
Lines 146638 147112 +474
==========================================
+ Hits 115773 116169 +396
- Misses 23667 23719 +52
- Partials 7198 7224 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bboozzoo
added
the
Needs security review
| @@ -104,6 +104,9 @@ func (iface *commonFilesInterface) validateSinglePath(np string) error { | |||
| if strings.HasSuffix(np, "/") { | |||
| return fmt.Errorf(`%q cannot end with "/"`, np) | |||
| } | |||
| if strings.HasSuffix(np, "@") { | |||
There was a problem hiding this comment.
this needs a comment that in order to prevent emitting of dubious rules that look like @{,/,/**} and could be confusing to the parser we disallow trailing @. We prob. need to consult with the store to see if there are snaps using such path with system-files or personal-files interfaces.
There was a problem hiding this comment.
By parser do you mean the person reading this code later, or the AppArmor Parser itself?
I'll add a comment explaining though
There was a problem hiding this comment.
both 'entities' could be equally confused without some context :)
…ith @ Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
|
The store team is checking whether there are any current users of |
|
We need to follow up with https://warthogs.atlassian.net/browse/SN-3859 before the next release to ensure that this won't cause regressions, as it's possible (though unlikely) some snaps have registered paths with trailing |
|
Ahh nevermind, I see there was an update posted to https://warthogs.atlassian.net/browse/SN-3859 yesterday, and it's all clear. Thank you! |
For the
common-filesinterface and those which usecommonFilesInterface(such assystem-filesandpersonal-files), these append a trailing{,/,/**}to the end of each path.The
@character is allowed in filepaths specified in these plugs, since filepaths like/dev/foo@barmay be required. However, if an@character occurs at the end of the filepath (e.g./foo/bar@), then the resulting filepath in the rule is/foo/bar@{,/,/**}. AppArmor treats@{FOO}as a variable, so having@{,/,/**}in the final rule snippet is problematic.This commit adds a check to
common_files.goto ensure that filepaths do not have a trailing@.This addresses #14651 (comment)