diff options
author | Kelvin M. Klann <kmk3.code@protonmail.com> | 2021-11-30 19:51:00 -0300 |
---|---|---|
committer | Kelvin M. Klann <kmk3.code@protonmail.com> | 2021-12-07 14:52:45 -0300 |
commit | 7abce0b4c2891f68751cc18263709e90d48e097d (patch) | |
tree | 07e52ab8780ab0d7c0f8a72fa63190a19531dc2b | |
parent | Fix duplicated fwarning warnings (diff) | |
download | firejail-7abce0b4c2891f68751cc18263709e90d48e097d.tar.gz firejail-7abce0b4c2891f68751cc18263709e90d48e097d.tar.zst firejail-7abce0b4c2891f68751cc18263709e90d48e097d.zip |
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.
-rw-r--r-- | src/firejail/firejail.h | 1 | ||||
-rw-r--r-- | src/firejail/main.c | 94 | ||||
-rw-r--r-- | src/firejail/sandbox.c | 2 | ||||
-rw-r--r-- | 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 bbc496afc..c04a644f4 100644 --- a/src/firejail/firejail.h +++ b/src/firejail/firejail.h | |||
@@ -506,6 +506,7 @@ void errLogExit(char* fmt, ...) __attribute__((noreturn)); | |||
506 | void fwarning(char* fmt, ...); | 506 | void fwarning(char* fmt, ...); |
507 | void fmessage(char* fmt, ...); | 507 | void fmessage(char* fmt, ...); |
508 | long long unsigned parse_arg_size(char *str); | 508 | long long unsigned parse_arg_size(char *str); |
509 | int check_can_drop_all_groups(); | ||
509 | void drop_privs(int force_nogroups); | 510 | void drop_privs(int force_nogroups); |
510 | int mkpath_as_root(const char* path); | 511 | int mkpath_as_root(const char* path); |
511 | void extract_command_name(int index, char **argv); | 512 | void extract_command_name(int index, char **argv); |
diff --git a/src/firejail/main.c b/src/firejail/main.c index e8815d5cd..0262db608 100644 --- a/src/firejail/main.c +++ b/src/firejail/main.c | |||
@@ -3155,62 +3155,64 @@ int main(int argc, char **argv, char **envp) { | |||
3155 | ptr += strlen(ptr); | 3155 | ptr += strlen(ptr); |
3156 | 3156 | ||
3157 | gid_t g; | 3157 | gid_t g; |
3158 | // add audio group | 3158 | if (!arg_nogroups || !check_can_drop_all_groups()) { |
3159 | if (!arg_nosound) { | 3159 | // add audio group |
3160 | g = get_group_id("audio"); | 3160 | if (!arg_nosound) { |
3161 | if (g) { | 3161 | g = get_group_id("audio"); |
3162 | sprintf(ptr, "%d %d 1\n", g, g); | 3162 | if (g) { |
3163 | ptr += strlen(ptr); | 3163 | sprintf(ptr, "%d %d 1\n", g, g); |
3164 | ptr += strlen(ptr); | ||
3165 | } | ||
3164 | } | 3166 | } |
3165 | } | ||
3166 | 3167 | ||
3167 | // add video group | 3168 | // add video group |
3168 | if (!arg_novideo) { | 3169 | if (!arg_novideo) { |
3169 | g = get_group_id("video"); | 3170 | g = get_group_id("video"); |
3170 | if (g) { | 3171 | if (g) { |
3171 | sprintf(ptr, "%d %d 1\n", g, g); | 3172 | sprintf(ptr, "%d %d 1\n", g, g); |
3172 | ptr += strlen(ptr); | 3173 | ptr += strlen(ptr); |
3174 | } | ||
3173 | } | 3175 | } |
3174 | } | ||
3175 | 3176 | ||
3176 | // add render group | 3177 | // add render group |
3177 | if (!arg_no3d) { | 3178 | if (!arg_no3d) { |
3178 | g = get_group_id("render"); | 3179 | g = get_group_id("render"); |
3179 | if (g) { | 3180 | if (g) { |
3180 | sprintf(ptr, "%d %d 1\n", g, g); | 3181 | sprintf(ptr, "%d %d 1\n", g, g); |
3181 | ptr += strlen(ptr); | 3182 | ptr += strlen(ptr); |
3183 | } | ||
3182 | } | 3184 | } |
3183 | } | ||
3184 | 3185 | ||
3185 | // add lp group | 3186 | // add lp group |
3186 | if (!arg_noprinters) { | 3187 | if (!arg_noprinters) { |
3187 | g = get_group_id("lp"); | 3188 | g = get_group_id("lp"); |
3188 | if (g) { | 3189 | if (g) { |
3189 | sprintf(ptr, "%d %d 1\n", g, g); | 3190 | sprintf(ptr, "%d %d 1\n", g, g); |
3190 | ptr += strlen(ptr); | 3191 | ptr += strlen(ptr); |
3192 | } | ||
3191 | } | 3193 | } |
3192 | } | ||
3193 | 3194 | ||
3194 | // add cdrom/optical groups | 3195 | // add cdrom/optical groups |
3195 | if (!arg_nodvd) { | 3196 | if (!arg_nodvd) { |
3196 | g = get_group_id("cdrom"); | 3197 | g = get_group_id("cdrom"); |
3197 | if (g) { | 3198 | if (g) { |
3198 | sprintf(ptr, "%d %d 1\n", g, g); | 3199 | sprintf(ptr, "%d %d 1\n", g, g); |
3199 | ptr += strlen(ptr); | 3200 | ptr += strlen(ptr); |
3200 | } | 3201 | } |
3201 | g = get_group_id("optical"); | 3202 | g = get_group_id("optical"); |
3202 | if (g) { | 3203 | if (g) { |
3203 | sprintf(ptr, "%d %d 1\n", g, g); | 3204 | sprintf(ptr, "%d %d 1\n", g, g); |
3204 | ptr += strlen(ptr); | 3205 | ptr += strlen(ptr); |
3206 | } | ||
3205 | } | 3207 | } |
3206 | } | ||
3207 | 3208 | ||
3208 | // add input group | 3209 | // add input group |
3209 | if (!arg_noinput) { | 3210 | if (!arg_noinput) { |
3210 | g = get_group_id("input"); | 3211 | g = get_group_id("input"); |
3211 | if (g) { | 3212 | if (g) { |
3212 | sprintf(ptr, "%d %d 1\n", g, g); | 3213 | sprintf(ptr, "%d %d 1\n", g, g); |
3213 | ptr += strlen(ptr); | 3214 | ptr += strlen(ptr); |
3215 | } | ||
3214 | } | 3216 | } |
3215 | } | 3217 | } |
3216 | 3218 | ||
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) { | |||
1225 | //**************************************** | 1225 | //**************************************** |
1226 | // drop privileges | 1226 | // drop privileges |
1227 | //**************************************** | 1227 | //**************************************** |
1228 | drop_privs(arg_nogroups); | 1228 | drop_privs(0); |
1229 | 1229 | ||
1230 | // kill the sandbox in case the parent died | 1230 | // kill the sandbox in case the parent died |
1231 | prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0); | 1231 | prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0); |
diff --git a/src/firejail/util.c b/src/firejail/util.c index 55df44414..c1c31b43c 100644 --- a/src/firejail/util.c +++ b/src/firejail/util.c | |||
@@ -103,6 +103,41 @@ void errLogExit(char* fmt, ...) { | |||
103 | exit(1); | 103 | exit(1); |
104 | } | 104 | } |
105 | 105 | ||
106 | // Returns whether all supplementary groups can be safely dropped | ||
107 | int check_can_drop_all_groups() { | ||
108 | static int can_drop_all_groups = -1; | ||
109 | |||
110 | // Avoid needlessly checking (and printing) things twice | ||
111 | if (can_drop_all_groups != -1) | ||
112 | goto out; | ||
113 | |||
114 | // nvidia cards require video group; ignore nogroups | ||
115 | if (access("/dev/nvidiactl", R_OK) == 0 && arg_no3d == 0) { | ||
116 | fwarning("NVIDIA card detected, nogroups command ignored\n"); | ||
117 | can_drop_all_groups = 0; | ||
118 | goto out; | ||
119 | } | ||
120 | |||
121 | /* When we are not sure that the system has working seat-based ACLs | ||
122 | * (e.g.: probably yes on (e)udev + (e)logind, probably not on eudev + | ||
123 | * seatd), supplementary groups (e.g.: audio and input) might be needed | ||
124 | * to avoid breakage (e.g.: audio or gamepads not working). See #4600 | ||
125 | * and #4603. | ||
126 | */ | ||
127 | if (access("/run/systemd/seats/", F_OK) != 0) { | ||
128 | fwarning("logind not detected, nogroups command ignored\n"); | ||
129 | can_drop_all_groups = 0; | ||
130 | goto out; | ||
131 | } | ||
132 | |||
133 | if (arg_debug) | ||
134 | fprintf(stderr, "nogroups command not ignored\n"); | ||
135 | can_drop_all_groups = 1; | ||
136 | |||
137 | out: | ||
138 | return can_drop_all_groups; | ||
139 | } | ||
140 | |||
106 | static int find_group(gid_t group, const gid_t *groups, int ngroups) { | 141 | static int find_group(gid_t group, const gid_t *groups, int ngroups) { |
107 | int i; | 142 | int i; |
108 | for (i = 0; i < ngroups; i++) { | 143 | for (i = 0; i < ngroups; i++) { |
@@ -141,6 +176,9 @@ static void clean_supplementary_groups(gid_t gid) { | |||
141 | if (rv == -1) | 176 | if (rv == -1) |
142 | goto clean_all; | 177 | goto clean_all; |
143 | 178 | ||
179 | if (arg_nogroups && check_can_drop_all_groups()) | ||
180 | goto clean_all; | ||
181 | |||
144 | // clean supplementary group list | 182 | // clean supplementary group list |
145 | gid_t new_groups[MAX_GROUPS]; | 183 | gid_t new_groups[MAX_GROUPS]; |
146 | int new_ngroups = 0; | 184 | int new_ngroups = 0; |
@@ -230,7 +268,7 @@ void drop_privs(int force_nogroups) { | |||
230 | if (arg_debug) | 268 | if (arg_debug) |
231 | printf("No supplementary groups\n"); | 269 | printf("No supplementary groups\n"); |
232 | } | 270 | } |
233 | else if (arg_noroot) | 271 | else if (arg_noroot || arg_nogroups) |
234 | clean_supplementary_groups(gid); | 272 | clean_supplementary_groups(gid); |
235 | 273 | ||
236 | // set uid/gid | 274 | // set uid/gid |