From 0c2271392cde3f92e9fab7fcde665b5053fb2be3 Mon Sep 17 00:00:00 2001 From: Zygmunt Krynicki Date: Wed, 6 Mar 2019 18:22:37 +0100 Subject: [PATCH 1/4] cmd/snap-confine: introduce sc_invocation Currently passing data around snap-confine is not very easy because of historically organic growth of the codebase. To simplify some of that I'd like to introduce sc_invocation. An ad-hoc structure that captures some of the variables that participate in the essential part of snap-confine's logic. The purpose of this patch is to introduce this structure, switch some of the code to use it (as opposed to accessing the local variables) and set the stage for a subsequent patch that moves a large chunk of code into the two new stubs: enter_{,non_}classic_execution_environment(). This should eventually allow us to make a single call to sc_should_use_normal_mode() and simply pass the result around via the invocation structure. There is plenty of more refactoring that can be done after this patch is in place. Signed-off-by: Zygmunt Krynicki --- cmd/snap-confine/snap-confine.c | 82 +++++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 19 deletions(-) diff --git a/cmd/snap-confine/snap-confine.c b/cmd/snap-confine/snap-confine.c index a129317e4d6..a56930dcab6 100644 --- a/cmd/snap-confine/snap-confine.c +++ b/cmd/snap-confine/snap-confine.c @@ -95,6 +95,18 @@ static void sc_maybe_fixup_udev(void) } } +typedef struct sc_invocation { + const char *base_snap_name; + const char *security_tag; + const char *snap_instance; + struct sc_apparmor *apparmor; + uid_t real_uid, effective_uid, saved_uid; + gid_t real_gid, effective_gid, saved_gid; +} sc_invocation; + +static void enter_classic_execution_environment(const sc_invocation * inv); +static void enter_non_classic_execution_environment(const sc_invocation * inv); + int main(int argc, char **argv) { // Use our super-defensive parser to figure out what we've been asked to do. @@ -182,9 +194,28 @@ int main(int argc, char **argv) " but should be. Refusing to continue to avoid" " permission escalation attacks"); } + + /* Invocation helps to pass relevant data to various parts of snap-confine. */ + sc_invocation invocation = { + .snap_instance = snap_instance, + .base_snap_name = base_snap_name, + .security_tag = security_tag, + .apparmor = &apparmor, + .real_uid = real_uid, + .effective_uid = effective_uid, + .saved_uid = saved_uid, + .real_gid = real_gid, + .effective_gid = effective_gid, + .saved_gid = saved_gid, + }; + /* For the ease of introducing inv to the if branch below. */ + const sc_invocation *inv = &invocation; + // TODO: check for similar situation and linux capabilities. if (geteuid() == 0) { if (classic_confinement) { + /* TODO: move everything into this call */ + enter_classic_execution_environment(inv); /* 'classic confinement' is designed to run without the sandbox * inside the shared namespace. Specifically: * - snap-confine skips using the snap-specific mount namespace @@ -195,6 +226,8 @@ int main(int argc, char **argv) debug ("skipping sandbox setup, classic confinement in use"); } else { + /* TODO: move everything into this call */ + enter_non_classic_execution_environment(inv); /* snap-confine uses privately-shared /run/snapd/ns to store * bind-mounted mount namespaces of each snap. In the case that * snap-confine is invoked from the mount namespace it typically @@ -229,19 +262,19 @@ int main(int argc, char **argv) snap_discard_ns_fd = sc_open_snap_discard_ns(); // Do per-snap initialization. - int snap_lock_fd = sc_lock_snap(snap_instance); + int snap_lock_fd = sc_lock_snap(inv->snap_instance); debug("initializing mount namespace: %s", - snap_instance); + inv->snap_instance); struct sc_mount_ns *group = NULL; - group = sc_open_mount_ns(snap_instance); + group = sc_open_mount_ns(inv->snap_instance); /* Stale mount namespace discarded or no mount namespace to join. We need to construct a new mount namespace ourselves. To capture it we will need a helper process so make one. */ - sc_fork_helper(group, &apparmor); - int retval = sc_join_preserved_ns(group, &apparmor, - base_snap_name, - snap_instance, + sc_fork_helper(group, inv->apparmor); + int retval = sc_join_preserved_ns(group, inv->apparmor, + inv->base_snap_name, + inv->snap_instance, snap_discard_ns_fd); if (retval == ESRCH) { /* Create and populate the mount namespace. This performs all @@ -253,10 +286,10 @@ int main(int argc, char **argv) if (unshare(CLONE_NEWNS) < 0) { die("cannot unshare the mount namespace"); } - sc_populate_mount_ns(&apparmor, + sc_populate_mount_ns(inv->apparmor, snap_update_ns_fd, - base_snap_name, - snap_instance); + inv->base_snap_name, + inv->snap_instance); /* Preserve the mount namespace. */ sc_preserve_populated_mount_ns(group); @@ -269,11 +302,12 @@ int main(int argc, char **argv) sc_maybe_fixup_udev(); /* User mount profiles do not apply to non-root users. */ - if (real_uid != 0) { + if (inv->real_uid != 0) { debug ("joining preserved per-user mount namespace"); retval = sc_join_preserved_per_user_ns(group, + inv-> snap_instance); if (retval == ESRCH) { debug @@ -281,8 +315,9 @@ int main(int argc, char **argv) if (unshare(CLONE_NEWNS) < 0) { die("cannot unshare the mount namespace"); } - sc_setup_user_mounts(&apparmor, + sc_setup_user_mounts(inv->apparmor, snap_update_ns_fd, + inv-> snap_instance); /* Preserve the mount per-user namespace. But only if the * experimental feature is enabled. This way if the feature is @@ -305,17 +340,17 @@ int main(int argc, char **argv) // control group. This simplifies testing if any processes // belonging to a given snap are still alive. // See the documentation of the function for details. - if (getegid() != 0 && saved_gid == 0) { + if (getegid() != 0 && inv->saved_gid == 0) { // Temporarily raise egid so we can chown the freezer cgroup // under LXD. if (setegid(0) != 0) { die("cannot set effective group id to root"); } } - sc_cgroup_freezer_join(snap_instance, getpid()); - if (geteuid() == 0 && real_gid != 0) { - if (setegid(real_gid) != 0) { - die("cannot set effective group id to %d", real_gid); + sc_cgroup_freezer_join(inv->snap_instance, getpid()); + if (geteuid() == 0 && inv->real_gid != 0) { + if (setegid(inv->real_gid) != 0) { + die("cannot set effective group id to %d", inv->real_gid); } } @@ -350,8 +385,9 @@ int main(int argc, char **argv) } } struct snappy_udev udev_s; - if (snappy_udev_init(security_tag, &udev_s) == 0) - setup_devices_cgroup(security_tag, &udev_s); + if (snappy_udev_init(inv->security_tag, &udev_s) == 0) + setup_devices_cgroup(inv->security_tag, + &udev_s); snappy_udev_cleanup(&udev_s); } // The rest does not so temporarily drop privs back to calling @@ -409,3 +445,11 @@ int main(int argc, char **argv) perror("execv failed"); return 1; } + +static void enter_classic_execution_environment(const sc_invocation * inv) +{ +} + +static void enter_non_classic_execution_environment(const sc_invocation * inv) +{ +} From 2902691323c1042ff81136249988c1e37c9ffbb3 Mon Sep 17 00:00:00 2001 From: Zygmunt Krynicki Date: Wed, 6 Mar 2019 18:51:52 +0100 Subject: [PATCH 2/4] cmd/snap-confine: move uid/gid out of sc_invocation Handling uid/gid is a bit more delicate as those values routinely change thought execution. Instead of passing them around in the invocation structure pass them explicitly to the only place that needs it for now. Signed-off-by: Zygmunt Krynicki --- cmd/snap-confine/snap-confine.c | 40 +++++++++++++++------------------ 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/cmd/snap-confine/snap-confine.c b/cmd/snap-confine/snap-confine.c index a56930dcab6..637bbfc9f54 100644 --- a/cmd/snap-confine/snap-confine.c +++ b/cmd/snap-confine/snap-confine.c @@ -100,12 +100,13 @@ typedef struct sc_invocation { const char *security_tag; const char *snap_instance; struct sc_apparmor *apparmor; - uid_t real_uid, effective_uid, saved_uid; - gid_t real_gid, effective_gid, saved_gid; } sc_invocation; static void enter_classic_execution_environment(const sc_invocation * inv); -static void enter_non_classic_execution_environment(const sc_invocation * inv); +static void enter_non_classic_execution_environment(const sc_invocation * inv, + uid_t real_uid, + gid_t real_gid, + gid_t saved_gid); int main(int argc, char **argv) { @@ -201,12 +202,6 @@ int main(int argc, char **argv) .base_snap_name = base_snap_name, .security_tag = security_tag, .apparmor = &apparmor, - .real_uid = real_uid, - .effective_uid = effective_uid, - .saved_uid = saved_uid, - .real_gid = real_gid, - .effective_gid = effective_gid, - .saved_gid = saved_gid, }; /* For the ease of introducing inv to the if branch below. */ const sc_invocation *inv = &invocation; @@ -227,7 +222,9 @@ int main(int argc, char **argv) ("skipping sandbox setup, classic confinement in use"); } else { /* TODO: move everything into this call */ - enter_non_classic_execution_environment(inv); + enter_non_classic_execution_environment(inv, real_uid, + real_gid, + saved_gid); /* snap-confine uses privately-shared /run/snapd/ns to store * bind-mounted mount namespaces of each snap. In the case that * snap-confine is invoked from the mount namespace it typically @@ -302,13 +299,12 @@ int main(int argc, char **argv) sc_maybe_fixup_udev(); /* User mount profiles do not apply to non-root users. */ - if (inv->real_uid != 0) { + if (real_uid != 0) { debug ("joining preserved per-user mount namespace"); retval = sc_join_preserved_per_user_ns(group, - inv-> - snap_instance); + inv->snap_instance); if (retval == ESRCH) { debug ("unsharing the mount namespace (per-user)"); @@ -317,8 +313,7 @@ int main(int argc, char **argv) } sc_setup_user_mounts(inv->apparmor, snap_update_ns_fd, - inv-> - snap_instance); + inv->snap_instance); /* Preserve the mount per-user namespace. But only if the * experimental feature is enabled. This way if the feature is * disabled user mount namespaces will still exist but will be @@ -335,12 +330,11 @@ int main(int argc, char **argv) } } } - // Associate each snap process with a dedicated snap freezer // control group. This simplifies testing if any processes // belonging to a given snap are still alive. // See the documentation of the function for details. - if (getegid() != 0 && inv->saved_gid == 0) { + if (getegid() != 0 && saved_gid == 0) { // Temporarily raise egid so we can chown the freezer cgroup // under LXD. if (setegid(0) != 0) { @@ -348,13 +342,12 @@ int main(int argc, char **argv) } } sc_cgroup_freezer_join(inv->snap_instance, getpid()); - if (geteuid() == 0 && inv->real_gid != 0) { - if (setegid(inv->real_gid) != 0) { - die("cannot set effective group id to %d", inv->real_gid); + if (geteuid() == 0 && real_gid != 0) { + if (setegid(real_gid) != 0) { + die("cannot set effective group id to %d", real_gid); } } - sc_unlock(snap_lock_fd); sc_close_mount_ns(group); @@ -450,6 +443,9 @@ static void enter_classic_execution_environment(const sc_invocation * inv) { } -static void enter_non_classic_execution_environment(const sc_invocation * inv) +static void enter_non_classic_execution_environment(const sc_invocation * inv, + uid_t real_uid, + gid_t real_gid, + gid_t saved_gid) { } From 4bd2950c735b982fa670cbba0d2551ff4094b821 Mon Sep 17 00:00:00 2001 From: Zygmunt Krynicki Date: Thu, 7 Mar 2019 12:20:30 +0100 Subject: [PATCH 3/4] cmd/snap-confine: remove apparmor from sc_invocation After a good night's sleep I realised I should not have put it there in the first place. This will allow the invocatoin to collect and contain only the parts of the arguments that are related to the invocation of snap-confine and leave out certain things that have more complex state (uids) or are an implementation detail (apparmor struct) out of it. Signed-off-by: Zygmunt Krynicki --- cmd/snap-confine/snap-confine.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/cmd/snap-confine/snap-confine.c b/cmd/snap-confine/snap-confine.c index 637bbfc9f54..778c6f0e882 100644 --- a/cmd/snap-confine/snap-confine.c +++ b/cmd/snap-confine/snap-confine.c @@ -99,11 +99,11 @@ typedef struct sc_invocation { const char *base_snap_name; const char *security_tag; const char *snap_instance; - struct sc_apparmor *apparmor; } sc_invocation; static void enter_classic_execution_environment(const sc_invocation * inv); static void enter_non_classic_execution_environment(const sc_invocation * inv, + struct sc_apparmor *aa, uid_t real_uid, gid_t real_gid, gid_t saved_gid); @@ -201,10 +201,10 @@ int main(int argc, char **argv) .snap_instance = snap_instance, .base_snap_name = base_snap_name, .security_tag = security_tag, - .apparmor = &apparmor, }; /* For the ease of introducing inv to the if branch below. */ const sc_invocation *inv = &invocation; + struct sc_apparmor *aa = &apparmor; // TODO: check for similar situation and linux capabilities. if (geteuid() == 0) { @@ -222,7 +222,8 @@ int main(int argc, char **argv) ("skipping sandbox setup, classic confinement in use"); } else { /* TODO: move everything into this call */ - enter_non_classic_execution_environment(inv, real_uid, + enter_non_classic_execution_environment(inv, aa, + real_uid, real_gid, saved_gid); /* snap-confine uses privately-shared /run/snapd/ns to store @@ -268,8 +269,8 @@ int main(int argc, char **argv) /* Stale mount namespace discarded or no mount namespace to join. We need to construct a new mount namespace ourselves. To capture it we will need a helper process so make one. */ - sc_fork_helper(group, inv->apparmor); - int retval = sc_join_preserved_ns(group, inv->apparmor, + sc_fork_helper(group, aa); + int retval = sc_join_preserved_ns(group, aa, inv->base_snap_name, inv->snap_instance, snap_discard_ns_fd); @@ -283,7 +284,7 @@ int main(int argc, char **argv) if (unshare(CLONE_NEWNS) < 0) { die("cannot unshare the mount namespace"); } - sc_populate_mount_ns(inv->apparmor, + sc_populate_mount_ns(aa, snap_update_ns_fd, inv->base_snap_name, inv->snap_instance); @@ -304,16 +305,18 @@ int main(int argc, char **argv) ("joining preserved per-user mount namespace"); retval = sc_join_preserved_per_user_ns(group, - inv->snap_instance); + inv-> + snap_instance); if (retval == ESRCH) { debug ("unsharing the mount namespace (per-user)"); if (unshare(CLONE_NEWNS) < 0) { die("cannot unshare the mount namespace"); } - sc_setup_user_mounts(inv->apparmor, + sc_setup_user_mounts(aa, snap_update_ns_fd, - inv->snap_instance); + inv-> + snap_instance); /* Preserve the mount per-user namespace. But only if the * experimental feature is enabled. This way if the feature is * disabled user mount namespaces will still exist but will be @@ -401,7 +404,7 @@ int main(int argc, char **argv) setup_user_xdg_runtime_dir(); #endif // https://wiki.ubuntu.com/SecurityTeam/Specifications/SnappyConfinement - sc_maybe_aa_change_onexec(&apparmor, security_tag); + sc_maybe_aa_change_onexec(aa, security_tag); #ifdef HAVE_SECCOMP if (sc_apply_seccomp_profile_for_security_tag(security_tag)) { /* If the process is not explicitly unconfined then load the global @@ -444,6 +447,7 @@ static void enter_classic_execution_environment(const sc_invocation * inv) } static void enter_non_classic_execution_environment(const sc_invocation * inv, + struct sc_apparmor *aa, uid_t real_uid, gid_t real_gid, gid_t saved_gid) From 0e5ad1edd16964d2be22e65d41782d2d5111d9fa Mon Sep 17 00:00:00 2001 From: Zygmunt Krynicki Date: Thu, 7 Mar 2019 12:22:08 +0100 Subject: [PATCH 4/4] cmd/snap-confine: remove sc_invocation from enter_classic_execution_environment The classic path is (for now) empty so let's not use the invocation there just yet. Signed-off-by: Zygmunt Krynicki --- cmd/snap-confine/snap-confine.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/snap-confine/snap-confine.c b/cmd/snap-confine/snap-confine.c index 778c6f0e882..0534e0b4754 100644 --- a/cmd/snap-confine/snap-confine.c +++ b/cmd/snap-confine/snap-confine.c @@ -101,7 +101,7 @@ typedef struct sc_invocation { const char *snap_instance; } sc_invocation; -static void enter_classic_execution_environment(const sc_invocation * inv); +static void enter_classic_execution_environment(void); static void enter_non_classic_execution_environment(const sc_invocation * inv, struct sc_apparmor *aa, uid_t real_uid, @@ -210,7 +210,7 @@ int main(int argc, char **argv) if (geteuid() == 0) { if (classic_confinement) { /* TODO: move everything into this call */ - enter_classic_execution_environment(inv); + enter_classic_execution_environment(); /* 'classic confinement' is designed to run without the sandbox * inside the shared namespace. Specifically: * - snap-confine skips using the snap-specific mount namespace @@ -442,7 +442,7 @@ int main(int argc, char **argv) return 1; } -static void enter_classic_execution_environment(const sc_invocation * inv) +static void enter_classic_execution_environment(void) { }