Swift doesn't drop all group privileges

Bug #909569 reported by David on 2011-12-29
262
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
High
Russell Bryant

Bug Description

Swift doesn't drop all group privileges. On modern linux it is insufficient to simply call setgid followed by setuid to drop privileges because the process will maintain the original user's extra groups. Instead something like setgroups or initgroups should be used. I will post more technical information later :-)

Thierry Carrez (ttx) wrote :

Setting to Incomplete pending more technical details

Changed in swift:
status: New → Incomplete
David (d--) wrote :

Sorry I forgot about this bug and will update it with details in a moment.

David (d--) wrote :

 In swift/common/utils.py the function drop_privileges can be found.

(line 464):
def drop_privileges(user):
    """
    Sets the userid/groupid of the current process, get session leader, etc.

    :param user: User name to change privileges to
    """
    user = pwd.getpwnam(user)
    os.setgid(user[3])
    os.setuid(user[2])
    try:
        os.setsid()
    except OSError:
        pass
    os.chdir('/') # in case you need to rmdir on where you started the daemon
    os.umask(0) # ensure files are created with the correct privileges

<--- On linux without dropping the starting user's groups it is possible for the drop_privileges method to not properly drop all intended groups(and keep group id of the user).
The drop_privileges method appears to be used by swift/common/daemon.py (on a quick inspection). I will have more time to look at this tomorrow and will do so then.

Thierry Carrez (ttx) wrote :

Adding John as Swift PTL.

John: for reference, could you give a quick rationale on why Swift starts as root and then drops privileges, rather than runs entirely under an unprivileged user ?

John Dickinson (notmyname) wrote :

If we want to support running swift on privileged ports (ie <1024), the swift processes have to start as a privileged user.

Pending more technical details, this does indeed seem to be a concern that needs to be addressed.

Thierry Carrez (ttx) wrote :

@John: If that's the only reason, how common is that ? Does Swift use a privileged port by default ? We need to weigh the convenience of allowing people to directly use a privileged port against the increased vulnerability exposure that results from having to drop privileges in code. If running off privileged ports is not common at all, then it might not be worth it. If that's a key feature, then I guess we'll have to live with it.

John Dickinson (notmyname) wrote :

Another reason to keep the drop privileges functionality is to allow the processes to be started from init scripts (which, at boot, are running as root).

I think we need to keep the functionality, but make sure we do it right (ie, fix this bug). I've marked this as "Low" importance, since it will require another vulnerability to exploit. I'd like to see this in the next swift release.

Changed in swift:
importance: Undecided → Low
Thierry Carrez (ttx) wrote :

@David: sounds like more a stengthening thing than an exploitable vulnerability. Agree on opening this up publicly ? (and targeting fix to Swift 1.4.6) ?

John Dickinson (notmyname) wrote :

added swift-core to bug subscribers

David (d--) wrote :

@ttx well sure. However, this probably warrants a CVE allocation.

Thierry Carrez (ttx) wrote :

If we are to get a CVE allocation (and an advisory), let's do this in private to get the stakeholders a bit of advance in patching.

@RusselB: interested in coordinating this one ?

Thierry Carrez (ttx) wrote :
John Dickinson (notmyname) wrote :

the patch needs to update the appropriate test as well.

test/unit/common/test_utils.py:test_drop_privileges

Russell Bryant (russellb) wrote :

@ttx: Yeah, I'll take this one.

Russell Bryant (russellb) wrote :

Here is another candidate patch. The changes from ttx's version are:

1) Check to be sure that the process is root before calling setgroups. If it's not root, it won't work.

2) Update the unit test to make sure that setgroups gets called. This also required making sure that the fake os module pretended the process was running as root.

John Dickinson (notmyname) wrote :

doesn't setgroups need to be added to line 58 (in the unpatched module) of test_utils.py in the MockOS class?

Russell Bryant (russellb) wrote :

It does not appear to be strictly necessary unless os.setgroups() gets used in a code path outside of test_drop_privileges. It's a good idea anyway, though. I have added it.

Russell Bryant (russellb) wrote :
David (d--) wrote :

I don't think this actually needs a special release or anything. I simply suggested that a cve be allocated because that should help distributions track this issue.

Thierry Carrez (ttx) wrote :

I'll let Russell decide how he wants to handle it.

