From 0dec25e5a641291e0371fe1fbcb98be9e2194a5b Mon Sep 17 00:00:00 2001 From: Zygmunt Krynicki Date: Wed, 6 Mar 2019 19:09:01 +0100 Subject: [PATCH 1/2] cmd/snap-confine: call sc_should_use_normal_mode once Historically snap-confine used two modes of operation. Initially on core systems snap-confine would unshare the mount namespace and preform some alterations but would otherwise run with the contents of the initial mount namesapce. On classic systems snap-confine would pivot into the base snap (core) and perform some more alterations. With the introduction of multiple boot bases, per-snap bases some of that logic seems rusty. To make it less confusing what is going on reduce the number of places that determine if legacy (sans-pivot) or normal mode (with pivot) to exactly one. There is more cleanup that can be done but it will require making the invocation structure something we can pass around to more functions. This will be attempted in subsequent patches. Signed-off-by: Zygmunt Krynicki --- cmd/snap-confine/mount-support.c | 5 +++-- cmd/snap-confine/mount-support.h | 3 ++- cmd/snap-confine/ns-support.c | 14 +++++--------- cmd/snap-confine/ns-support.h | 3 ++- cmd/snap-confine/snap-confine.c | 10 ++++++++-- 5 files changed, 20 insertions(+), 15 deletions(-) diff --git a/cmd/snap-confine/mount-support.c b/cmd/snap-confine/mount-support.c index 6004b6391a7..bf67e8d3402 100644 --- a/cmd/snap-confine/mount-support.c +++ b/cmd/snap-confine/mount-support.c @@ -516,7 +516,8 @@ static bool __attribute__ ((used)) } void sc_populate_mount_ns(struct sc_apparmor *apparmor, int snap_update_ns_fd, - const char *base_snap_name, const char *snap_name) + const char *base_snap_name, const char *snap_name, + bool is_normal_mode) { // Get the current working directory before we start fiddling with // mounts and possibly pivot_root. At the end of the whole process, we @@ -529,7 +530,7 @@ void sc_populate_mount_ns(struct sc_apparmor *apparmor, int snap_update_ns_fd, // Classify the current distribution, as claimed by /etc/os-release. sc_distro distro = sc_classify_distro(); // Check which mode we should run in, normal or legacy. - if (sc_should_use_normal_mode(distro, base_snap_name)) { + if (is_normal_mode) { // In normal mode we use the base snap as / and set up several bind mounts. const struct sc_mount mounts[] = { {"/dev"}, // because it contains devices on host OS diff --git a/cmd/snap-confine/mount-support.h b/cmd/snap-confine/mount-support.h index 082c304fcc8..55a97701b17 100644 --- a/cmd/snap-confine/mount-support.h +++ b/cmd/snap-confine/mount-support.h @@ -51,7 +51,8 @@ int sc_open_snap_discard_ns(void); * this is impossible it will chdir to SC_VOID_DIR. **/ void sc_populate_mount_ns(struct sc_apparmor *apparmor, int snap_update_ns_fd, - const char *base_snap_name, const char *snap_name); + const char *base_snap_name, const char *snap_name, + bool is_normal_mode); /** * Ensure that / or /snap is mounted with the SHARED option. diff --git a/cmd/snap-confine/ns-support.c b/cmd/snap-confine/ns-support.c index 2071a5e5450..97f52542039 100644 --- a/cmd/snap-confine/ns-support.c +++ b/cmd/snap-confine/ns-support.c @@ -308,7 +308,8 @@ enum sc_discard_vote { static int sc_inspect_and_maybe_discard_stale_ns(int mnt_fd, const char *snap_name, const char *base_snap_name, - int snap_discard_ns_fd) + int snap_discard_ns_fd, + bool is_normal_mode) { char base_snap_rev[PATH_MAX] = { 0 }; char fname[PATH_MAX] = { 0 }; @@ -328,12 +329,6 @@ static int sc_inspect_and_maybe_discard_stale_ns(int mnt_fd, // Find the device that is backing the current revision of the base snap. base_snap_dev = find_base_snap_device(base_snap_name, base_snap_rev); - // Check if we are running in normal mode with pivot root. Do this here - // because once on the inside of the transformed mount namespace we can no - // longer tell. - bool is_normal_mode = - sc_should_use_normal_mode(sc_classify_distro(), base_snap_name); - // Store the PID of this process. This is done instead of calls to // getppid() below because then we can reliably track the PID of the // parent even if the child process is re-parented. @@ -448,7 +443,8 @@ static void helper_capture_per_user_ns(struct sc_mount_ns *group, pid_t parent); int sc_join_preserved_ns(struct sc_mount_ns *group, struct sc_apparmor *apparmor, const char *base_snap_name, - const char *snap_name, int snap_discard_ns_fd) + const char *snap_name, int snap_discard_ns_fd, + bool is_normal_mode) { // Open the mount namespace file. char mnt_fname[PATH_MAX] = { 0 }; @@ -490,7 +486,7 @@ int sc_join_preserved_ns(struct sc_mount_ns *group, struct sc_apparmor // Inspect and perhaps discard the preserved mount namespace. if (sc_inspect_and_maybe_discard_stale_ns (mnt_fd, snap_name, base_snap_name, - snap_discard_ns_fd) == EAGAIN) { + snap_discard_ns_fd, is_normal_mode) == EAGAIN) { return ESRCH; } // Remember the vanilla working directory so that we may attempt to restore it later. diff --git a/cmd/snap-confine/ns-support.h b/cmd/snap-confine/ns-support.h index 288ad10a109..2d3957aff47 100644 --- a/cmd/snap-confine/ns-support.h +++ b/cmd/snap-confine/ns-support.h @@ -92,7 +92,8 @@ void sc_close_mount_ns(struct sc_mount_ns *group); **/ int sc_join_preserved_ns(struct sc_mount_ns *group, struct sc_apparmor *apparmor, const char *base_snap_name, - const char *snap_name, int snap_discard_ns_fd); + const char *snap_name, int snap_discard_ns_fd, + bool is_normal_mode); /** * Join a preserved, per-user, mount namespace if one exists. diff --git a/cmd/snap-confine/snap-confine.c b/cmd/snap-confine/snap-confine.c index a072fef89b7..3c73d303b8d 100644 --- a/cmd/snap-confine/snap-confine.c +++ b/cmd/snap-confine/snap-confine.c @@ -324,6 +324,12 @@ static void enter_non_classic_execution_environment(const sc_invocation * inv, struct sc_mount_ns *group = NULL; group = sc_open_mount_ns(inv->snap_instance); + // Check if we are running in normal mode with pivot root. Do this here + // because once on the inside of the transformed mount namespace we can no + // longer tell. + bool is_normal_mode = sc_should_use_normal_mode(sc_classify_distro(), + inv->base_snap_name); + /* 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. */ @@ -331,7 +337,7 @@ static void enter_non_classic_execution_environment(const sc_invocation * inv, int retval = sc_join_preserved_ns(group, inv->apparmor, inv->base_snap_name, inv->snap_instance, - snap_discard_ns_fd); + snap_discard_ns_fd, is_normal_mode); if (retval == ESRCH) { /* Create and populate the mount namespace. This performs all of the bootstrapping mounts, pivots into the new root filesystem and @@ -342,7 +348,7 @@ static void enter_non_classic_execution_environment(const sc_invocation * inv, } sc_populate_mount_ns(inv->apparmor, snap_update_ns_fd, inv->base_snap_name, - inv->snap_instance); + inv->snap_instance, is_normal_mode); /* Preserve the mount namespace. */ sc_preserve_populated_mount_ns(group); From c7633e9debfebe64dd320996c5ae4a373e0ea25e Mon Sep 17 00:00:00 2001 From: Zygmunt Krynicki Date: Wed, 6 Mar 2019 19:32:49 +0100 Subject: [PATCH 2/2] cmd/snap-confine: move is_normal_mode to sc_invocation This allows us to almost start passing sc_invocation instead of a plethora of arguments to various functions inside ns-support and mount-support modules. This also makes it a tiny bit problematic because is_normal_mode is a boolean that is only set inside the non-classic code path. This is correct for now but should be carefully changed later. Signed-off-by: Zygmunt Krynicki --- cmd/snap-confine/snap-confine.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/cmd/snap-confine/snap-confine.c b/cmd/snap-confine/snap-confine.c index 3c73d303b8d..0725123469b 100644 --- a/cmd/snap-confine/snap-confine.c +++ b/cmd/snap-confine/snap-confine.c @@ -100,10 +100,11 @@ typedef struct sc_invocation { const char *security_tag; const char *snap_instance; struct sc_apparmor *apparmor; + bool is_normal_mode; } 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(sc_invocation * inv, uid_t real_uid, gid_t real_gid, gid_t saved_gid); @@ -202,9 +203,10 @@ int main(int argc, char **argv) .base_snap_name = base_snap_name, .security_tag = security_tag, .apparmor = &apparmor, + /* is_normal_mode is not probed yet */ }; /* For the ease of introducing inv to the if branch below. */ - const sc_invocation *inv = &invocation; + sc_invocation *inv = &invocation; // TODO: check for similar situation and linux capabilities. if (geteuid() == 0) { @@ -283,7 +285,7 @@ static void enter_classic_execution_environment(const sc_invocation * inv) debug("skipping sandbox setup, classic confinement in use"); } -static void enter_non_classic_execution_environment(const sc_invocation * inv, +static void enter_non_classic_execution_environment(sc_invocation * inv, uid_t real_uid, gid_t real_gid, gid_t saved_gid) @@ -327,8 +329,8 @@ static void enter_non_classic_execution_environment(const sc_invocation * inv, // Check if we are running in normal mode with pivot root. Do this here // because once on the inside of the transformed mount namespace we can no // longer tell. - bool is_normal_mode = sc_should_use_normal_mode(sc_classify_distro(), - inv->base_snap_name); + inv->is_normal_mode = sc_should_use_normal_mode(sc_classify_distro(), + inv->base_snap_name); /* Stale mount namespace discarded or no mount namespace to join. We need to construct a new mount namespace ourselves. @@ -337,7 +339,7 @@ static void enter_non_classic_execution_environment(const sc_invocation * inv, int retval = sc_join_preserved_ns(group, inv->apparmor, inv->base_snap_name, inv->snap_instance, - snap_discard_ns_fd, is_normal_mode); + snap_discard_ns_fd, inv->is_normal_mode); if (retval == ESRCH) { /* Create and populate the mount namespace. This performs all of the bootstrapping mounts, pivots into the new root filesystem and @@ -348,7 +350,7 @@ static void enter_non_classic_execution_environment(const sc_invocation * inv, } sc_populate_mount_ns(inv->apparmor, snap_update_ns_fd, inv->base_snap_name, - inv->snap_instance, is_normal_mode); + inv->snap_instance, inv->is_normal_mode); /* Preserve the mount namespace. */ sc_preserve_populated_mount_ns(group);