search

Enerccio / ewlc

Wayland compositor library - extended
MIT License
20 stars 3 forks source link

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

Open WLCIssuesBot opened 7 years ago

WLCIssuesBot commented 7 years ago

Issue by Fale Tuesday Sep 06, 2016 at 22:57 GMT Originally opened as https://github.com/Cloudef/wlc/issues/195

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

WLCIssuesBot commented 7 years ago

Comment by Cloudef Tuesday Sep 06, 2016 at 23:01 GMT

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

WLCIssuesBot commented 7 years ago

Comment by Fale Tuesday Sep 06, 2016 at 23:08 GMT

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

WLCIssuesBot commented 7 years ago

Comment by Cloudef Wednesday Sep 07, 2016 at 00:02 GMT

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?

WLCIssuesBot commented 7 years ago

Comment by opoplawski Wednesday Sep 07, 2016 at 19:31 GMT

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

WLCIssuesBot commented 7 years ago

Comment by Cloudef Wednesday Sep 07, 2016 at 20:03 GMT

Can those "supplementary groups" be set unwillingly?

WLCIssuesBot commented 7 years ago

Comment by opoplawski Wednesday Sep 07, 2016 at 20:13 GMT

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

WLCIssuesBot commented 7 years ago

Comment by Cloudef Wednesday Sep 07, 2016 at 20:31 GMT

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

WLCIssuesBot commented 7 years ago

Comment by Fale Wednesday Sep 28, 2016 at 22:00 GMT

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