-
Notifications
You must be signed in to change notification settings - Fork 575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
interfaces/many: deny arbitrary desktop files and misc from /usr/share #8301
Changes from 31 commits
486a3eb
105b47f
f1d87ff
6464c63
66bab80
d8879bd
6cfa4c8
3424667
bc49802
1af64c3
94d5ff2
99744e4
edee8f8
79095fc
59cc3fb
ff11e20
a320568
790ce3d
4448d3a
2ba55dc
2c5d293
55783d0
99005f5
58f8438
29ec3ac
272a48f
7846842
d008e8f
292f3f4
dfbdf7b
8d997c9
2a2212e
349de3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ import ( | |
"regexp" | ||
"sort" | ||
|
||
"github.com/snapcore/snapd/dirs" | ||
"github.com/snapcore/snapd/interfaces" | ||
"github.com/snapcore/snapd/release" | ||
"github.com/snapcore/snapd/snap" | ||
|
@@ -125,3 +126,58 @@ func verifySlotPathAttribute(slotRef *interfaces.SlotRef, attrs interfaces.Attre | |
} | ||
return cleanPath, nil | ||
} | ||
|
||
// aareExclusivePatterns takes a string and generates deny alternations. Eg, | ||
// aareExclusivePatterns("foo") returns: | ||
// []string{ | ||
// "[^f]*", | ||
// "f[^o]*", | ||
// "fo[^o]*", | ||
// } | ||
func aareExclusivePatterns(orig string) []string { | ||
// This function currently is only intended to be used with desktop | ||
// prefixes as calculated by info.DesktopPrefix (the snap name and | ||
// instance name, if present). To avoid having to worry about aare | ||
// special characters, etc, perform ValidateDesktopPrefix() and return | ||
// an empty list if invalid. If this function is modified for other | ||
// input, aare/quoting/etc will have to be considered. | ||
if !snap.ValidateDesktopPrefix(orig) { | ||
return make([]string, 0) | ||
jdstrand marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
s := make([]string, len(orig)) | ||
|
||
prefix := "" | ||
for i, letter := range orig { | ||
prefix = orig[:i] | ||
s[i] = fmt.Sprintf("%s[^%c]*", prefix, letter) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since apparmor globbing is not quite regex, are there any characters here we might need to escape such as "^" itself or is this all handled in the expected way by apparmor's regex/globs ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jdstrand is the authority here, but I suspect the problematic characters would be Given that the input to this function is validated to not contain the problem characters, I don't think it is worth trying to implement escaping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is precisely why I did not do this; we are using this with snapInstanceName only. I'll add a snap.ValidateInstanceName() and add some additional tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done (at first I did ValidateInstanceName() but based on other feedback, I'm not using a new ValidateDesktopPrefix()). |
||
} | ||
return s | ||
} | ||
|
||
// getDesktopFileRules(<snap instance name>) generates snippet rules for | ||
// allowing access to the specified snap's desktop files in | ||
// dirs.SnapDesktopFilesDir, but explicitly denies access to all other snaps' | ||
// desktop files since xdg libraries may try to read all the desktop files | ||
// in the dir, causing excessive noise. (LP: #1868051) | ||
func getDesktopFileRules(snapInstanceName string) []string { | ||
baseDir := dirs.SnapDesktopFilesDir | ||
|
||
rules := []string{ | ||
"# Support applications which use the unity messaging menu, xdg-mime, etc", | ||
"# This leaks the names of snaps with desktop files", | ||
fmt.Sprintf("%s/ r,", baseDir), | ||
"# Allowing reading only our desktop files (required by (at least) the unity", | ||
"# messaging menu).", | ||
"# parallel-installs: this leaks read access to desktop files owned by keyed", | ||
"# instances of @{SNAP_NAME} to @{SNAP_NAME} snap", | ||
fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,", baseDir), | ||
"# Explicitly deny access to other snap's desktop files", | ||
fmt.Sprintf("deny %s/@{SNAP_INSTANCE_DESKTOP}[^_.]*.desktop r,", baseDir), | ||
} | ||
for _, t := range aareExclusivePatterns(snapInstanceName) { | ||
mvo5 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
rules = append(rules, fmt.Sprintf("deny %s/%s r,", baseDir, t)) | ||
} | ||
|
||
return rules | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This access is removed but not added anywhere back AFAICT - is there a regression risk here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This path does not exist in core images (notice is is in /usr, which comes from core* snaps). This is explained in commit 105b47f: