Pre-auth COPY in versioned_writes can result in a successful COPY that wouldn't have been authorized

Bug #1562175 reported by Janie Richling
266
This bug affects 2 people
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Fix Released
Undecided
Unassigned
OpenStack Security Advisory
Won't Fix
Undecided
Unassigned
OpenStack Security Notes
Fix Released
Undecided
Vincenzo Di Somma

Bug Description

There is an issue in the versioning feature of swift (found in the latest on master - 2.6/2.7.0). Exploiting the vulnerability would not damage the cluster, but can have some undesirable behavior.

Scenario 1:
A user can cause an existing object in another account to be copied - when that user is not authorized for that account.
- user1 has access to tenant1
- user2 does not have access to tenant1
- user1 creates a versioned container ("versioned_container")
- user1 creates a container for those versions to be stored ("versions")
- user1 PUTs an object version in "versioned_container" called myobject with content "a"
- user2 PUTs an object version in "versioned_container" called myobject with content "b"
   - the "pre-authed" copy of content "a" to the "versions" container succeeds
   - the last PUT fails with Forbidden. (content "b" is not saved)
   - So, the end state is that user2 caused the latest myobject version to be copied to "versions",
      which user2 should not have access to.
     (Exploiting this, user2 can drive up the usage of User1 impacting consumption against the quota and billing costs)
- user2 PUTs an object version in "versioned_container" called myobject with content "c"
   - same thing as the last PUT attempt, but the timestamp in the filename of myobject in the "versions" container.
     (X-Timestamp is same in old obj listing - but when/if swift restores a version, the filename timestamp is used as X-Timestamp)
   - No new data is created here. The old object still has content "a".

Scenario 2:
Another flow involving the same issue which results in user1 getting unexpected results.
In this scenario, when user1 attempts to revert to a prior version of an object, it will not be that version.
Note there is a work-around, but the user must be very aware of the issue to detect what had happened.
- user1 PUTs myobject with content "a"
- user1 PUTs myobject with content "b", this moves content "a" to the versions container
- user2 PUTs myobject this moves content "b" to the versions container
- user1 DELETEs myobject; this moves content "b" to the versioned_container but User1 expects the content to be "a"
(Work-around: DELETE twice - then the original "a" would be in place - Note it was not lost.)

So in summary, the bug is that a user can cause bytes to go into the container were old versions are stored when this user does not have access to the associated account. Note that the user still cannot write to the formal location of this versioned object, and the user cannot influence the contents of the objects. There is also no way to write more bytes than the size of ONE copy of an authorized object. Note that the original object will always be preserved - and this does not allow an authorized user to remove any versions.

Suggestion on how this can be fixed:
Change to perform some authorization before the copy to old versions is made inside of _put_versioned_obj instead of the pre-authed request that is currently made. There is a comment in the code saying the pre_auth request is made in case the user has write access to the container, but not READ. It says this was allowed before middleware was in place. In this particular scenario, the user doesn't even have write access.

Tim Burke (1-tim-z)
Changed in swift:
status: New → Confirmed
Revision history for this message
Jeremy Stanley (fungi) 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.

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

@swift-coresec, any progress ?

Revision history for this message
Matthew Oliver (matt-0) wrote :

When putting the object that has a versioned container we are using a pre-authed request to move it before checking whether the user is authorised. On Deletes it's fine because we we attempt to auth before doing anything.

There are 2 ways we can solve this:

 1. We could either do the same for the PUT object. that is authenticate before we do anything. Or;
 2. just make sure we use make_subrequest rather then make_pre_authed_request, and the former copies the swift.authorize and so it gets authed at the proxy.

The 2nd option however means there would be a change in version_writes behaviour. The 2nd option would mean when giving someone the ACL to write a container that gets versioned, the user also needs an ACL on the versions_container also.

Because of that, he is a patch for option 1 with updated unit and functional tests. Like the DELETE method, this patch checks to see that the user has writes the container before pre-auth requesting to move the object in the container.

Revision history for this message
Matthew Oliver (matt-0) wrote :

wow, I must stilled be jetlagged, that file name is bad :P

Revision history for this message
Tim Burke (1-tim-z) wrote :

FWIW, versioned_writes has changed a good bit recently. While this bug is still present on master, any backport will be complicated by:

 * refactoring COPY as middleware [1] which both
   - seems likely to land before any fix here can be properly reviewed and
   - simplifies the authorization checks required, as we no longer need to think about COPY
 * decoupling versioned_writes from COPY [2]

Looking past mitaka toward a liberty backport, we'll also hit:

 * fixing the upgrade bug caused by reverse container listings [3]
 * using calendar.timegm instead of time.mktime [4] (mostly a problem because of test conflicts)
 * using reverse container listings [5]

