cmd/snap-{confine,update-ns}: apply mount profiles using snap-update-ns #3621

zyga · Jul 25, 2017

This branch teaches snap-confine how to use snap-update-ns to apply the mount profile.

Over time I'd like to move more responsibility into fstab profiles and thus allow us to implement
layouts entirely through mount profiles. This brings us closer by allowing us to mutate only
one implementation. For now I kept the old implementation intact because I want to remove it
along with a cleanup of the appamor profile, which is non-trivial.

Because of the additional security impact, snap-update-ns has undergone a retroactive security
review and has been hardened. It now executes under a child apparmor profile (when started
by snap-confine) and has some modifications for better testability and integrity.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

codecov-io · Jul 26, 2017

Codecov Report

Merging #3621 into master will decrease coverage by <.01%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##           master    #3621      +/-   ##
==========================================
- Coverage   75.88%   75.88%   -0.01%     
==========================================
  Files         417      417              
  Lines       36194    36219      +25     
==========================================
+ Hits        27466    27483      +17     
- Misses       6802     6811       +9     
+ Partials     1926     1925       -1

Impacted Files	Coverage Δ
cmd/snap-update-ns/main.go	`0% <0%> (ø)`	⬆️
cmd/snap-update-ns/bootstrap.go	`86.04% <100%> (+16.04%)`	⬆️
snap/validate.go	`95.75% <100%> (+0.05%)`	⬆️
cmd/snap/cmd_aliases.go	`93.33% <0%> (-1.67%)`	⬇️

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 33ff99d...c32a9fe. Read the comment docs.

zyga · Aug 21, 2017

zyga reviewed Aug 21, 2017

View changes

Comments from live chat.

Let's keep an fd open (to snap-update-ns) and exec it this way. Also use /proc/self/exe to find the tool before.

Skip -option like arguments.

stolowski · Aug 29, 2017

stolowski approved these changes Aug 29, 2017

View changes

LGTM, just one question below.

I'm not clear about syntax of apparmor profiles here: it's clear /{var/lib/snapd/hostfs/,} is an obsolute path, but how about remaining paths?

This rule is not clear. Can you enumerate the paths? Maybe we could then split the rule into two rules.

This rule is a composition of the following possibilities:

/usr/lib/snapd/snap-update-ns
/usr/libexec/snapd/snap-update-ns
same as above but when using core snap mounted at /snap/core/*/
same as above but when using core snap mounted at /var/lib/snapd/snap/core/*
same as above but when all of those are visible through hostfs at /var/lib/snapd/hostfs

From IRC, I think we should break this up a little to make it easier to audit:

// Allow executing snap-update-ns when:
// not re-execing
/usr/lib{,exec}/snapd/snap-update-ns Ux,
// when re-execing
/snap/core/*/usr/lib{,exec}/snapd/snap-update-ns Ux,
// when ???
/var/lib/snapd/{hostfs,snap/core/*}/usr/lib{,exec}/snapd-update-ns Ux,

Note, if within the core snap, do we ever need lib{,exec}?

I'll do something like this, the ??? case is actually always used in practice because when apparmor recovers the path for snap-update-ns this will be the path it chooses to use (/var/lib/snapd/hostfs/snap/core/*/usr/lib/snapd/snap-update). Seeing this makes me want to start to consider unmounting more hostfs things that we know are already present in the filesystem otherwise (like the whole /snap hierarchy).

@stolowski all of the paths are absolute as the rule starts with /

Apparmor rules are just regular expressions coupled with some flags. Here the regular expression is using {a,b,c} as shell-like alternation syntax.

Note they aren't regular expressions. They are AARE which is much more limited (see man apparmor.d for details).

jdstrand · Aug 30, 2017

jdstrand requested changes Aug 30, 2017

View changes

I ran out of time before I could finish the PR, so commenting on what I have. What is left is bootstrap.c, bootstrap_exec.go and main.go.

This change seems unrelated. It seems fine though.

Yes, this just fixes make hack.

Can you add a comment about default_value? It wasn't clear why you added it since str2bool only assigns to &value, but you are doing this to return true when something is not set. I think default_value is a bit of a misnomer here in terms of the API and should probably be named inverted, but inverted doesn't really go with the implementation. Maybe it would be better to not have 'default_value' at all and instead use:

