Merge pull request #4632 from kmk3/consider-nosound-novideo-groups

Consider nosound and novideo when keeping groups & misc refactors

2 files changed, 124 insertions, 92 deletions

diff --git a/src/firejail/main.c b/src/firejail/main.c
index 32c25c8d9..b4117bb70 100644
--- a/src/firejail/main.c
+++ b/src/firejail/main.c
@@ -3094,94 +3094,98 @@ int main(int argc, char **argv, char **envp) {
         }
         EUID_ASSERT();
 
-        // close each end of the unused pipes
-        close(parent_to_child_fds[0]);
-        close(child_to_parent_fds[1]);
+        // close each end of the unused pipes
+        close(parent_to_child_fds[0]);
+        close(child_to_parent_fds[1]);
 
         // notify child that base setup is complete
-        notify_other(parent_to_child_fds[1]);
+        notify_other(parent_to_child_fds[1]);
 
-        // wait for child to create new user namespace with CLONE_NEWUSER
-        wait_for_other(child_to_parent_fds[0]);
-        close(child_to_parent_fds[0]);
+        // wait for child to create new user namespace with CLONE_NEWUSER
+        wait_for_other(child_to_parent_fds[0]);
+        close(child_to_parent_fds[0]);
 
-        if (arg_noroot) {
-                // update the UID and GID maps in the new child user namespace
+        if (arg_noroot) {
+                // update the UID and GID maps in the new child user namespace
                 // uid
-                char *map_path;
-                if (asprintf(&map_path, "/proc/%d/uid_map", child) == -1)
-                        errExit("asprintf");
-
-                char *map;
-                uid_t uid = getuid();
-                if (asprintf(&map, "%d %d 1", uid, uid) == -1)
-                        errExit("asprintf");
-                EUID_ROOT();
-                update_map(map, map_path);
-                EUID_USER();
-                free(map);
-                free(map_path);
-
-                // gid file
+                char *map_path;
+                if (asprintf(&map_path, "/proc/%d/uid_map", child) == -1)
+                        errExit("asprintf");
+
+                char *map;
+                uid_t uid = getuid();
+                if (asprintf(&map, "%d %d 1", uid, uid) == -1)
+                        errExit("asprintf");
+                EUID_ROOT();
+                update_map(map, map_path);
+                EUID_USER();
+                free(map);
+                free(map_path);
+
+                // gid file
                 if (asprintf(&map_path, "/proc/%d/gid_map", child) == -1)
                         errExit("asprintf");
-                char gidmap[1024];
-                char *ptr = gidmap;
-                *ptr = '\0';
-
-                // add user group
-                gid_t gid = getgid();
-                sprintf(ptr, "%d %d 1\n", gid, gid);
-                ptr += strlen(ptr);
-
-                if (!arg_nogroups) {
-                        //  add firejail group
-                        gid_t g = get_group_id("firejail");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
-                        }
-
-                        //  add tty group
-                        g = get_group_id("tty");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
-                        }
-
-                        //  add audio group
-                        g = get_group_id("audio");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
-                        }
-
-                        //  add video group
-                        g = get_group_id("video");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
-                        }
-
-                        //  add games group
-                        g = get_group_id("games");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                        }
-                 }
-
-                EUID_ROOT();
-                update_map(gidmap, map_path);
-                EUID_USER();
-                free(map_path);
-        }
+                char gidmap[1024];
+                char *ptr = gidmap;
+                *ptr = '\0';
+
+                // add user group
+                gid_t gid = getgid();
+                sprintf(ptr, "%d %d 1\n", gid, gid);
+                ptr += strlen(ptr);
+
+                if (!arg_nogroups) {
+                        // add firejail group
+                        gid_t g = get_group_id("firejail");
+                        if (g) {
+                                sprintf(ptr, "%d %d 1\n", g, g);
+                                ptr += strlen(ptr);
+                        }
+
+                        // add tty group
+                        g = get_group_id("tty");
+                        if (g) {
+                                sprintf(ptr, "%d %d 1\n", g, g);
+                                ptr += strlen(ptr);
+                        }
+
+                        // add audio group
+                        if (!arg_nosound) {
+                                g = get_group_id("audio");
+                                if (g) {
+                                        sprintf(ptr, "%d %d 1\n", g, g);
+                                        ptr += strlen(ptr);
+                                }
+                        }
+
+                        // add video group
+                        if (!arg_novideo) {
+                                g = get_group_id("video");
+                                if (g) {
+                                        sprintf(ptr, "%d %d 1\n", g, g);
+                                        ptr += strlen(ptr);
+                                }
+                        }
+
+                        // add games group
+                        g = get_group_id("games");
+                        if (g) {
+                                sprintf(ptr, "%d %d 1\n", g, g);
+                        }
+                }
+
+                EUID_ROOT();
+                update_map(gidmap, map_path);
+                EUID_USER();
+                free(map_path);
+        }
         EUID_ASSERT();
 
