Merge lp://qastaging/~kyrofa/snap-confine/create_user_data_directory into lp://qastaging/~snappy-dev/snap-confine/trunk

Does this code run with more privileges than the services that it is working for?

If so, does something prevent the services from running at the same time?

If so, does something fix the directories' owner and group later?

Thanks

Thanks for taking a look, Seth.

> Does this code run with more privileges than the services that it is working for?

Yes, the launcher runs with setuid root.

> If so, does something prevent the services from running at the same time?

No, nothing prevents services (or binaries) from running at the same time, but from what I've read mkdir() is atomic. Perhaps I misunderstood your concern here?

> If so, does something fix the directories' owner and group later?

Ah, great catch, I need to do that. Services would have been fine, but as-is this will create directories owned by root for binaries, which isn't what we want here. There are two ways I see to do that: the chown() syscall after the fact, or fork and drop privs before creating the directories. Normally I'd say the latter would be more secure so as to mitigate race condition attacks, but since we'd only ever be dropping privilege levels with the chown() (from root to user, never the other way around), I wonder if it's really a concern. Thoughts?

I've fixed that with a call to chown() as you can see in the most recent push. Let me know if there's a concern with it.

The trouble is that while one mkdir() call is atomic, a series of them is not; and mkdir() will happily follow symbolic links in the path.

I think there's a few approaches that would work:

- perform the chown() operations in reverse order, so the snap owner never owns a directory that's being operated on
- perform all the mkdir operations with the effective uid of the snap owner, so they only ever have their permissions anyway
- use mkdirat(2) instead of mkdir(2) to ensure that the directories are being created in the proper parent directory regardless of rename() or symlink() or mount(.., MS_BIND) tricks

There's pros and cons to each of these; the mkdirat() version may still require the reverse-order chown() calls too.

Thanks

Hi Kyle - The current MP isn't acceptable because it would allow an unprivileged user to create a path, owned by that user, anywhere in the filesystem by doing something like `SNAP_USER_DATA=/create/path/to/anywhere ubuntu-core-launcher ...`, where ... are valid arguments to ubuntu-core-launcher.

Does setup_user_data() have to be called before dropping user privileges?

Oh my, what a glaring error that was!

Of course you're right Tyler-- the directory creation needs to happen before the apparmor/seccomp confinement, but should definitely happen after the privileges are dropped. My latest push reflects that.

Seth, I believe that actually addresses your concern as well, yes? Since now all mkdir operations are performed with the effective uid of whoever launched it?

Note on the previous push I also updated the indentation to be consistent in the main() function. I'm sorry it makes for more to review, but it made it really difficult to conform when there was no conformity.

Thanks Kyle - This looks pretty good now but there's still one issue. Jamie tells me that snaps were granted control of the paths specified in SNAP_USER_DATA and SNAP_APP_USER_DATA_PATH, as well as SNAP_USER_DATA/../ and SNAP_APP_USER_DATA_PATH/../, because of the bug that you're trying to fix here. This change would happily follow symlinks that snaps had created in those locations since there's nothing to prevent the following of symlinks.

I'd suggest modifying the mkdir() loop to use openat() w/ O_NOFOLLOW and mkdirat(), as Seth originally suggested.

We'll need to update ubuntu-core-security, at the same time or after this change lands in the image, to remove access to those directories.

Yes, those white space changes sure make it more difficult to review! :)

Alright Tyler, I've refactored the mkpath function to use open/openat/mkdirat with O_NOFOLLOW instead of mkdir. It complicated it a bit, but I think it's still readable. I also refactored the tests to be more readable.

Thanks for making those changes, Kyle. It all looks good to me now.

This MP contains whitespace changes that make it difficult to review and is inconsistent with the current codebase. For example, it seems you went from 4 spaces to 3 spaces in main(). Can you update the MP to remove these whitespace changes?

Once that is done, I will sponsor the upload.

Ok, on closer inspection I see that these whitespace changes are sorely needed. Looking at diff -Naur -wb, I see the non-whitespace changes and they are clear and fine. Thank you for the patch and for adding O_CLOEXEC.

And for completeness, I've queued the ubuntu-core-security changes in http://bazaar.launchpad.net/~ubuntu-security/ubuntu-core-security/trunk/revision/193