bool sc_is_reexec_enabled() {
    return !getenv_bool("SNAP_REEXEC");
}

Ah, nice catch wrt default_value and how str2bool operates, I missed that. Let me tweak that.

I reworked the default value, fixed the bug, added testes and documented how it operates.

A comment here talking about snap_update_ns_fd and what snap-update-ns will do on our behalf would be great here.

Ack, will add

Done

Why are you setting this unconditionally rather than setting this conditional on getenv_bool()?

Ouch, that's a debug-leftover :)

Done

This debug statement is, perhaps, not as clear as it could be. Perhaps this instead:

debug("fexecv(%d (snap-update-ns), %s %s %s,)", snap_update_ns_fd, argv[0], argv[1], argv[2]);

Done

No need for an else here. fexecve() either never returns or if it does with error, you call die().

Hmm? This else is for the parent process.

Yes. Because the parent won't enter the child's 'if' branch and the child never leaves the 'if' branch, you don't need the else.

Ah, I see your point now. Changed

die("waitpid() failed for snap-update-ns process"); maybe?

Done

This is simpler:

if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
    die("snap-update-ns failed with code %d", WEXITSTATUS(status));
}

Also, while equivalent for our usage, we are using '%i' instead of '%d' in udev-support.c for printing the status. Not a blocker.

Done, also the one below.

Instead of this, I suggest:

} else if (WIFSIGNALED(status)) {
    die("child died with signal %i", WTERMSIG(status));
}

This is sufficient with the options we pass to waitpid() (man waitpid).

Right, I was wondering if we should be this precise. Done now.

Please account for terminating null in case length of /proc/self/exe is PATH_MAX:

char buf[PATH_MAX+1] = { 0 };

Done. I'm worried now, should all path handling code use PATH_MAX + 1 buffers?

It depends on the api used. readlink() doesn't add a trailing '\0' so we need to account for it using all of buf, and then we come along and tack on '\0'. Because we are using sc_must_snprintf() down below, what you had wasn't a security issue-- sc_must_sprintf() would've die()d-- but this way in the off chance of a super long symlink that is exactly PATH_MAX, it won't die() unnecessarily.

Please add a comment stating:

// readlink doesn't add terminating null, but our initialization of
// buf handles this for us.

Done

Perhaps:

s += 1; // advance pointer to basename of exe

Done

This is unclear and an abuse of the API since you aren't giving a format string. I think something like this would be clearer:

#include <libgen.h>
...
	if (readlink("/proc/self/exe", buf, sizeof buf) < 0) {
		die("cannot readlink /proc/self/exe");
	}
	if (buf[0] != '/') { // this shouldn't happen, but make sure have absolute path
		die("readlink not an absolute path");
	}
	char *bufcopy __attribute__ ((cleanup(sc_cleanup_string))) = NULL;
	bufcopy = strdup(buf);
	char *dname = dirname(bufcopy);
	sc_must_snprintf(buf, sizeof buf, "%s/%s", dname, "snap-update-ns");

Woah, I didn't mean to pass s as the format string. I just wanted to strcpy at s with the length limit intact. I'll rework this.

Done, with slight tweaks.

Perhaps rephrase as:

// Find and open snap-update-ns from the same filesystem as where we (snap-confine)
// were called.

Applied.

I'm not clear about syntax of apparmor profiles here: it's clear /{var/lib/snapd/hostfs/,} is an obsolute path, but how about remaining paths?

This rule is not clear. Can you enumerate the paths? Maybe we could then split the rule into two rules.

This rule is a composition of the following possibilities:

/usr/lib/snapd/snap-update-ns
/usr/libexec/snapd/snap-update-ns
same as above but when using core snap mounted at /snap/core/*/
same as above but when using core snap mounted at /var/lib/snapd/snap/core/*
same as above but when all of those are visible through hostfs at /var/lib/snapd/hostfs

From IRC, I think we should break this up a little to make it easier to audit:

// Allow executing snap-update-ns when:
// not re-execing
/usr/lib{,exec}/snapd/snap-update-ns Ux,
// when re-execing
/snap/core/*/usr/lib{,exec}/snapd/snap-update-ns Ux,
// when ???
/var/lib/snapd/{hostfs,snap/core/*}/usr/lib{,exec}/snapd-update-ns Ux,

Note, if within the core snap, do we ever need lib{,exec}?

I'll do something like this, the ??? case is actually always used in practice because when apparmor recovers the path for snap-update-ns this will be the path it chooses to use (/var/lib/snapd/hostfs/snap/core/*/usr/lib/snapd/snap-update). Seeing this makes me want to start to consider unmounting more hostfs things that we know are already present in the filesystem otherwise (like the whole /snap hierarchy).

