-
Notifications
You must be signed in to change notification settings - Fork 156
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.
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
Relinquish supplementary groups when dropping privileges by setgid/setuid #31
Comments
Also drop membership in any supplementary groups when changing group with group=. Fixes: #31 Based on bugreport and patch by Ingvar Hagelund.
Added in ef25784. Thanks! |
In the change in beta3, setgroups is only called before setgid. It should also be called before setuid. Let's say there is a group "system" which gives extra privileges. A confused sysadmin has added the root user to that group. If hitch is called with --user, but not --group. setgroups will not be called. hitch will start, setuid to the new user, but will keep the extra group "system", and keep extra privileges. Or am I wrong? There is something fishy going on with the groups anyhow, wich might or might not be related. Consider the following: [ingvar@lardal ~]$ hitch-openssl example.com.pem & [ingvar@lardal ~]$ ps -o pid,uid,euid,gid,egid,fgid -p Here we see hitch get effective user id and group id correctly. Now, try to run it with user/group specified: [ingvar@lardal ~]$ sudo hitch-openssl -u hitch -g hitch example.com.pem & [ingvar@lardal ~]$ ps -o pid,uid,euid,gid,egid,fgid -p So, the process is still running with effective user 0, that is root??!? Also, in beta3, the man page does not mention -g | --group, though the binary happily accepts that config. Ingvar |
A working patch that at least sets the correct gid here, please apply: http://users.linpro.no/ingvar/varnish/hitch-1.0.0-beta3.setgroup.patch Ingvar |
Hi, and thanks for following up on this. From what I can see, your output shows the process running with EUID==980, which I assume is your hitch user. I've added your patch for setgroups(), and here is new output for comparing with:
(yes, the parent process is supposed to run as root. it does not handle any connections.) |
"The CERT C Secure Coding Standard" advices to explicit relinquish supplementary groups when dropping privileges by setgid/setuid. This is discussed for example at http://ow.ly/OpPFH.
I'll make a pull request with a simple patch for this, byt just doing setgroups(0, NULL)
Ingvar
The text was updated successfully, but these errors were encountered: