mount-control: step 2

[mardy]

This is the second part of the mount-control saga: this adds the interface itself and a spread test.

The full (draft) MR is here, whereas the first part (which this branch depends on) is #10653.

[codecov-commenter]

Codecov Report

Merging #10739 (6f8fc34) into master (9ade0e2) will increase coverage by 0.11%.
The diff coverage is 98.79%.

@@            Coverage Diff             @@
##           master   #10739      +/-   ##
==========================================
+ Coverage   78.21%   78.32%   +0.11%     
==========================================
  Files         916      919       +3     
  Lines      103800   104430     +630     
==========================================
+ Hits        81187    81800     +613     
- Misses      17520    17533      +13     
- Partials     5093     5097       +4     

Flag	Coverage Δ
unittests	78.32% <98.79%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
interfaces/builtin/mount_control.go	98.13% <98.13%> (ø)
interfaces/utils/path_patterns.go	100.00% <100.00%> (ø)
osutil/synctree.go	76.41% <0.00%> (-2.84%)	⬇️
daemon/api_model.go	75.83% <0.00%> (-0.84%)	⬇️
daemon/api_connections.go	93.04% <0.00%> (-0.54%)	⬇️
overlord/snapstate/snapstate.go	83.00% <0.00%> (-0.24%)	⬇️
overlord/devicestate/devicemgr.go	78.14% <0.00%> (-0.07%)	⬇️
gadget/gadget.go	89.86% <0.00%> (ø)
asserts/database.go	81.17% <0.00%> (ø)
interfaces/udev/spec.go	91.07% <0.00%> (ø)
... and 26 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ade0e2...6f8fc34. Read the comment docs.

[pedronis]

did a first pass, some comments

[alexmurray]

It would be great to get @jrjohansen to give this function a once-over.

[jrjohansen]

how faithful does the conversion from aa globbing to re need to be? The conversion does not appear to be correct in some cases

[alexmurray]

There are a few places that worry me so I would like to see some more validation of various things like the filesystem types and some more tests added to try and make sure we can avoid snaps trying to abuse this interface.

[alexmurray]

