Merge pull request #4732 from kmk3/fix-groups-misc3

Fix keeping certain groups with nogroups

5 files changed, 97 insertions, 55 deletions

diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index a7673ae20..c04a644f4 100644
--- a/src/firejail/firejail.h
+++ b/src/firejail/firejail.h
@@ -506,7 +506,8 @@ void errLogExit(char* fmt, ...) __attribute__((noreturn));
 void fwarning(char* fmt, ...);
 void fmessage(char* fmt, ...);
 long long unsigned parse_arg_size(char *str);
-void drop_privs(int nogroups);
+int check_can_drop_all_groups();
+void drop_privs(int force_nogroups);
 int mkpath_as_root(const char* path);
 void extract_command_name(int index, char **argv);
 void logsignal(int s);
diff --git a/src/firejail/main.c b/src/firejail/main.c
index 59e88bdc6..0262db608 100644
--- a/src/firejail/main.c
+++ b/src/firejail/main.c
@@ -2642,7 +2642,7 @@ int main(int argc, char **argv, char **envp) {
                         else if (cfg.dns4 == NULL)
                                 cfg.dns4 = dns;
                         else {
-                                fwarning("Warning: up to 4 DNS servers can be specified, %s ignored\n", dns);
+                                fwarning("up to 4 DNS servers can be specified, %s ignored\n", dns);
                                 free(dns);
                         }
                 }
