From 7abce0b4c2891f68751cc18263709e90d48e097d Mon Sep 17 00:00:00 2001 From: "Kelvin M. Klann" Date: Tue, 30 Nov 2021 19:51:00 -0300 Subject: [PATCH] Fix keeping certain groups with nogroups This amends commit b828a9047 ("Keep audio and video groups regardless of nogroups", 2021-11-28) from PR #4725. The commit above did not change the behavior (the groups are still not kept). With this commit, it appears to work properly: $ groups | grep audio >/dev/null && echo kept kept # with check_can_drop_all_groups == 0 $ firejail --quiet --noprofile --nogroups groups | grep audio >/dev/null && echo kept kept # with check_can_drop_all_groups == 1 $ firejail --quiet --noprofile --nogroups groups | grep audio >/dev/null && echo kept $ Add a new check_can_drop_all_groups function to check whether the supplementary groups can be safely dropped without potentially causing issues with audio, 3D hardware acceleration or input (and maybe more). It returns false if nvidia (and no `no3d`) is used or if (e)logind is not running, as in either case the supplementary groups might be needed. Note: With this, the behavior from before #4725 is restored on (e)logind systems (when not using nvidia), as it makes the supplementary groups always be dropped on such systems. Note2: Even with the static variable, these checks still happen at least twice. It seems that it happens once per translation unit (and I think that it may happen more times if there are multiple processes involved). This also amends (/kind of reverts) commit 6ddedeba0 ("Make nogroups work on nvidia again", 2021-11-29) from PR #4725, as it restores the nvidia check from it into the new check_can_drop_all_groups function. --- src/firejail/firejail.h | 1 + src/firejail/main.c | 94 +++++++++++++++++++++-------------------- src/firejail/sandbox.c | 2 +- src/firejail/util.c | 40 +++++++++++++++++- 4 files changed, 89 insertions(+), 48 deletions(-) diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h index bbc496afc35..c04a644f4eb 100644 --- a/src/firejail/firejail.h +++ b/src/firejail/firejail.h @@ -506,6 +506,7 @@ void errLogExit(char* fmt, ...) __attribute__((noreturn)); void fwarning(char* fmt, ...); void fmessage(char* fmt, ...); long long unsigned parse_arg_size(char *str); +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); diff --git a/src/firejail/main.c b/src/firejail/main.c index e8815d5cd16..0262db608bf 100644 --- a/src/firejail/main.c +++ b/src/firejail/main.c @@ -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/sandbox.c b/src/firejail/sandbox.c index 3887b570151..96fa4c81af5 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 55df4441485..c1c31b43c7c 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; @@ -230,7 +268,7 @@ void drop_privs(int force_nogroups) { 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