Fix keeping certain groups with nogroups #4732
Conversation
To not be confused with arg_nogroups, as in the vast majority of cases drop_privs is called with either 0 or 1 rather than arg_nogroups. The rename makes it clearer that what the parameter does is to drop all groups without exception, unlike arg_nogroups, which may have certain groups be kept.
This amends commit 11418a4 ("dns fixes", 2019-10-31). fwarning already prints "Warning: " at the beginning. Kind of relates to commit 6ddedeb ("Make nogroups work on nvidia again", 2021-11-29) / PR netblue30#4725, which removed code affected by this. Command used to find the duplicates: git grep -i -F 'fwarning("Warning:' -- src
kmk3
requested review from
glitsj16,
reinerh,
netblue30,
topimiettinen and
rusty-snake
kmk3
force-pushed
the
fix-groups-misc3
branch
from
9be4725
to
1d1ade4
Compare
|
Force-pushed to fix this: https://github.com/netblue30/firejail/runs/4386125509?check_suite_focus=true |
By the way, I had also tried using a global variable (declared on main.c and |
| fwarning("NVIDIA card detected, nogroups command ignored\n"); | ||
| can_drop_all_groups = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd move the else if ... here and put the comment below it. Now it looks like earlier if (access(... didn't have an else part but of course it does when you read further.
There was a problem hiding this comment.
I'd move the
else if ...here and put the comment below it. Now it looks
like earlierif (access(...didn't have an else part but of course it does
when you read further.
I think I got this idea from something like this code on profile.c:
// mkdir
if (strncmp(ptr, "mkdir ", 6) == 0) {
fs_mkdir(ptr + 6);
return 1; // process mkdir again while applying blacklists
}
// mkfile
if (strncmp(ptr, "mkfile ", 7) == 0) {
fs_mkfile(ptr + 7);
return 1; // process mkfile again while applying blacklists
}
// sandbox name
else if (strncmp(ptr, "name ", 5) == 0) {
cfg.name = ptr + 5;
if (strlen(cfg.name) == 0) {
fprintf(stderr, "Error: invalid sandbox name\n");
exit(1);
}
return 0;
}
else if (strcmp(ptr, "ipc-namespace") == 0) {
arg_ipc = 1;
return 0;
}But yeah, it's not very obvious; I'll change it along with some other stuff and
force-push.
There was a problem hiding this comment.
Done. Just moving the comment made things look a bit crowded, so I added a few
more gotos instead.
|
I just tested your pull request. audio and input groups seem to be allowed. I haven't tested video group because I don't have a webcam. |
Thanks for testing! Could you also test if this fixes the audio/input issues
|
I wrote that the pull request worked after testing retroarch with retroarch.profile. I didn't ignore noroot or nogroups. |
Nice, glad to have it confirmed that these issues are fixed with this. (To clarify, I asked again because my original request was rather vague and it |
This amends commit b828a90 ("Keep audio and video groups regardless of nogroups", 2021-11-28) from PR netblue30#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 netblue30#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 netblue30#4725, as it restores the nvidia check from it into the new check_can_drop_all_groups function.
kmk3
force-pushed
the
fix-groups-misc3
branch
from
1d1ade4
to
7abce0b
Compare
|
merged, thanks! |
Added on commit 7abce0b ("Fix keeping certain groups with nogroups", 2021-11-30) / PR netblue30#4732. As reported by @rusty-snake on netblue30#4930, conflicting messages are printed when using whitelist-run-common.inc with nogroups: $ cat test.profile include whitelist-run-common.inc nogroups $ firejail --profile=./test.profile groups Reading profile ./test.profile Reading profile /etc/firejail/whitelist-run-common.inc Parent pid 1234, child pid 1235 Warning: logind not detected, nogroups command ignored <--- is a lie Warning: cleaning all supplementary groups Child process initialized in 30.00 ms rusty-snake <---- running `groups` outside of the sandbox shows more so groups are actually cleaned Parent is shutting down, bye... This probably happens because wrc causes /run/systemd to be hidden in the sandbox and because check_can_drop_all_groups is called multiple times, seemingly both before and after the whitelisting goes into effect. So disable the message about nogroups being ignored, but keep the message about cleaning all supplementary groups (which is unlikely to be printed unless it really happens). Fixes netblue30#4930.
When nogroups is used, the following warning may be issued (potentially
multiple times, as drop_privs may be called more than once):
Warning: cleaning all supplementary groups
But the warning is being shown even when it seems that all supplementary
groups can be safely dropped (and are thus dropped), which is likely a
common scenario. This commit prevents the warning from being printed in
that case, making it so that it is only shown in the non-happy paths (as
was the case on firejail 0.9.66).
Misc: The added code was copied from drop_privs.
This amends commit 7abce0b ("Fix keeping certain groups with
nogroups", 2021-11-30) / PR netblue30#4732.
Kind of relates to netblue30#4930.
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:
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 isnot 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.
Note2: Even with the static variable, these checks still happen at least
once per translation unit (so twice in total per my testing).
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.