Skip to content

Commit

Permalink
cmd/libsnap-confine-private/tool: switch identity only after forking …
Browse files Browse the repository at this point in the history 
…a child (canonical#15100)

* cmd/libsnap-confine-private: tweak sc_identity to use bool fields

Switch sc_identity change_{uid,gid} to use bools, such that it's clearer
that those fields are not carrying actual uid/gid but only dictate the
action to be taken.

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* cmd/libsnap-confine-private/tool: switch identity only after forking a child

We had a recurring pattern of switching the identity, then calling a
tool, followed by restoring the old identity. Since the tool is called
in a forked child process anyway, there is no clear win to switch the
identity in the parent process, which then requires a followup restore
operation. Move switching of identity to the child process, this
simplifying the code path.

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

---------

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
  • Loading branch information
@bboozzoo
bboozzoo authored Feb 24, 2025
1 parent 80568f1 commit 3e1460c
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 20 deletions.
41 changes: 25 additions & 16 deletions cmd/libsnap-confine-private/tool.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,18 @@ static int sc_open_snapd_tool(const char *tool_name);
* The environment vector has special support for expanding the string "SNAPD_DEBUG=x".
* If such string is present, the "x" is replaced with either "0" or "1" depending on
* the result of is_sc_debug_enabled().
*
* Identity indicates the set of effective uid/gid which are to be applied in
* the child process right after forking.
**/
static void sc_call_snapd_tool(int tool_fd, const char *tool_name, char **argv, char **envp);
static void sc_call_snapd_tool(int tool_fd, const char *tool_name, sc_identity identity, char **argv, char **envp);

/**
* sc_call_snapd_tool_with_apparmor calls a snapd tool by file descriptor,
* possibly confining the program with a specific apparmor profile.
**/
static void sc_call_snapd_tool_with_apparmor(int tool_fd, const char *tool_name, struct sc_apparmor *apparmor,
const char *aa_profile, char **argv, char **envp);
const char *aa_profile, sc_identity identity, char **argv, char **envp);

int sc_open_snap_update_ns(void) { return sc_open_snapd_tool("snap-update-ns"); }

Expand All @@ -77,11 +80,11 @@ void sc_call_snap_update_ns(int snap_update_ns_fd, const char *snap_name, struct
"--from-snap-confine", snap_name_copy, NULL};
char *envp[] = {"SNAPD_DEBUG=x", NULL};

/* Switch the group to root so that directories, files and locks created by
* snap-update-ns are owned by the root group. */
sc_identity old = sc_set_effective_identity(sc_root_group_identity());
sc_call_snapd_tool_with_apparmor(snap_update_ns_fd, "snap-update-ns", apparmor, aa_profile, argv, envp);
(void)sc_set_effective_identity(old);
/* invoke snap-update-ns having switched to root group, so that created
* directories, files and locks are owned by the root group. */
sc_identity root_identity = sc_root_group_identity();
sc_call_snapd_tool_with_apparmor(snap_update_ns_fd, "snap-update-ns", apparmor, aa_profile, root_identity, argv,
envp);
}

void sc_call_snap_update_ns_as_user(int snap_update_ns_fd, const char *snap_name, struct sc_apparmor *apparmor) {
Expand Down Expand Up @@ -112,7 +115,13 @@ void sc_call_snap_update_ns_as_user(int snap_update_ns_fd, const char *snap_name
* with either SNAPD_DEBUG=0 or SNAPD_DEBUG=1, see that function
* for details. */
"SNAPD_DEBUG=x", xdg_runtime_dir_env, snap_real_home_env, NULL};
sc_call_snapd_tool_with_apparmor(snap_update_ns_fd, "snap-update-ns", apparmor, aa_profile, argv, envp);
/* keep the current identity */
sc_identity no_change_identity = {
.change_gid = false,
.change_uid = false,
};
sc_call_snapd_tool_with_apparmor(snap_update_ns_fd, "snap-update-ns", apparmor, aa_profile, no_change_identity,
argv, envp);
}

int sc_open_snap_discard_ns(void) { return sc_open_snapd_tool("snap-discard-ns"); }
Expand All @@ -124,11 +133,10 @@ void sc_call_snap_discard_ns(int snap_discard_ns_fd, const char *snap_name) {
/* SNAPD_DEBUG=x is replaced by sc_call_snapd_tool_with_apparmor with
* either SNAPD_DEBUG=0 or SNAPD_DEBUG=1, see that function for details. */
char *envp[] = {"SNAPD_DEBUG=x", NULL};
/* Switch the group to root so that directories and locks created by
* snap-discard-ns are owned by the root group. */
sc_identity old = sc_set_effective_identity(sc_root_group_identity());
sc_call_snapd_tool(snap_discard_ns_fd, "snap-discard-ns", argv, envp);
(void)sc_set_effective_identity(old);
/* invoke snap-discard-ns having switched to root group, so that created
* directories, files and locks are owned by the root group. */
sc_identity root_identity = sc_root_group_identity();
sc_call_snapd_tool(snap_discard_ns_fd, "snap-discard-ns", root_identity, argv, envp);
}

static int sc_open_snapd_tool(const char *tool_name) {
Expand Down Expand Up @@ -164,18 +172,19 @@ static int sc_open_snapd_tool(const char *tool_name) {
return tool_fd;
}

static void sc_call_snapd_tool(int tool_fd, const char *tool_name, char **argv, char **envp) {
sc_call_snapd_tool_with_apparmor(tool_fd, tool_name, NULL, NULL, argv, envp);
static void sc_call_snapd_tool(int tool_fd, const char *tool_name, sc_identity identity, char **argv, char **envp) {
sc_call_snapd_tool_with_apparmor(tool_fd, tool_name, NULL, NULL, identity, argv, envp);
}

static void sc_call_snapd_tool_with_apparmor(int tool_fd, const char *tool_name, struct sc_apparmor *apparmor,
const char *aa_profile, char **argv, char **envp) {
const char *aa_profile, sc_identity identity, char **argv, char **envp) {
debug("calling snapd tool %s", tool_name);
pid_t child = fork();
if (child < 0) {
die("cannot fork to run snapd tool %s", tool_name);
}
if (child == 0) {
sc_set_effective_identity(identity);
/* If the caller provided template environment entry for SNAPD_DEBUG
* then expand it to the actual value. */
for (char **env = envp;
Expand Down
8 changes: 4 additions & 4 deletions cmd/libsnap-confine-private/utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ bool sc_is_in_container(void);
typedef struct sc_identity {
uid_t uid;
gid_t gid;
unsigned change_uid : 1;
unsigned change_gid : 1;
bool change_uid : 1;
bool change_gid : 1;
} sc_identity;

/**
Expand All @@ -75,8 +75,8 @@ static inline sc_identity sc_root_group_identity(void) {
sc_identity id = {
/* Explicitly set our intent of changing just the GID.
* Refactoring of this code must retain this property. */
.change_uid = 0,
.change_gid = 1,
.change_uid = false,
.change_gid = true,
.gid = 0,
};
return id;
Expand Down

0 comments on commit 3e1460c

Please sign in to comment.