-        // notify child that UID/GID mapping is complete
-        notify_other(parent_to_child_fds[1]);
-        close(parent_to_child_fds[1]);
+        // notify child that UID/GID mapping is complete
+        notify_other(parent_to_child_fds[1]);
+        close(parent_to_child_fds[1]);
 
-        EUID_ROOT();
+        EUID_ROOT();
         if (lockfd_network != -1) {
                 flock(lockfd_network, LOCK_UN);
                 close(lockfd_network);
diff --git a/src/firejail/util.c b/src/firejail/util.c
index 86977cecf..3bfb4435e 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -103,6 +103,36 @@ void errLogExit(char* fmt, ...) {
         exit(1);
 }
 
+static int find_group(gid_t group, const gid_t *groups, int ngroups) {
+        int i;
+        for (i = 0; i < ngroups; i++) {
+                if (group == groups[i])
+                        return i;
+        }
+
+        return -1;
+}
+
+// Gets group from "groupname" and adds it to "new_groups" if it exists on
+// "groups".  Always returns the current value of new_ngroups.
+static int copy_group_ifcont(const char *groupname,
+                             const gid_t *groups, int ngroups,
+                             gid_t *new_groups, int *new_ngroups, int new_sz) {
+        if (*new_ngroups >= new_sz) {
+                errno = ERANGE;
+                goto out;
+        }
+
+        gid_t g = get_group_id(groupname);
+        if (g && find_group(g, groups, ngroups) >= 0) {
+                new_groups[*new_ngroups] = g;
+                (*new_ngroups)++;
+        }
+
+out:
+        return *new_ngroups;
+}
+
 static void clean_supplementary_groups(gid_t gid) {
         assert(cfg.username);
         gid_t groups[MAX_GROUPS];
@@ -112,34 +142,32 @@ static void clean_supplementary_groups(gid_t gid) {
                 goto clean_all;
 
         // clean supplementary group list
-        // allow only firejail, tty, audio, video, games
         gid_t new_groups[MAX_GROUPS];
         int new_ngroups = 0;
         char *allowed[] = {
                 "firejail",
                 "tty",
-                "audio",
-                "video",
                 "games",
                 NULL
         };
 
         int i = 0;
         while (allowed[i]) {
-                gid_t g = get_group_id(allowed[i]);
-                if (g) {
-                        int j;
-                        for (j = 0; j < ngroups; j++) {
-                                if (g == groups[j]) {
-                                        new_groups[new_ngroups] = g;
-                                        new_ngroups++;
-                                        break;
-                                }
-                        }
-                }
+                copy_group_ifcont(allowed[i], groups, ngroups,
+                                  new_groups, &new_ngroups, MAX_GROUPS);
                 i++;
         }
 
+        if (!arg_nosound) {
+                copy_group_ifcont("audio", groups, ngroups,
+                                  new_groups, &new_ngroups, MAX_GROUPS);
+        }
+
+        if (!arg_novideo) {
+                copy_group_ifcont("video", groups, ngroups,
+                                  new_groups, &new_ngroups, MAX_GROUPS);
+        }
+
         if (new_ngroups) {
                 rv = setgroups(new_ngroups, new_groups);
                 if (rv)