dropping privileges and checking error returns

Bug #1628360 reported by Seth Arnold
16
This bug affects 2 people
Affects Status Importance Assigned to Milestone
OpenStack Security Advisory
Won't Fix
Undecided
Unassigned
oslo.privsep
In Progress
Low
Unassigned

Bug Description

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

Revision history for this message
Seth Arnold (seth-arnold) wrote :

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

Thanks

Changed in ossa:
status: New → Incomplete
description: updated
Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote :

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.

Revision history for this message
Joshua Harlow (harlowja) wrote :

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

Revision history for this message
Seth Arnold (seth-arnold) wrote :

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

Revision history for this message
Angus Lees (gus) wrote :

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 ;)

Revision history for this message
Seth Arnold (seth-arnold) wrote :

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

Thanks

Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote :

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

description: updated
Changed in ossa:
status: Incomplete → Won't Fix
information type: Private Security → Public
Revision history for this message
Ben Nemec (bnemec) wrote :

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.

Changed in oslo.privsep:
status: New → Confirmed
importance: Undecided → Low
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to oslo.privsep (master)
Changed in oslo.privsep:
status: Confirmed → In Progress
Revision history for this message
Max (maxlamprecht) wrote :

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.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on oslo.privsep (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/oslo.privsep 3.2.0

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

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.