dropping privileges and checking error returns

[Seth Arnold]

Hello, I'm conducting a super-quick review of oslo.privsep as part of the
Ubuntu Main Inclusion process.

I noticed a few odd things with some of the privilege dropping code that
may or may not represent bugs:

    def _drop_privs(self):
        try:
            # Keep current capabilities across setuid away from root.
            capabilities.set_keepcaps(True)

            if self.group is not None:
                try:
                    os.setgroups([])
                except OSError:
                    msg = _('Failed to remove supplemental groups')
                    LOG.critical(msg)
                    raise FailedToDropPrivileges(msg)

            if self.user is not None:
                setuid(self.user)

            if self.group is not None:
                setgid(self.group)

        finally:
            capabilities.set_keepcaps(False)

First, if the transition is to a non-root user, the only setgid()
transitions that will work are to the real group ID or saved set-group-ID.
If the setgid() and setuid() lines are swapped, all group IDs will work.
Is this intentional?

Second, I don't understand why supplementary groups aren't dropped always.
The 'is not None' check doesn't make sense to me. Why not drop the
supplementary groups unconditionally?

Third, and most troubling, the setuid() and setgid() functions throw
exceptions when they fail but this function ignores all exceptions. These
calls can fail and when they do, it can have catastrophic consequences.
The error returns from the system calls must be checked. Does oslo.privsep
die properly when these functions fail?

Thanks

[Seth Arnold (seth-arnold)]

I added a manual subscription to hopefully make these visible. If that didn't do it, I'll just open them public.

Thanks

[Tristan Cacqueray (tristan-cacqueray)]

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

The first 2 points looks valid, however the third one seems incorrect. The set[ug]id function does check for failure and will raise the FailedToDropPrivileges if something wrong happen. Thus it's the responsability of the caller to except this exception or be interrupted, which sounds fine.

As explained on bug 1628348, oslo.privsep isn't handled directly by the OpenStack Vulnerability Management Team, though we are happy to help here.

[Joshua Harlow (harlowja)]

Plugged in angus, hopefully he can chime in when he is active :)

[Seth Arnold (seth-arnold)]

Tristan, thanks for the review; I'm glad to hear I'm wrong about _drop_privs() eating the exceptions via the finally: block. (Now I'm curious what language I was thinking of.)

The other two issues don't worry me nearly as much as that did.

Thanks

[Angus Lees (gus)]

Seth: First, thanks again for the review - please continue to question anything that smells strange.

1.
The setgid and setuid operations being reversed is a good observation - I will flip them as you have suggested. I think the reason I haven't noticed this earlier is that this function preserves capabililities across set[gu]id, which in the common case of root->non-root means the setgid is performed with CAP_SETGID. It would indeed be better to flip the two operations as you have suggested and not implicitly require this capability.

2.
Supplementary groups aren't dropped because if self.group is unspecified (None), I don't want to call any operation that requires elevated capabilities (in this case CAP_SETGID again). This is used in unittests that leave user/group=None and assume this code can be executed with no special privilege. Another approach would be (as you suggest) to always drop supplementary groups, implicitly requiring CAP_SETGID, and modify tests to mock out (or otherwise disable) the setgroups call. I can follow this approach instead if we have concerns with the current code.

3.
As Tristan pointed out try/finally doesn't capture exceptions. I agree that would be a serious issue if true and you were right to be alarmed ;)

[Seth Arnold (seth-arnold)]

Excellent all around; I'm content, feel free to make public and close as you wish!

Thanks

[Tristan Cacqueray (tristan-cacqueray)]

According to the VMT taxonomy, this is a class D type of bug ( https://security.openstack.org/vmt-process.html#incident-report-taxonomy ).

[Ben Nemec (bnemec)]

Looks like the uid and gid calls are still in the same order, so I guess that part of this bug still needs addressing: https://github.com/openstack/oslo.privsep/blob/master/oslo_privsep/daemon.py#L384

Otherwise, it seems the other concerns have been addressed. Please reply if I'm mistaken about that.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/oslo.privsep/+/873550

[Max (maxlamprecht)]

I can reproduce the problem and have created a patch.

The following shows the problem well:

$ sudo python3
Python 3.10.6 (main, Nov 14 2022, 16:10:14) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os;
>>> os.setuid(1000)
>>> os.setgid(1000)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
PermissionError: [Errno 1] Operation not permitted

$ sudo python3
Python 3.10.6 (main, Nov 14 2022, 16:10:14) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os;
>>> os.setgid(1000)
>>> os.setuid(1000)

Patch is attached.

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Max <email address hidden>" on branch: master
Review: https://review.opendev.org/c/openstack/oslo.privsep/+/873550

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/oslo.privsep 3.2.0 release.