@stolowski all of the paths are absolute as the rule starts with /

Apparmor rules are just regular expressions coupled with some flags. Here the regular expression is using {a,b,c} as shell-like alternation syntax.

Note they aren't regular expressions. They are AARE which is much more limited (see man apparmor.d for details).

Why is this Ux? I thought we talked about this, but I thought we were going transition to another profile.... This could be done conveniently with a child profile without using libapparmor in snap-confine.

This is Ux because we don't have any profile to transition to yet. Generation of snap-specific profiles for snap-update-ns is further down the line.

Note that this is not any less safe than before:

snap-confine no longer performs those mounts
snap-confine runs unconfined snap-update-ns
(which always ran unconfined when invoked from snapd)
snap-update-ns is not written in C, thus harder to attack and reads only files written by root (snapd)

I disagree about 'not any less safe'. Because it is written in Go it potentially isn't as big of an attack surface but because snap-confine is setuid, it calls snap-update-ns with elevated privileges and therefore the user is able to call snap-update-ns with elevated privileges through snap-confine. Coding errors could therefore allow unintended access. The snapd calling snap-update-ns is different: snapd is not called by the user and it is invoked in a safe environment.

Today we have most of the rules in snap-confine already since snap-confine and snap-update-ns overlapped. As part of this PR, please add confinement for snap-update-ns. I know long term you have ideas about per-snap profiles, but we can at least not regress on confinement by doing something simple like:

