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

Call setuid/setgid without setgroups may lead to a security issue #1783

Closed
badshah400 opened this issue Sep 20, 2022 · 3 comments
Closed

Call setuid/setgid without setgroups may lead to a security issue #1783

badshah400 opened this issue Sep 20, 2022 · 3 comments
Assignees

Comments

@badshah400
Copy link

When building rpm packages for openSUSE, rpmlint prints the following complaint:

[  130s] xrootd-fuse.x86_64: E: missing-call-to-setgroups-before-setuid /usr/bin/xrootdfs
[  130s] xrootd-libs.x86_64: E: missing-call-to-setgroups-before-setuid /usr/lib64/libXrdUtils.so.3.0.0
[  130s] xrootd-server.x86_64: E: missing-call-to-setgroups-before-setuid /usr/bin/cmsd
[  130s] xrootd-server.x86_64: E: missing-call-to-setgroups-before-setuid /usr/bin/xrootd
[  130s] This executable is calling setuid and setgid without setgroups or initgroups.
[  130s] This means it didn't relinquish all groups, and this would be a potential
[  130s] security issue.

I am not very conversant with particulars about setuid usage myself, but here is some documentation that may help:
https://wiki.sei.cmu.edu/confluence/plugins/servlet/mobile?contentId=87152295#content/view/87152295

@abh3
Copy link
Member

abh3 commented Sep 28, 2022

I looked at this code and it would appear to be a false warning. First, setgroups is never called as it does not need to be called (note the example link also never calls setgroups). The only requirement is that setgid is to be called before setuid and the code does that. So, is it the case he compiler only checks for setgroups and not setgid? I sure looks that way.

@abh3 abh3 self-assigned this Sep 28, 2022
@badshah400
Copy link
Author

I am not an expert, as I said earlier, but after reading about this a bit, it looks like a call to setgroups(0, NULL) is typically used before calls to setgid(). See, for example, https://github.com/libuv/libuv/blob/abe4f3d58d1f89f7b9c3092a917486832ceff7a2/src/unix/process.c#L354-L373.

If you are sure this is not necessary in this case, please let me know and we will suppress this false-positive warning for our builds. Thanks for looking into this.

@abh3
Copy link
Member

abh3 commented Sep 29, 2022

Indeed, this particular use of getgid() and setuid(0 specifically is used to temporarily change to the privileges afforded to a client logging in using a particular username and password (i.e. secpwd security). As such, we do not want to complete destroy the existing ancillary groups afforded to the server as they would be extremely difficult to recreate. See
https://security.stackexchange.com/questions/122141/always-setgroups-before-setuid

So, I am closing this as "not an error in the context used".. However, thank you for bringing this to our attention as it's aways good to review potential security issues.

@abh3 abh3 closed this as completed Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants