Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix keeping certain groups with nogroups #4732

Merged
merged 3 commits into from
Dec 8, 2021
Merged

Fix keeping certain groups with nogroups #4732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28d3091
util.c: Rename nogroups to force_nogroups on drop_privs
kmk3 Nov 30, 2021
be5970c
Fix duplicated fwarning warnings
kmk3 Dec 1, 2021
7abce0b
Fix keeping certain groups with nogroups
kmk3 Nov 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/firejail/firejail.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
96 changes: 49 additions & 47 deletions src/firejail/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down 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/profile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/firejail/sandbox.c
View file
Open in desktop
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
49 changes: 44 additions & 5 deletions src/firejail/util.c
View file
Open in desktop
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;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@topimiettinen on Dec 2:

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.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Just moving the comment made things look a bit crowded, so I added a few
more gotos instead.

/* 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 @@ -215,21 +253,22 @@ static void clean_supplementary_groups(gid_t gid) {


// 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
Expand Down