The good news is these are all fairly well-contained to versioned_writes provided they are taken as a group. I am uncertain as to whether it would be better to backport them all as a chain or develop a de novo patch.

I've attached a patch to address this bug, with patchset 37 of [1] as the parent commit. This is done under my stated assumption that [1] will land before this fix; as a result it does not cover server-side copies (via either COPY-with-Destination or PUT-with-X-Copy-From) which would need to be considered if backports require new patches.

[1] https://review.openstack.org/#/c/156923/
[2] https://review.openstack.org/#/c/260179/
[3] https://review.openstack.org/#/c/299686/
[4] https://review.openstack.org/#/c/271542/
[5] https://review.openstack.org/#/c/234391/

Revision history for this message
Janie Richling (jrichli) wrote :

Using Matt's patch, I tested unittests, functests, probetests, and tested the case manually and saw that the version was no longer created on disk from the unauthorized user.
I used the same script that revealed the issue before to show that the issue has now been resolved.
nit: s/becuase/because/ in test_versioned_writes.py changes.

Revision history for this message
Matthew Oliver (matt-0) wrote : Re: [Bug 1562175] Re: Pre-auth COPY in versioned_writes can result in a successful COPY that wouldn't have been authorized
Download full text (5.2 KiB)

Thanks Janie, if it becomes an upstream patch I'm sure we can fix your nit
;)

On Fri, May 6, 2016 at 2:55 PM, Janie Richling <email address hidden> wrote:

> Using Matt's patch, I tested unittests, functests, probetests, and tested
> the case manually and saw that the version was no longer created on disk
> from the unauthorized user.
> I used the same script that revealed the issue before to show that the
> issue has now been resolved.
> nit: s/becuase/because/ in test_versioned_writes.py changes.
>
> --
> You received this bug notification because you are a member of Swift
> Core security contacts, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1562175
>
> Title:
> Pre-auth COPY in versioned_writes can result in a successful COPY that
> wouldn't have been authorized
>
> Status in OpenStack Security Advisory:
> Incomplete
> Status in OpenStack Object Storage (swift):
> Confirmed
>
> Bug description:
> This issue is being treated as a potential security risk under
> embargo. Please do not make any public mention of embargoed (private)
> security vulnerabilities before their coordinated publication by the
> OpenStack Vulnerability Management Team in the form of an official
> OpenStack Security Advisory. This includes discussion of the bug or
> associated fixes in public forums such as mailing lists, code review
> systems and bug trackers. Please also avoid private disclosure to
> other individuals not already approved for access to this information,
> and provide this same reminder to those who are made aware of the
> issue prior to publication. All discussion should remain confined to
> this private bug report, and any proposed fixes should be added to the
> bug as attachments.
>
> There is a potential security issue in the versioning feature of swift
> (found in the latest on master - 2.6/2.7.0). Exploiting the
> vulnerability would not damage the cluster, but can have some
> undesirable behavior.
>
> Scenario 1:
> A user can cause an existing object in another account to be copied -
> when that user is not authorized for that account.
> - user1 has access to tenant1
> - user2 does not have access to tenant1
> - user1 creates a versioned container ("versioned_container")
> - user1 creates a container for those versions to be stored ("versions")
> - user1 PUTs an object version in "versioned_container" called myobject
> with content "a"
> - user2 PUTs an object version in "versioned_container" called myobject
> with content "b"
> - the "pre-authed" copy of content "a" to the "versions" container
> succeeds
> - the last PUT fails with Forbidden. (content "b" is not saved)
> - So, the end state is that user2 caused the latest myobject version
> to be copied to "versions",
> which user2 should not have access to.
> (Exploiting this, user2 can drive up the usage of User1 impacting
> consumption against the quota and billing costs)
> - user2 PUTs an object version in "versioned_container" called myobject
> with content "c"
> - same thing as the last PUT attempt, but the timestamp in the
> filename of myobject in the "versions" con...

Read more...

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

Considering the backport complication, perhaps we could lift the embargo and submit the proposed fix to gerrit if that helps cherry-picking to liberty and mitaka.

Anyway, swift-coresec, please review above proposed patches.

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

@swift-coresec: any progress ?

Revision history for this message
Jeremy Stanley (fungi) wrote :

I've subscribed the Security Project core security reviewers to weigh in on the potential impact of making this bug public as suggested by Tristan in comment #8.

Revision history for this message
Travis McPeak (travis-mcpeak) wrote :

OK, trying to understand the impact. From the bug description:

