Skip to content
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

Privilege dropping code may not be working #4990

Closed
theopolis opened this issue Feb 8, 2020 · 2 comments · Fixed by #5001
Closed

Privilege dropping code may not be working #4990

theopolis opened this issue Feb 8, 2020 · 2 comments · Fixed by #5001
Labels
bug Not working as intended

Comments

@theopolis
Copy link
Contributor

It looks like the code to drop privileges may have been broken via commit 37f0e1f

That commit reverted the correct order from #911, which first drops the gid then the uid. If setuid is called first then the target user may not have the ability to setgid.

Additionally, this implementation should consider using initgroups as well (before dropping the uid) to drop supplemental groups from the source user and set the groups for the target user. If you do not drop supplemental groups the target user most likely retains privileges granted by those groups.

Heads up that I am raising this issue by performing a code review only. I have not run sway to verify the breakage.

@emersion
Copy link
Member

Good catch. Reference: https://wiki.sei.cmu.edu/confluence/display/c/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges

Would you be willing to submit a pull request to fix this?

Additionally, this implementation should consider using initgroups as well (before dropping the uid) to drop supplemental groups from the source user and set the groups for the target user. If you do not drop supplemental groups the target user most likely retains privileges granted by those groups.

This is not POSIX. We don't alter our supplementary group IDs before dropping root, so it shouldn't be necessary.

@emersion emersion added the bug Not working as intended label Feb 10, 2020
@theopolis
Copy link
Contributor Author

This is not POSIX. We don't alter our supplementary group IDs before dropping root, so it shouldn't be necessary

Makes sense!

I will try to submit a PR this week! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Not working as intended
Development

Successfully merging a pull request may close this issue.

2 participants