/usr/lib/snapd/snap-confine (attach_disconnected) {
    ... remove mount rules no longer needed by snap-confine ...
    ...
    // Allow executing snap-update-ns when:
    // not re-execing
    /usr/lib{,exec}/snapd/snap-update-ns Cxr -> snap_update_ns,
    // when re-execing
    /snap/core/*/usr/lib{,exec}/snapd/snap-update-ns Cxr -> snap_update_ns,
    // when ???
    /var/lib/snapd/{hostfs,snap/core/*}/usr/lib{,exec}/snapd-update-ns Cxr -> snap_update_ns,
    ...
    profile snap_update_ns (attach_disconnected) {
        ...
        <mount rules previously in snap-confine>
        ...
    }

With this, you need no code changes to your PR. The profiling for the child profile will also inform your future work's rules, so it isn't wasted.

"because snap-confine is setuid, it calls snap-update-ns with elevated privileges and therefore the user is able to call snap-update-ns with elevated privileges through snap-confine. Coding errors could therefore allow unintended access. The snapd calling snap-update-ns is different: snapd is not called by the user and it is invoked in a safe environment."

This reminds me of an important comment in my response in the forum that was not addressed:

"I do have concerns though that we are changing the assumptions of snap-update-ns; specifically that it was written with the understanding that it would be launched under snapd which is not an attack vector for privilege escalation. Now it would not only be launched from snapd but also from a setuid executable and now therefore be an attack vector for those trying to escalate privileges via implementation bugs in snap-confine/snap-update-ns. Put another way, while this makes snap-confine smaller, the attack surface is not actually reduced (it is even bigger considering snap-update-ns' imports (we tried very hard to limit the number of libraries snap-confine uses (best practice for writing setuid applications)). Because snap-update-ns will be running in the context of a setuid application, the lines within the codebase between what is running setuid becomes muddy (eg, today it is pretty easy to know when to ask the security team for reviews: "is the code in cmd/snap-confine changed?". This becomes difficult and easy to miss when to ask for security reviews-- eg, snap-update-ns imports "github.com/snapcore/snapd/logger" which all of a sudden will now at times run under setuid-- who will remember to ask the security team to do a review of changes to logger?). Furthermore, snap-update-ns and everything it imports will now need a proper security review. With the rapid pace of development of the go code within snappy, it is easy to imagine introducing security flaws....
"

I'll put it more simply. Here are the imports for things not part of the standard library:

cmd/snap-update-ns/main.go imports:

"github.com/jessevdk/go-flags"
"github.com/snapcore/snapd/dirs" which imports:
- "github.com/snapcore/snapd/release" which imports:
  - "github.com/snapcore/snapd/apparmor"
"github.com/snapcore/snapd/interfaces/mount" which imports:
- "github.com/snapcore/snapd/dirs" (via backend.go, lock.go, ns.go)
- "github.com/snapcore/snapd/interfaces" (via backend.go, spec.go) which imports:
  - "github.com/snapcore/snapd/snap" (via backend.go, core.go, naming.go, repo.go, sorting.go) which imports:
    - "github.com/snapcore/snapd/dirs" (via broken.go, info.go)
    - "github.com/snapcore/snapd/osutil" (via container.go, implicit.go, seed_yaml.go)
    - "github.com/snapcore/snapd/snap/snapdir" (via container.go)
    - "github.com/snapcore/snapd/snap/squashfs" (via container.go) which imports:
      - "github.com/snapcore/snapd/osutil"
    - "gopkg.in/yaml.v2" (via gadget.go, info_snap_yaml.go)
    - "github.com/snapcore/snapd/strutil" (via info.go, info_snap_yaml.go) which imports:
      - "gopkg.in/yaml.v2" (via map.go)
    - "github.com/snapcore/snapd/timeout" (via info.go, info_snap_yaml.go)
- "github.com/snapcore/snapd/osutil" (via backend.go, lock.go, ns.go, profile.go) which imports:
  - "github.com/snapcore/snapd/dirs" (via exec.go)
  - "github.com/snapcore/snapd/strutil" (via io.go)
- "github.com/snapcore/snapd/snap" (via backend.go)
- "github.com/snapcore/snapd/cmd" (via ns.go) which imports:
  - "github.com/snapcore/snapd/dirs"
  - "github.com/snapcore/snapd/logger" which imports:
    - "github.com/snapcore/snapd/osutil"
  - "github.com/snapcore/snapd/osutil"
  - "github.com/snapcore/snapd/release"
  - "github.com/snapcore/snapd/strutil"
"github.com/snapcore/snapd/logger"
"github.com/snapcore/snapd/snap"

That's a lot of code and I bet none of it was written with setuidness in mind. Obviously all of it isn't being used by snap-update-ns, but it is all in the address space and therefore available with certain types of memory attacks (this is why in the snap-confine profile we limit the libraries (what is in the address space) to load instead of simply allowing /usr/lib/*.so). I don't have time (today) to verify each API snap-update-ns uses and verify that all the code paths are safe under setuid, which perfectly illustrates my point: snap-update-ns is using too many external libs for a setuid application and even if we did audit them all, these imported packages are getting updates all the time. setuid applications should be small and should not change much because the changes might change assumptions in other parts of the code (we finally got to the point where we aren't modifying snap-confine weekly-- a good thing!). AppArmor confinement mitigates that to some degree (the snap-confine profile has a good bit of privilege as it is), but on systems without AppArmor, this is a pretty big attack surface.

I know the feature is important and I think we can move forward if we:

cut down on the imports in snap-update-ns (this might involve shuffling some things into cmd/snap-update-ns for other code to import from there rather than having snap-update-ns import from elsewhere)
adding a comment to the import list of snap-update-ns stating that we are intentionally keeping it small for security reasons
getting security team involvement in cmd/snap-update-ns PRs. This does not necessarily require deep reviews from the security team (ie, the security team is at liberty to perform a shallow review stating a deep review is not required)

@zyga, can you comment on this?

This is an interesting idea. So long as that API is very controlled and does extensive input validation, this would work fine. It will likely add some overhead over a fork/exec that would need to be considered.

FYI, here is the exploratory PR for reducing the imports in snap-update-ns: #3840

@niemeyer and @zyga: I've thought through the security implications of snap-confine fork/execing snap-update-ns with minimized imports (eg, finishing #3840) vs @zyga's idea of having snap-confine ask snapd to call snap-update-ns on its behalf.

In terms of security, I don't think there is an appreciable difference with either approach (assuming we get things right, which I'll of course help with).

Outside of security, the fork/exec is simpler to understand and less overhead, but requires the security team have some involvement in changes to snap-update-ns. The IPC approach allows snap-update-ns to be updated more easily (eg, without mandatory security team involvement) since it will always be invoked from a safe environment, but it does mean snapd must be running for anything to execute, which might affect snaps starting on boot before snapd, launching while snapd is restarting, etc.

I'll leave the decision to which approach to pursue up to the snapd team. If you still want to discuss in a meeting with me, I'm available.

After discussing with @niemeyer and @zyga, there is another option which should mitigate my concerns.

If we consider the situation where there is a bug in snap-confine that allows the unprivileged user to call the snap-update-ns executable as root in arbitrary ways, since snap-confine is calling snap-update-ns via its command line interface, the extent of direct influence over snap-update-ns is limited to what the snap-update-ns cli exposes. Beyond that there is the matter of how snap-confine can influence snap-update-ns indirectly which leaves the environment and the mount profiles. With that in mind, the plan of action is:

adjust snap-update-ns to unconditionally and immediately clear its environment upon its invocation (optionally adding back whitelisted variables in the future if required). It shall die() on error. There shall be comments in this part of the code stating that changes need a security review.
snap-update-ns cli argument parsing shall be hardened for processing malicious input and reviewed by the security team. There shall be comments in this part of the code stating that changes need a security review.
write a restrictive child AppArmor profile for snap-update-ns and allow snap-confine to call snap-update-ns only under that profile and with env scrubbing (ie, 'Cx')
ensure that the snap-confine AppArmor profile denies writes to mount profiles (this should already be handled)
ensure snap-confine is closing unneeded file descriptors before invoking snap-update-ns (ie use O_CLOEXEC for anything that isn't being passed)
in addition to snap-confine changes, the security team shall going forward audit changes to the snap-confine and snap-update-ns AppArmor profiles, changes to snap-update-ns environment cleaning (and future whitelisting) and snap-update-ns command line parsing code

On an AppArmor enabled system where snap-confine is confined, the above mitigates the concerns I have since snap-update-ns will be hardened and security-reviewed on its inputs (cli and environment) and snap-confine will be coded to not leak file descriptors and be confined to not allow updating the mount profiles.

On a system without AppArmor, a user-controlled snap-confine is in a position to modify the mount profiles to trigger bugs in the snap-update-ns code. This is true today however with a delayed attack: if snap-confine can change the mount profiles, snapd will happily at some future point call snap-update-ns on them. Obviously we desire to have robust mount profile parsing code, but since it is more likely that a malicious user exploiting bugs in snap-confine would manipulate mount points directly to carry out attacks (or modify the snap-update-ns binary or shell out if the exploit allowed it or ...), we don't need to put extra security reviews on mount profile parsing code.

In addition to the security reviews, I've agreed to implementing the apparmor profile changes for snap-confine and snap-update-ns.

zyga · Aug 31, 2017

zyga reviewed Aug 31, 2017

View changes

I'm working on addressing all the feedback from this.

No need for an else here. fexecve() either never returns or if it does with error, you call die().

Hmm? This else is for the parent process.

Yes. Because the parent won't enter the child's 'if' branch and the child never leaves the 'if' branch, you don't need the else.

Ah, I see your point now. Changed

Perhaps:

s += 1; // advance pointer to basename of exe

Done

This is unclear and an abuse of the API since you aren't giving a format string. I think something like this would be clearer:

#include <libgen.h>
...
	if (readlink("/proc/self/exe", buf, sizeof buf) < 0) {
		die("cannot readlink /proc/self/exe");
	}
	if (buf[0] != '/') { // this shouldn't happen, but make sure have absolute path
		die("readlink not an absolute path");
	}
	char *bufcopy __attribute__ ((cleanup(sc_cleanup_string))) = NULL;
	bufcopy = strdup(buf);
	char *dname = dirname(bufcopy);
	sc_must_snprintf(buf, sizeof buf, "%s/%s", dname, "snap-update-ns");

Woah, I didn't mean to pass s as the format string. I just wanted to strcpy at s with the length limit intact. I'll rework this.

Done, with slight tweaks.

I'm not clear about syntax of apparmor profiles here: it's clear /{var/lib/snapd/hostfs/,} is an obsolute path, but how about remaining paths?

This rule is not clear. Can you enumerate the paths? Maybe we could then split the rule into two rules.

This rule is a composition of the following possibilities:

/usr/lib/snapd/snap-update-ns
/usr/libexec/snapd/snap-update-ns
same as above but when using core snap mounted at /snap/core/*/
same as above but when using core snap mounted at /var/lib/snapd/snap/core/*
same as above but when all of those are visible through hostfs at /var/lib/snapd/hostfs

From IRC, I think we should break this up a little to make it easier to audit:

// Allow executing snap-update-ns when:
// not re-execing
/usr/lib{,exec}/snapd/snap-update-ns Ux,
// when re-execing
/snap/core/*/usr/lib{,exec}/snapd/snap-update-ns Ux,
// when ???
/var/lib/snapd/{hostfs,snap/core/*}/usr/lib{,exec}/snapd-update-ns Ux,

Note, if within the core snap, do we ever need lib{,exec}?

I'll do something like this, the ??? case is actually always used in practice because when apparmor recovers the path for snap-update-ns this will be the path it chooses to use (/var/lib/snapd/hostfs/snap/core/*/usr/lib/snapd/snap-update). Seeing this makes me want to start to consider unmounting more hostfs things that we know are already present in the filesystem otherwise (like the whole /snap hierarchy).

@stolowski all of the paths are absolute as the rule starts with /

Apparmor rules are just regular expressions coupled with some flags. Here the regular expression is using {a,b,c} as shell-like alternation syntax.

Note they aren't regular expressions. They are AARE which is much more limited (see man apparmor.d for details).

Why is this Ux? I thought we talked about this, but I thought we were going transition to another profile.... This could be done conveniently with a child profile without using libapparmor in snap-confine.

This is Ux because we don't have any profile to transition to yet. Generation of snap-specific profiles for snap-update-ns is further down the line.

Note that this is not any less safe than before:

snap-confine no longer performs those mounts
snap-confine runs unconfined snap-update-ns
(which always ran unconfined when invoked from snapd)
snap-update-ns is not written in C, thus harder to attack and reads only files written by root (snapd)

I disagree about 'not any less safe'. Because it is written in Go it potentially isn't as big of an attack surface but because snap-confine is setuid, it calls snap-update-ns with elevated privileges and therefore the user is able to call snap-update-ns with elevated privileges through snap-confine. Coding errors could therefore allow unintended access. The snapd calling snap-update-ns is different: snapd is not called by the user and it is invoked in a safe environment.

Today we have most of the rules in snap-confine already since snap-confine and snap-update-ns overlapped. As part of this PR, please add confinement for snap-update-ns. I know long term you have ideas about per-snap profiles, but we can at least not regress on confinement by doing something simple like:

/usr/lib/snapd/snap-confine (attach_disconnected) {
    ... remove mount rules no longer needed by snap-confine ...
    ...
    // Allow executing snap-update-ns when:
    // not re-execing
    /usr/lib{,exec}/snapd/snap-update-ns Cxr -> snap_update_ns,
    // when re-execing
    /snap/core/*/usr/lib{,exec}/snapd/snap-update-ns Cxr -> snap_update_ns,
    // when ???
    /var/lib/snapd/{hostfs,snap/core/*}/usr/lib{,exec}/snapd-update-ns Cxr -> snap_update_ns,
    ...
    profile snap_update_ns (attach_disconnected) {
        ...
        <mount rules previously in snap-confine>
        ...
    }

With this, you need no code changes to your PR. The profiling for the child profile will also inform your future work's rules, so it isn't wasted.

"because snap-confine is setuid, it calls snap-update-ns with elevated privileges and therefore the user is able to call snap-update-ns with elevated privileges through snap-confine. Coding errors could therefore allow unintended access. The snapd calling snap-update-ns is different: snapd is not called by the user and it is invoked in a safe environment."

This reminds me of an important comment in my response in the forum that was not addressed:

"I do have concerns though that we are changing the assumptions of snap-update-ns; specifically that it was written with the understanding that it would be launched under snapd which is not an attack vector for privilege escalation. Now it would not only be launched from snapd but also from a setuid executable and now therefore be an attack vector for those trying to escalate privileges via implementation bugs in snap-confine/snap-update-ns. Put another way, while this makes snap-confine smaller, the attack surface is not actually reduced (it is even bigger considering snap-update-ns' imports (we tried very hard to limit the number of libraries snap-confine uses (best practice for writing setuid applications)). Because snap-update-ns will be running in the context of a setuid application, the lines within the codebase between what is running setuid becomes muddy (eg, today it is pretty easy to know when to ask the security team for reviews: "is the code in cmd/snap-confine changed?". This becomes difficult and easy to miss when to ask for security reviews-- eg, snap-update-ns imports "github.com/snapcore/snapd/logger" which all of a sudden will now at times run under setuid-- who will remember to ask the security team to do a review of changes to logger?). Furthermore, snap-update-ns and everything it imports will now need a proper security review. With the rapid pace of development of the go code within snappy, it is easy to imagine introducing security flaws....
"

I'll put it more simply. Here are the imports for things not part of the standard library:

cmd/snap-update-ns/main.go imports:

"github.com/jessevdk/go-flags"
"github.com/snapcore/snapd/dirs" which imports:
- "github.com/snapcore/snapd/release" which imports:
  - "github.com/snapcore/snapd/apparmor"
"github.com/snapcore/snapd/interfaces/mount" which imports:
- "github.com/snapcore/snapd/dirs" (via backend.go, lock.go, ns.go)
- "github.com/snapcore/snapd/interfaces" (via backend.go, spec.go) which imports:
  - "github.com/snapcore/snapd/snap" (via backend.go, core.go, naming.go, repo.go, sorting.go) which imports:
    - "github.com/snapcore/snapd/dirs" (via broken.go, info.go)
    - "github.com/snapcore/snapd/osutil" (via container.go, implicit.go, seed_yaml.go)
    - "github.com/snapcore/snapd/snap/snapdir" (via container.go)
    - "github.com/snapcore/snapd/snap/squashfs" (via container.go) which imports:
      - "github.com/snapcore/snapd/osutil"
    - "gopkg.in/yaml.v2" (via gadget.go, info_snap_yaml.go)
    - "github.com/snapcore/snapd/strutil" (via info.go, info_snap_yaml.go) which imports:
      - "gopkg.in/yaml.v2" (via map.go)
    - "github.com/snapcore/snapd/timeout" (via info.go, info_snap_yaml.go)
- "github.com/snapcore/snapd/osutil" (via backend.go, lock.go, ns.go, profile.go) which imports:
  - "github.com/snapcore/snapd/dirs" (via exec.go)
  - "github.com/snapcore/snapd/strutil" (via io.go)
- "github.com/snapcore/snapd/snap" (via backend.go)
- "github.com/snapcore/snapd/cmd" (via ns.go) which imports:
  - "github.com/snapcore/snapd/dirs"
  - "github.com/snapcore/snapd/logger" which imports:
    - "github.com/snapcore/snapd/osutil"
  - "github.com/snapcore/snapd/osutil"
  - "github.com/snapcore/snapd/release"
  - "github.com/snapcore/snapd/strutil"
"github.com/snapcore/snapd/logger"
"github.com/snapcore/snapd/snap"

That's a lot of code and I bet none of it was written with setuidness in mind. Obviously all of it isn't being used by snap-update-ns, but it is all in the address space and therefore available with certain types of memory attacks (this is why in the snap-confine profile we limit the libraries (what is in the address space) to load instead of simply allowing /usr/lib/*.so). I don't have time (today) to verify each API snap-update-ns uses and verify that all the code paths are safe under setuid, which perfectly illustrates my point: snap-update-ns is using too many external libs for a setuid application and even if we did audit them all, these imported packages are getting updates all the time. setuid applications should be small and should not change much because the changes might change assumptions in other parts of the code (we finally got to the point where we aren't modifying snap-confine weekly-- a good thing!). AppArmor confinement mitigates that to some degree (the snap-confine profile has a good bit of privilege as it is), but on systems without AppArmor, this is a pretty big attack surface.

I know the feature is important and I think we can move forward if we:

cut down on the imports in snap-update-ns (this might involve shuffling some things into cmd/snap-update-ns for other code to import from there rather than having snap-update-ns import from elsewhere)
adding a comment to the import list of snap-update-ns stating that we are intentionally keeping it small for security reasons
getting security team involvement in cmd/snap-update-ns PRs. This does not necessarily require deep reviews from the security team (ie, the security team is at liberty to perform a shallow review stating a deep review is not required)