missing-call-to-setgroups-before-setuid

[DeadMozay]

This executable is calling setuid and setgid without setgroups or initgroups.
There is a high probability this means it didn't relinquish all groups, and
this would be a potential security issue to be fixed.

[ammen99]

Are you sure? The code has been taken from Sway: https://github.com/swaywm/sway/blob/master/sway/main.c#L188

The use-case here is setting the SUID bit. As far as I know, this only modifies the effective user/group id, nothing else. Isn't that right?

[DeadMozay]

Are you sure? The code has been taken from Sway: https://github.com/swaywm/sway/blob/master/sway/main.c#L188

The use-case here is setting the SUID bit. As far as I know, this only modifies the effective user/group id, nothing else. Isn't that right?

Yes I'm sure, there is a similar error in sway

[ammen99]

I am not 100% sure what a correct fix would be, but would gladly accept a PR.

[ericonr]

Relevant links:

• Missing call to setgroups before setuid in sway/main.c swaywm/sway#884
• Split setgid and setuid, add privilege check [RFC] swaywm/sway#911
• Cage: drop gid before uid cage-kiosk/cage#142

For possibly relevant code:

• https://github.com/void-linux/runit/blob/master/src/chpst.c#L80

@DeadMozay could you link to an open issue in Sway about this?

[ericonr]

There is a high probability this means it didn't relinquish all groups, and
this would be a potential security issue to be fixed.

Wouldn't this manifest itself as wrong groups output?

[ericonr]

Output of

#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>

int main(int argc, char * const argv[])
{
	uid_t
		uid = getuid(),
		euid = geteuid();
	gid_t
		gid = getgid(),
		egid = getegid();

	gid_t groups[128] = { 0 };
	int s = getgroups(128, groups);
	printf("groups (%d):", s);
	for (int i; i < s; i++) {
		printf(" %d", groups[i]);
	}
	puts("");

	printf("uid: %d, euid: %d\n", uid, euid);
	printf("gid: %d, egid: %d\n", gid, egid);
	return 0;
}

is

groups (11): 4 8 12 13 16 17 24 101 987 990 1000
uid: 1000, euid: 1000
gid: 1000, egid: 1000

as my own user and

groups (11): 4 8 12 13 16 17 24 101 987 990 1000
uid: 1000, euid: 0
gid: 1000, egid: 1000

when it's setuid.

What kind of situation makes setgroups necessary?

[DeadMozay]

I am not 100% sure what a correct fix would be, but would gladly accept a PR.

here are the details, POS36-C
https://wiki.sei.cmu.edu/confluence/plugins/servlet/mobile?contentId=87152295#content/view/87152295

[ericonr]

Under normal circumstances, setuid() and related calls do not alter the supplementary group IDs. However, a setuid-root program can alter its supplementary group IDs and then relinquish root privileges, in which case it maintains the supplementary group IDs but lacks the privilege necessary to relinquish them. Consequently, it is recommended that a program relinquish supplementary group IDs immediately before relinquishing root privileges.

Does Wayfire try to call setgroups anywhere to change the supplementary groups?

[ammen99]

Does Wayfire try to call setgroups anywhere to change the supplementary groups?

Definitely not, I didn't even know what that is before this issue was opened.

[ericonr]

Definitely not, I didn't even know what that is before this issue was opened.

So I don't see how calling setgroups before dropping privs is necessary. You'd have to reimplement the /etc/group parsing functionality in order to be able to call setgroups in the expected manner as well.

[ammen99]

Alright, @DeadMozay thanks for the link, but if you read carefully, we need to setgroups only if we modify supplementatry groups, which we don't.