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

Consider nosound and novideo when keeping groups & misc refactors
netblue30 · Nov 20, 2021 · 6acd0d3 · 6acd0d3
2 parents c477e00 + ea564eb
commit 6acd0d3
Show file tree

Hide file tree

Showing 2 changed files with 124 additions and 92 deletions.
diff --git 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
@@ -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)