@@ -3155,62 +3155,64 @@ int main(int argc, char **argv, char **envp) {
                 ptr += strlen(ptr);
 
                 gid_t g;
-                // add audio group
-                if (!arg_nosound) {
-                        g = get_group_id("audio");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
+                if (!arg_nogroups || !check_can_drop_all_groups()) {
+                        // 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 video group
+                        if (!arg_novideo) {
+                                g = get_group_id("video");
+                                if (g) {
+                                        sprintf(ptr, "%d %d 1\n", g, g);
+                                        ptr += strlen(ptr);
+                                }
                         }
-                }
 
-                // add render group
-                if (!arg_no3d) {
-                        g = get_group_id("render");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
+                        // add render group
+                        if (!arg_no3d) {
+                                g = get_group_id("render");
+                                if (g) {
+                                        sprintf(ptr, "%d %d 1\n", g, g);
+                                        ptr += strlen(ptr);
+                                }
                         }
-                }
 
-                // add lp group
-                if (!arg_noprinters) {
-                        g = get_group_id("lp");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
+                        // add lp group
+                        if (!arg_noprinters) {
+                                g = get_group_id("lp");
+                                if (g) {
+                                        sprintf(ptr, "%d %d 1\n", g, g);
+                                        ptr += strlen(ptr);
+                                }
                         }
-                }
 
-                // add cdrom/optical groups
-                if (!arg_nodvd) {
-                        g = get_group_id("cdrom");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
-                        }
-                        g = get_group_id("optical");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
+                        // add cdrom/optical groups
+                        if (!arg_nodvd) {
+                                g = get_group_id("cdrom");
+                                if (g) {
+                                        sprintf(ptr, "%d %d 1\n", g, g);
+                                        ptr += strlen(ptr);
+                                }
+                                g = get_group_id("optical");
+                                if (g) {
+                                        sprintf(ptr, "%d %d 1\n", g, g);
+                                        ptr += strlen(ptr);
+                                }
                         }
-                }
 
-                // add input group
-                if (!arg_noinput) {
-                        g = get_group_id("input");
-                        if (g) {
-                                sprintf(ptr, "%d %d 1\n", g, g);
-                                ptr += strlen(ptr);
+                        // add input group
+                        if (!arg_noinput) {
+                                g = get_group_id("input");
+                                if (g) {
+                                        sprintf(ptr, "%d %d 1\n", g, g);
+                                        ptr += strlen(ptr);
+                                }
                         }
                 }
 
diff --git a/src/firejail/profile.c b/src/firejail/profile.c
index 756b370aa..5e24591fa 100644
--- a/src/firejail/profile.c
+++ b/src/firejail/profile.c
@@ -1106,7 +1106,7 @@ int profile_check_line(char *ptr, int lineno, const char *fname) {
                 else if (cfg.dns4 == NULL)
                         cfg.dns4 = dns;
                 else {
-                        fwarning("Warning: up to 4 DNS servers can be specified, %s ignored\n", dns);
+                        fwarning("up to 4 DNS servers can be specified, %s ignored\n", dns);
                         free(dns);
                 }
                 return 0;
diff --git a/src/firejail/sandbox.c b/src/firejail/sandbox.c
index 3887b5701..96fa4c81a 100644
--- a/src/firejail/sandbox.c
+++ b/src/firejail/sandbox.c
@@ -1225,7 +1225,7 @@ int sandbox(void* sandbox_arg) {
         //****************************************
         // drop privileges
         //****************************************
-        drop_privs(arg_nogroups);
+        drop_privs(0);
 
         // kill the sandbox in case the parent died
         prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
diff --git a/src/firejail/util.c b/src/firejail/util.c
index 97afe9649..c1c31b43c 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -103,6 +103,41 @@ void errLogExit(char* fmt, ...) {
         exit(1);
 }
 
+// Returns whether all supplementary groups can be safely dropped
+int check_can_drop_all_groups() {
+        static int can_drop_all_groups = -1;
+
+        // Avoid needlessly checking (and printing) things twice
+        if (can_drop_all_groups != -1)
+                goto out;
+
+        // nvidia cards require video group; ignore nogroups
+        if (access("/dev/nvidiactl", R_OK) == 0 && arg_no3d == 0) {
+                fwarning("NVIDIA card detected, nogroups command ignored\n");
+                can_drop_all_groups = 0;
+                goto out;
+        }
+
+        /* When we are not sure that the system has working seat-based ACLs
+         * (e.g.: probably yes on (e)udev + (e)logind, probably not on eudev +
+         * seatd), supplementary groups (e.g.: audio and input) might be needed
+         * to avoid breakage (e.g.: audio or gamepads not working).  See #4600
+         * and #4603.
+         */
+        if (access("/run/systemd/seats/", F_OK) != 0) {
+                fwarning("logind not detected, nogroups command ignored\n");
+                can_drop_all_groups = 0;
+                goto out;
+        }
+
+        if (arg_debug)
+                fprintf(stderr, "nogroups command not ignored\n");
+        can_drop_all_groups = 1;
+
+out:
+        return can_drop_all_groups;
+}
+
 static int find_group(gid_t group, const gid_t *groups, int ngroups) {
         int i;
         for (i = 0; i < ngroups; i++) {
@@ -141,6 +176,9 @@ static void clean_supplementary_groups(gid_t gid) {
         if (rv == -1)
                 goto clean_all;
 
+        if (arg_nogroups && check_can_drop_all_groups())
+                goto clean_all;
+
         // clean supplementary group list
         gid_t new_groups[MAX_GROUPS];
         int new_ngroups = 0;
@@ -215,21 +253,22 @@ clean_all:
 
 
 // drop privileges
-// - for root group or if nogroups is set, supplementary groups are not configured
-void drop_privs(int nogroups) {
+// - for root group or if force_nogroups is set, supplementary groups are not configured
+void drop_privs(int force_nogroups) {
         gid_t gid = getgid();
         if (arg_debug)
-                printf("Drop privileges: pid %d, uid %d, gid %d, nogroups %d\n",  getpid(), getuid(), gid, nogroups);
+                printf("Drop privileges: pid %d, uid %d, gid %d, force_nogroups %d\n",
+                       getpid(), getuid(), gid, force_nogroups);
 
         // configure supplementary groups
         EUID_ROOT();
-        if (gid == 0 || nogroups) {
+        if (gid == 0 || force_nogroups) {
                 if (setgroups(0, NULL) < 0)
                         errExit("setgroups");
                 if (arg_debug)
                         printf("No supplementary groups\n");
         }
-        else if (arg_noroot)
+        else if (arg_noroot || arg_nogroups)
                 clean_supplementary_groups(gid);
 
         // set uid/gid