search

Cloudef / wlc

High-level Wayland compositor library
MIT License
331 stars 58 forks source link

Missing call to setgroups before setuid in src/session/fd.c #195

Open Fale opened 8 years ago

Fale commented 8 years ago

In https://github.com/Cloudef/wlc/blob/master/src/session/fd.c#L568 there is a call to setgroups before setuids. There is a high probability this means it didn't relinquish all groups, and this would be a potential security issue to be fixed. See https://www.securecoding.cert.org/confluence/display/c/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges

Cloudef commented 8 years ago

I don't get it, setgid is called before setuid as it should?

Fale commented 8 years ago

AFAIK if you have two conditions in the same if there is no guarantee they will be executed in the order they are written into. A patch of an identical situation for example can be found on https://build.opensuse.org/package/view_file/server:monitoring/ntop/POS36-C.patch?expand=1

Cloudef commented 8 years ago

That should not be true at all, short-circuiting and evaluation order should be required by both C and C++. Are you sure they did not just want to handle the error cases separately?

opoplawski commented 8 years ago

I believe the issue is that you are not calling setgroups() to relinquish any supplementary groups before calling setuid().

Cloudef commented 8 years ago

Can those "supplementary groups" be set unwillingly?

opoplawski commented 8 years ago

Not sure what you mean by unwillingly, but a process will have all of the supplemental groups that a user belongs to by default.

Cloudef commented 8 years ago

Oh, I see. That makes sense and setgroups(0, NULL); should drop them I guess?

Fale commented 7 years ago

FYI: https://github.com/SirCmpwn/sway/pull/911