Skip to content

Commit

Permalink
Fix keeping certain groups with nogroups
Browse files Browse the repository at this point in the history
This amends commit b828a90 ("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 6ddedeb ("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.
  • Loading branch information
kmk3 committed Dec 7, 2021
1 parent be5970c commit 7abce0b
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 48 deletions.
1 change: 1 addition & 0 deletions src/firejail/firejail.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
94 changes: 48 additions & 46 deletions src/firejail/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/firejail/sandbox.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
40 changes: 39 additions & 1 deletion src/firejail/util.c
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 7abce0b

Please sign in to comment.