"A user can cause an existing object in another account to be copied - when that user is not authorized for that account." Does this mean that there is a path for a user to be granted access to a container or object they shouldn't have? I didn't get that from reading the rest of the bug description, but if that's the case this seems pretty bad.

Not saying we don't want to fix in the open, just want us to have eyes wide open to WHAT we're fixing in the open.

Revision history for this message
Janie Richling (jrichli) wrote :

@Travis, No, I would not describe the consequence as "granting access to a container or object they shouldn't". To attempt to summarize, it is more like a user can *cause* a copy to be written into a container that it does not otherwise have access to. The user cannot control the contents of that object - apart from selecting which existing object to use.

The user can select a versioned object path that it does not have access to, request a put on that versioned object, and this request will execute the copy part of the request before it fails due to lack of permissions.

Revision history for this message
Travis McPeak (travis-mcpeak) wrote :

Thanks Janie, as far as I understand it this is likely a lower impact vulnerability and should be fine to fix publicly.

Revision history for this message
Janie Richling (jrichli) wrote :

I am not sure why the OpenStack Security Advisory status is at "incomplete". But going from what Travis has posted, I am going to make this public.

description: updated
Janie Richling (jrichli)
information type: Private Security → Public
Revision history for this message
Jeremy Stanley (fungi) wrote :

The OSSA task remains incomplete until it's determined that there is a backportable solution we'll be able to include in a public security advisory. If it only ends up fixed in master and is not backported to the current stable branches, the VMT will set the OSSA task to Won't Fix.

information type: Public → Public Security
Revision history for this message
Jeremy Stanley (fungi) wrote :

I've switched the report from Public to Public Security to indicate that it's still being tracked as a potential vulnerability until there is a definitive choice by the Swift devs as to whether they want to treat it as a vulnerability or merely a hardening opportunity.

Revision history for this message
Janie Richling (jrichli) wrote :

Thank you for explaining the status to me Jeremy. By the way, I should have mentioned in my last comment, I had also talked with John (notmyname) on this, and he suggested that it was indeed time set to "public". But I understand if you would like to communicate with him or other cores directly.

Revision history for this message
Janie Richling (jrichli) wrote :
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to swift (master)

Reviewed: https://review.openstack.org/357628
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=d2fc2614575b04fd9cab5ae589880b92eee9b186
Submitter: Jenkins
Branch: master

commit d2fc2614575b04fd9cab5ae589880b92eee9b186
Author: Matthew Oliver <email address hidden>
Date: Fri Aug 19 16:17:31 2016 +1000

    Authorise versioned write PUTs before copy

    Currently a versioned write PUT uses a pre-authed request to move
    it into the versioned container before checking whether the
    user is authorised. This can lead to some interesting behaviour
    whereby a user can select a versioned object path that it does not
    have access to, request a put on that versioned object, and this
    request will execute the copy part of the request before it fails
    due to lack of permissions.

    This patch changes the behaviour to be the same as versioned DELETE
    where the request is authorised before anything is moved.

    Change-Id: Ia8b92251718d10b1eb44a456f28d3d2569a30003
    Closes-Bug: #1562175

Changed in swift:
status: Confirmed → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to swift (feature/hummingbird)

Fix proposed to branch: feature/hummingbird
Review: https://review.openstack.org/363111

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to swift (feature/hummingbird)
Download full text (84.1 KiB)

Reviewed: https://review.openstack.org/363111
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=1ab2a296f58ae76aeffef9f3f0fb90e15358be27
Submitter: Jenkins
Branch: feature/hummingbird

commit 3b5850836c59c46f2507a7f62aceccf4c37e5d41
Author: gecong1973 <email address hidden>
Date: Tue Aug 30 15:08:49 2016 +0800

    Remove white space between print and ()

    There is a white space between print and ()
    in /tempauth.py, This patch fix it

    Change-Id: Id3493bdef12223aa3a2bffc200db8710f5949101

commit f88e7fc0da2ed6a63e0ea3c3459d80772b3068cd
Author: zheng yin <email address hidden>
Date: Mon Aug 29 20:21:44 2016 +0800

    Clarify test case in common/ring/test_builder

    They use a bare assertRaises(ValueError, ring.RingBuilder, *,*,*), but
    it's not clear which one raises which ValueError(), so I extend them to
    validate the error strings as well.

    Change-Id: I63280a9fc47ff678fe143e635046a0b402fd4506

commit d68b1bd6ddf44c5088e9d02dcb2f1b802c71411b
Author: zhufl <email address hidden>
Date: Mon Aug 29 14:31:27 2016 +0800

    Remove unnecessary tearDown

    This is to remove unnecessary tearDown to keep code clean.

    Change-Id: Ie70e40d6b55f379b0cc9bc372a35705462cade8b