On one hand we can certainly make this public (since it's really not exploitable, just an amplification issue), release the fix and ask for a CVE to cover it.

On the other we don't really have a clear process (yet) for getting a CVE without doing responsible disclosure and a security advisory.

I'm now slightly biased towards the former, as we'll have to formalize a type of vulnerability that warrants CVE but no embargoed disclosure at some point anyway... Do we want to make a security advisory for this ?

Russell Bryant (russellb) wrote :

I'm tempted to say that if an issue warrants a CVE, we should just follow the full process. It's hard to define clear criteria for something that warrants a CVE but doesn't warrant the full responsible disclosure process. It would likely be controversial at times when some people don't agree on which bucket we put an issue in.

In this particular case, even though there's no known way to exploit it, I think the issue is still worth calling out. If someone using Swift was using a frozen version and only cherry picking specific patches called out as a security issue, I would want them to grab this one just in case it helps reduce the impact of some future vulnerability.

Thierry Carrez (ttx) wrote :

@Russell: Agreed. And with Swift being much more mature than the rest of OpenStack, I think the expectations are set a bit higher in terms of what should be silently fixed and what should follow disclosure process.

Russell Bryant (russellb) wrote :

Ok, so we'll proceed with an advisory on this. Next steps:

1) We need approval from swift-core on the proposed patch.

2) I'll work on a vulnerability description and post the draft here for approval.

Russell Bryant (russellb) wrote :

Draft of vulnerability description.

-----

Title: Swift does not drop all group privileges
Impact: Low
Reporter: David Black
Products: Swift
Affects: All versions

Description:
David Black reported a vulnerability in Swift. Swift is started with root privileges and then switches to a less privileged user. The code that implements dropping root privileges did not properly clear the list of groups.

This issue is not exploitable by itself. However, it is considered a security issue as it could potentially result in an increase in what could be accomplished by exploiting another security vulnerability in Swift.

Robert Clark (robert-clark) wrote :

This issue is not exploitable by itself. However, it is considered a security issue as it could potentially result in an increase in what could be accomplished by exploiting another security vulnerability in Swift.

Seems a little clumsy to me, perhaps:
We do not believe that this vulnerability is directly exploitable but it may be possible to leverage it as part of an attack involving other vulnerabilities in Swift....

Russell Bryant (russellb) wrote :

Thanks, Robert. Draft rev2 of vulnerability description.

-----

Title: Swift does not drop all group privileges
Impact: Low
Reporter: David Black
Products: Swift
Affects: All versions

Description:
David Black reported a vulnerability in Swift. Swift is started with root privileges and then switches to a less privileged user. The code that implements dropping root privileges did not properly clear the list of groups.

We do not believe that this vulnerability is directly exploitable but it may be possible to leverage it as part of an attack involving other vulnerabilities in Swift.

Thierry Carrez (ttx) wrote :

I would say "This vulnerability is not directly exploitable", since any exploit would involve another, unknown, vulnerability. Otherwise looks good.

Waiting on swift-core members pre-approval of the patch.

Changed in swift:
importance: Low → Medium
status: Incomplete → In Progress
Thierry Carrez (ttx) on 2012-01-19
Changed in swift:
importance: Medium → High
Thierry Carrez (ttx) wrote :

@Swift-core: Please review proposed patch and comment here if it looks ok to you. We'll use those pre-approvals to fast-track the patch through Gerrit review when this bug is publicly disclosed, in order to limit the vulnerability window.

John Dickinson (notmyname) wrote :

lgtm

John Dickinson (notmyname) wrote :

If this is proposed and merged tomorrow (Feb 7), it can be included in swift 1.4.6 (being cut for QA on Feb 8).

Thierry Carrez (ttx) wrote :

@RussellB: I would be good to have in 1.4.6... Could you push the notification to the stakeholders list, explain that since it's not directly exploitable the fix will be proposed directly and included in 1.4.6 this week, and propose the fix in Swift today ?

Thierry Carrez (ttx) on 2012-02-07
Changed in swift:
assignee: nobody → Russell Bryant (russellb)
milestone: none → 1.4.6
Russell Bryant (russellb) wrote :

Notification to stakeholders sent, patch proposed in gerrit, and bug opened to the public

visibility: private → public
Thierry Carrez (ttx) wrote :

commit e90424e88bfaae17bf16f5c32b4d18deb5a6e71f

Changed in swift:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2012-02-09
Changed in swift:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers