Looks very ubuntu-related and heavily depends on strange directory/confinement
setups and apparmor.

Note that you execl() the udev helper script euid=uid=0 and
passing user environment to it. Only apparmor profile protects from immediate
rootshell by passing PATH= along.

Its also no good idea to have /tmp user owned, as some suids expect
root owned /tmp. (Not sure whether in the dozens of bind mounts, all
mounts are eventually nodev|nosuid).

Overall, I dont like the idea of namespace/mount confinements much,
since it changes underlying semantics (FS-access etc.) in a way
traditionally designed programs cannot expect. This will eventually
lead to some security issues.

(In reply to Sebastian Krahmer from comment #1)
> Looks very ubuntu-related and heavily depends on strange
> directory/confinement
> setups and apparmor.
> 
> Note that you execl() the udev helper script euid=uid=0 and
> passing user environment to it. Only apparmor profile protects from immediate
> rootshell by passing PATH= along.

Thanks I will raise this upstream to ensure is is not possible to exploit.
 
> Its also no good idea to have /tmp user owned, as some suids expect
> root owned /tmp. (Not sure whether in the dozens of bind mounts, all
> mounts are eventually nodev|nosuid).

I will raise this point as well

As for effective bind mounts, that is not something I can say but please keep in mind that snappy applications can run as the user (foreground applications) or as root (all background applications). Therefore nosuid is not as important since application developers can readily run their code as root if required.

As for nodev, I will raise the question upstream.
 
> Overall, I dont like the idea of namespace/mount confinements much,
> since it changes underlying semantics (FS-access etc.) in a way
> traditionally designed programs cannot expect. This will eventually
> lead to some security issues.

The unorthodox filesystem layout is part of the snappy system. There are specific locations where snaps are expected to write to or read from (all depending on runtime environment variables such as $SNAP, $SNAP_DATA, $SNAP_USER_DATA, etc).

It is true that ultimately applications need to be integrated with snappy concepts, either by using common libraries which contain the knowledge already or by selective patching or using specific configuration files to override the defaults.

Thank you for the review

Now in:
https://build.opensuse.org/project/show/system:snappy
https://build.opensuse.org/package/show/system:snappy/snapd

There are various issues which need to be solved in snap-confine,
before it could be whitelisted as suid:

1. There seems to be no binding between the seccomp profile that
   is loaded and the binary name. E.g. users could attach wrong profiles
   to programs, in the worst case subverting execution flow of critical
   programs.

2. Users can create files in root owned directories:
   SNAP_NAME=../../../../etc/XYZ snap-confine snap.this.profile /usr/bin/id
   so the SNAP_NAME needs to be sanitized. It also affects later mounts,
   which could end up on real / or so.

3. All the bind-mount magic has to add the NODEV|NOSUID flags, so users
   cant do dirty tricks with suid files. (Some mounts maybe even RDONLY)

4. Theres a logic quirk in permanently dropping the uid before
   calling execve(). The 

   if (geteuid() == 0) {

   is never true due to the seteuid(real_uid) call above. So the saved
   setuser id is never dropped. I think the setuid(real_uid) should
   be called in any case, otherwise the comment "Permanently drop if not root"
   is wrong.

5. Before execve(), the NO_NEW_PRIVS prctl should be set. I dont understand
   the comment about disabling thats made about it in the code.
   You dont want users to execute /bin/su in artificially setup / with
   their own libs etc.

6. When I understand correctly, snap-confine mounts a lot of
   directories (like /home and /etc) into the confinement in "classic mode".
   Thats not really a confinement and should not be used for untrusted
   apps.

(In reply to Sebastian Krahmer from comment #6)
> There are various issues which need to be solved in snap-confine,
> before it could be whitelisted as suid:
> 
> 1. There seems to be no binding between the seccomp profile that
>    is loaded and the binary name. E.g. users could attach wrong profiles
>    to programs, in the worst case subverting execution flow of critical
>    programs.

This is true, I will look at how this can be more tightly controlled. The key thing is that seccomp/appamor are loaded based on the security tag. In non-test cases we always execute snap-exec that uses the same information to load and execute the desired application. I think we could figure out how to streamline this so that there's no chance to corrupt some of the data to execute the wrong thing.

I also think that in non-test mode we could always run snap-exec as that would just reduce the attack surface significantly.
 
> 2. Users can create files in root owned directories:
>    SNAP_NAME=../../../../etc/XYZ snap-confine snap.this.profile /usr/bin/id
>    so the SNAP_NAME needs to be sanitized. It also affects later mounts,
>    which could end up on real / or so.

This was known recently and is already in progress towards being fixed in master. It will be a part of the next release.
 
> 3. All the bind-mount magic has to add the NODEV|NOSUID flags, so users
>    cant do dirty tricks with suid files. (Some mounts maybe even RDONLY)

Some of those are legitimate as we rely on apparmor to mediate access. Still you are right that we don't need dev/suid everywhere. I will do a pass that limits this.
 
> 4. Theres a logic quirk in permanently dropping the uid before
>    calling execve(). The 
> 
>    if (geteuid() == 0) {
> 
>    is never true due to the seteuid(real_uid) call above. So the saved
>    setuser id is never dropped. I think the setuid(real_uid) should
>    be called in any case, otherwise the comment "Permanently drop if not
> root"
>    is wrong.

Thanks, I'll investigate this.

> 5. Before execve(), the NO_NEW_PRIVS prctl should be set. I dont understand
>    the comment about disabling thats made about it in the code.
>    You dont want users to execute /bin/su in artificially setup / with
>    their own libs etc.

I'll investigate but I suspect we may in some cases need new privs. Not sure, I'll talk to the ubuntu security team about this.
 
> 6. When I understand correctly, snap-confine mounts a lot of
>    directories (like /home and /etc) into the confinement in "classic mode".
>    Thats not really a confinement and should not be used for untrusted
>    apps.

In "classic distribution" mode (this is confusing as classic has two meanings) we do bind mount /home and /etc and rely on apparmor to control access. Since on SUSE we cannot yet use apparmor everything runs in so-called "demoed" (without confinement). This is by design, on any devmode system the confinement is not enforcing and cannot be used for untrusted apps.