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

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

Sep 06 '16 22:09 Fale

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

Sep 06 '16 23:09 Cloudef

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

Sep 06 '16 23:09 Fale

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?

Sep 07 '16 00:09 Cloudef

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

Sep 07 '16 19:09 opoplawski

Can those "supplementary groups" be set unwillingly?

Sep 07 '16 20:09 Cloudef

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

Sep 07 '16 20:09 opoplawski

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

Sep 07 '16 20:09 Cloudef

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

Sep 28 '16 22:09 Fale