Please add type validation (this could be similar to option validation where items are checked against an allow-list) so that we can ensure the generated apparmor profile cannot have malicious content inserted into it, ie if a type was specified as: auto) options=() path/to/malicious/content /var/lib/snapd/hostfs/...,\n mount fstype=( - then I think currently this could allow a snap to mount over parts of the root file-system or some other trusted location, or they could inject other apparmor policy bits as well to gain additional accesses.

[mardy]

I added a simple regular expression for alphanumeric characters, instead of a full list of possible options. This should address the security issue you raised, while remaining open to support any FS type. Please let me know, if you see a reason for an explicit list.

[alexmurray]

LGTM!

[pedronis]

thanks

the slot-side base declaration needs some changes.

some happy paths are not hit by unit tests

[pedronis]

thank you

[anonymouse64]

I haven't looked at the unit tests, but I have some thoughts :-)

let me know if you have any questions about my proposals, but the main problem I see right now is that we allow any filesystem type when we should be at least denying some specific filesystems that could be easily abused with this interface otherwise

[anonymouse64]

thanks for this unit test

[anonymouse64]

I'm a bit torn on this function, on the one hand, this interface is super-privileged so nothing will be uploadable to the store to be released without approval, but on the other hand, we've seen many instances of using system-files where a user declares something super wildly permissive and finds that it "just works" locally during development and goes to upload only to find that we actually don't encourage/allow/condone/permit that sort of usage of system-files, and many days I find myself wishing that whomever used system-files this way would have been warned during development that what they are doing is not encouraged or in many ways supported.

So on the one hand, I'm looking ahead and thinking of how people could abuse this when all they need to have to make this work locally is to have an absolute path that is clean, but on the other hand there may be legitimate use cases for mounting /sys/fs/kernel/give/me/all/your/secrets to /snap/foo/current/not-your-secrets.txt and we don't want to have to push out an update to snapd to allow this at the last minute like we have had to for other interfaces.

Also, I think system-files is uniquely more likely to be abused since it is about the single most frustrating aspect of apparmor confinement for snaps - arbitrary file access, whereas this interface is about arbitrary mount access which is not nearly as common of a requirement as arbitrary file access.

[anonymouse64]

as mentioned below, this is not enough, we should be disallowing certain kinds of filesystems from being mounted

[mardy]

OK. I've added one more check

[jrjohansen]

unless I am reading this wrong, character class within an alrenation is not handled correctly. eg {[,],} -> ([|]|) when it should be ([,]|)

[mardy]

It works correctly, it actually produces ^([,]|)$; I've added a unit test for this now.

[jrjohansen]

this doesn't looks wrong for the convertion when in a char class as well

[mardy]

Sorry, I don't understand what you mean here. Maybe the most practical approach is if you tell me some example patterns to test, and we see what this code produces for them?

[jrjohansen]

okay, so I have been through this 3 separate times now trying to see what I thought I was seeing before, and I don't see it. I think this is good

[alexmurray]

Hmm I wonder about fuse since this is done in userspace and hence a snap itself would have to provide this implementation so could this then be used to cause issues in the kernel or to other users of that file-system if it were malicious? Perhaps until we have a good use-case for snaps wanting to use mount-control with fuse we should remove that? Otherwise LGTM!

[mardy]

Hmm I wonder about fuse since this is done in userspace and hence a snap itself would have to provide this implementation so could this then be used to cause issues in the kernel or to other users of that file-system if it were malicious? Perhaps until we have a good use-case for snaps wanting to use mount-control with fuse we should remove that? Otherwise LGTM!

Makes sense. Removed!

[anonymouse64]

Unfortunately this needs more work around validating what/where and perhaps needs a fundamental rethinking about what sort of strings we want to accept in what/where.

[anonymouse64]

now that we have an allow list, it is sort of unnecessary to have the regexp too, but I think it's fine to leave in

[pedronis]

did another pass, some questions

[pedronis]

why /media ? I think in principle we want to allow / and control what is perimtted in terms of absolute targets in the snap-declaration?

[pedronis]

shouldn't we also prevent $ from appearing multiple times?

[anonymouse64]

@pedronis I assume by "allow / and ..." you mean to allow arbitrary absolute paths and not to allow mounting something at "/" exactly which would be very weird and break many things?

Assuming you meant absolute paths, maybe where should be something like:

	whereRegexp = regexp.MustCompile(`^(/[^\$"@]+|\$SNAP_DATA|\$SNAP_COMMON)[^\$"@]*$`)

which will disallow "/" or any other string starting with an env var (except $SNAP_DATA or $SNAP_COMMON), but otherwise allow anything underneath $SNAP_DATA or $SNAP_COMMON, as well as disallowing any other env vars in the string, any @ or any ". It also disallows $SNAP_DATA/$SNAP_DATA

[pedronis]

yes, I meant any absolute path (at this level)

[mardy]

The dollar sign is not dangerous (AppArmor wouldn't treat it specially), but it sounds so suspicious that I think it's fine to disallow it.

I would suggest using a slightly simpler regexp:

whereRegexp = regexp.MustCompile(`^(\$SNAP_COMMON|\$SNAP_DATA)?/[^\$"@]+$`)

The only practical difference from Ian's is that this disallows having just $SNAP_COMMON or $SNAP_DATA as the target, but requires $SNAP_DATA/something, which looks a reasonable restriction to me.

[anonymouse64]

I can see a use case behind where: $SNAP_DATA where a snap wants it's entire data dir to be located on some other partition than the main writable partition on it, but I suppose we could enable that use case when folks ask for it.

@pedronis what do you think of @mardy's proposal?

[pedronis]

I'm fine with the simplified proposal.

[anonymouse64]

overall looking really close, I have a few testing suggestions and some questions about the parsing code, also see the suggestion from @pedronis too

[anonymouse64]

Suggested change

	return fmt.Errorf(`mount-control "what" attribute is invalid: must start with / and not contain special characters`)
	return fmt.Errorf(`mount-control "what" attribute is invalid: must start with /, $SNAP_DATA, or $SNAP_COMMON and not contain special characters`)

though the message is getting rather long now

[mardy]

I'll accept the suggestion and remove the "is invalid:" bit to save some characters

[anonymouse64]

ok, but I don't see the suggestion accepted? the original error message I commented on is still in the diff (unless this is another github UI bug?)

[mardy]

Ah, it's because I changed it in a different way: I think you accidentally commented on this one, thinking that it was about the "where" attribute. But this error is on the "what" attribute, for which we don't have the $SNAP_DATA or $SNAP_COMMON restriction.

[anonymouse64]

whatever the result of the ^ and $ question I asked above is, we should have those covered in tests here either in the happy paths or in the unhappy paths

[mardy]

Added

[anonymouse64]

does it not work to define this in the environment section?

[mardy]

It does :-) Updated.

[anonymouse64]

you could also just have a check which exits the test if os.query is-opensuse-tumbleweed is true before doing these checks, it would make the nested if's here a bit more readable/less indented

[mardy]

Indeed; updated.

[anonymouse64]

can we also have negative cases for the options not matching and for the filesystem not matching?

[mardy]

Added!

[anonymouse64]

thanks for all the changes, I left a few suggestions, but we can take those in followups since this PR is essentially ready to go I think, let's land it :-)

[anonymouse64]

ok, but I don't see the suggestion accepted? the original error message I commented on is still in the diff (unless this is another github UI bug?)

[anonymouse64]

this error message shouldn't mention /media anymore

[mardy]

Oops! Fixed.

[anonymouse64]

if there isn't a usage of counting the number of mount entries in follow up PR's, I have a slight preference for this to be a boolean instead as it's ever so slightly more readable to do:

	hasMountEntries := false
	err := enumerateMounts(plug, func(mountInfo *MountInfo) error {
		hasMountEntries = true
		return validateMountInfo(mountInfo)
	})
	if err != nil {
		return err
	}

	if !hasMountEntries {
		return mountAttrTypeError
	}

[mardy]

IIRC it won't be used. Updated.

[anonymouse64]

since this is unexpected to happen, can you make this an internal error?

[mardy]

I'm not sure what an "internal error" is; I just added "internal error:" at the beginning of the message :-)

[anonymouse64]

that's exactly what I meant, just add "internal error" to the start of the error message 😄

[anonymouse64]

last commit 6f8fc3458bdcc15b37e3e74e88b0716cbd14ea77 looks good to me, but there is now a conflict

[mardy]

last commit 6f8fc34 looks good to me, but there is now a conflict

It's because of the merge of the kernel-module-load interface. I've now rebased the branch and squashed most commits, let's see how it goes :-)