commit d2fc2614575b04fd9cab5ae589880b92eee9b186
Author: Matthew Oliver <email address hidden>
Date: Fri Aug 19 16:17:31 2016 +1000

    Authorise versioned write PUTs before copy

    Currently a versioned write PUT uses a pre-authed request to move
    it into the versioned container before checking whether the
    user is authorised. This can lead to some interesting behaviour
    whereby a user can select a versioned object path that it does not
    have access to, request a put on that versioned object, and this
    request will execute the copy part of the request before it fails
    due to lack of permissions.

    This patch changes the behaviour to be the same as versioned DELETE
    where the request is authorised before anything is moved.

    Change-Id: Ia8b92251718d10b1eb44a456f28d3d2569a30003
    Closes-Bug: #1562175

commit c1ef6539b6ba9d2e4354c9cd2eec8a0195cdb19f
Author: Clay Gerrard <email address hidden>
Date: Thu Aug 25 11:00:49 2016 -0700

    add test for expirer processes == process

    This is a follow up from a change that improved the error message.

    Related-Change: I3d12b79470d122b2114f9ee486b15d381f290f95

    Change-Id: I093801f3516a60b298c13e2aa026c11c68a63792

commit 01477c78c1163822de41484e914a0736e622085b
Author: zheng yin <email address hidden>
Date: Thu Aug 25 15:37:42 2016 +0800

    Fix ValueError information in obj/expirer

    I fix error information in raise ValueError(...)
    For example:
        if a>=b:
            # It should be under below and not 'a must be less than or equal to b'
            raise ValueError('a must be less than b')

    Change-Id: I3d12b79470d122b2114f9ee486b15d381f290f95

commit b81f53b964fdb8f3b50dd369ce2e194ee4dbb0b7
Author: zheng yin <email address hidden>
Date: Tue Aug 23 14:26:47 2016 +0800

    Improve readability in the obj server's unit tests

    This change improves the reada...

tags: added: in-feature-hummingbird
Revision history for this message
Jeremy Stanley (fungi) wrote :

Given that this is mostly an accounting bug, giving the attacker some ability to trigger copies of an object in a container they don't control but no ability to actually alter the content, I'm curious if anyone feels this warrants a backport to supported stable branches (stable/liberty and stable/mitaka for now). If it's deemed severe and someone is willing to work on getting backports merged then I'll gladly draft an impact description and get the ball rolling on CVE assignment and subsequent advisory. If not, then this is more likely just a class B1 (or perhaps C1) in our taxonomy (in which case the OSSN editors may be interested in drafting a note about the associated risks). https://security.openstack.org/vmt-process.html#incident-report-taxonomy

I've added a new OSSN bugtask so we can get some input from the OSSN editors on which path makes more sense in this case.

Vincenzo Di Somma (vds)
Changed in ossn:
assignee: nobody → Vincenzo Di Somma (vds)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/swift 2.10.0

This issue was fixed in the openstack/swift 2.10.0 release.

Luke Hinds (lhinds)
Changed in ossn:
status: New → In Progress
Revision history for this message
Luke Hinds (lhinds) wrote :

Reading back over this, it seems there is no remediation action outside of the patch itself? Are there are any steps that need to be taken outside of applying the patch that we could recommend in the security note?

Revision history for this message
Janie Richling (jrichli) wrote :

Hi Luke. That is correct, no additional steps beyond applying the patch.

Revision history for this message
Luke Hinds (lhinds) wrote :

So a note is not a good fit here, and reading back Jeremy was more asking for feedback then a note being constructed just yet. As this looks like this will need an OSSA instead, perhaps the already authored text could still have some use in the OSSA?

https://review.openstack.org/#/c/396080/

Revision history for this message
Jeremy Stanley (fungi) wrote :

Yes, if someone wants to take up the task of backporting the fix (or some similar solution) to stable/newton, stable/mitaka and possibly stable/liberty (depending on whether you can get it done before liberty reaches EOL in a week), then we can issue a security advisory (OSSA). Given the apparent low severity of this issue however, I can completely understand if that's not a priority.

Revision history for this message
Luke Hinds (lhinds) wrote :

We could keep the current note around if its needed for an OSSA, but for now I will close the OSSN with fix commited.

Changed in ossn:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

This issue was fixed in the openstack/swift 2.10.0 release.

Luke Hinds (lhinds)
Changed in ossn:
status: Fix Committed → Fix Released
Revision history for this message
Jeremy Stanley (fungi) wrote :

This was fixed long enough ago that the vulnerable branches are no longer supported, so I'm marking our security advisory task Won't Fix.

Changed in ossa:
status: Incomplete → Won't Fix
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

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