[OSSA 2015-006] unauthorized delete from container with x-version-location (CVE-2015-1856)

[clayg]

The handling of object versions wrt container ACL's has been an area of interest lately and some questionable authorization behaviors have come to light:

https://etherpad.openstack.org/p/object_version_and_ACL_use_cases

Unfortunately I just discovered another one that actually seems damaging. Ability to destroy data without write access.

Any authenticated used can overwrite the most recent versions of any versioned object who's name is known in a container with the X-Versions-Location metadata field set if they have listing access to the x-versions-location container - regardless of their write authorization to the container. Basically if you can list an x-versions-location container you can overwrite all the current data in the source container (if you know it's name) with old copies even if you don't have write (or read) access to the source container.

Basically we're creating a preauthorized COPY from from the x-versions-location container (assuming the user has listing access to the x-versions-location container, and an old version exists) without checking to see if authenticated user has write access to the destination.

Script to reproduce the problem is attached.

Possible patch to follow.

[clayg (clay-gerrard)]

I think we can call the authorize callback for the original DELETE request before handling the version case to ensure the request is authorized to DELETE objects in the source container.

Then call the authorize callback again for the fall-through DELETE to the x-versions-location object (which may fail independently if the user doesn't have write access to the target.

[Christian Schwede (cschwede)]

Thanks Clay for the investigation and the proposed fix!

I was able to reproduce this issue, and also to fix it with your second patch.

I'm currently writing a functional test and would like to add it afterwards if you don't mind.

[clayg (clay-gerrard)]

good morning!

yeah of course, a unittest wouldn't be so terrible either :P

But honestly I'm more interested in just moving forward with Thiago's work [1] - I just think it'll just be harder to backport unfortunately (if we decide this is a for realzy security thing that people care about).

attaching the fix to go with Thiago's change

1. https://review.openstack.org/#/c/134347

[Christian Schwede (cschwede)]

Yes, backporting isn't that nice, but in this case I would prefer to get this fix merged more sooner than later.

And the fix itself is only a few lines, should be ok to merge it afterwards in Thiagos patch?

I attached a functional test that basically does the same as your shell script - fails on master, passes with your fix.

[Alistair Coles (alistair-coles)]

Confirmed and agree this is a security concern.

Clay's patch (this one https://bugs.launchpad.net/swift/+bug/1430645/+attachment/4340823/+files/more-version-auth.patch) fixes for me running keystone auth and passed existing functional and unit tests so +1 from me.

As an aside, I did end up confused by subsequent functional test failures - see: https://review.openstack.org/163385

[Tristan Cacqueray (tristan-cacqueray)]

Thanks for the report! I've added and confirmed the OSSA task.

Here is impact description draft #1:

Title: Unauthorized delete of versioned Swift object
Reporter: Clay Gerrard (Rackspace)
Products: Swift
Affects: up to version 2.2.2

Description:
Clay Gerrard from Rackspace reported a vulnerability in Swift object versioning. An authenticated user can delete the most recent version of any versioned object who's name is known if the user have listing access to the x-versions-location container. Only Swift setups with allow_version setting are affected.

[Jeremy Stanley (fungi)]

The discussion in https://review.openstack.org/134347 is sort of skirting this vulnerability, just need to remain careful to keep it under wraps until we settle on a disclosure date. I recommend subscribing Thiago to this bug report so that we're all on the same page.

Assuming this is something that the Swift developers want fixed for currently supported stable branches, we'll need some sort of backports conforming to our stable branch maintenance requirements. If this ends up only being fixed in the master branch instead then we would forgo any advisory and just switch the bug to public once the determination is made.

[clayg (clay-gerrard)]

Tristan Cacqueray,

I'm currently employed @ SwiftStack and we like to know how I can update the source from which you got that out-dated information.

Jeremy Stanley,

We can do that? That's a totally great idea. I totally did that.

[Alistair Coles (alistair-coles)]

@Christian thanks for working on a test. I applied your patch from https://bugs.launchpad.net/swift/+bug/1430645/comments/5
but see another test fail - must be some test coupling, perhaps you have overridden the default account in the test env?

```
swift@saio-1:~/swift$ nosetests ./test/functional/tests.py:TestObjectVersioning -m test_versioning_
SKIPPING FUNCTIONAL TESTS SPECIFIC TO AUTH VERSION 3
SKIPPING FUNCTIONAL TESTS SPECIFIC TO SERVICE TOKENS
test_versioning_check_acl (test.functional.tests.TestObjectVersioning) ... ok
test_versioning_dlo (test.functional.tests.TestObjectVersioning) ... FAIL

======================================================================
FAIL: test_versioning_dlo (test.functional.tests.TestObjectVersioning)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/swift/swift/test/functional/tests.py", line 2550, in test_versioning_dlo
    self.assertEqual(3, versions_container.info()['object_count'])
AssertionError: 3 != 4
    '3 != 4' = '%s != %s' % (safe_repr(3), safe_repr(4))
    '3 != 4' = self._formatMessage('3 != 4', '3 != 4')
>> raise self.failureException('3 != 4')

----------------------------------------------------------------------
Ran 2 tests in 64.834s

FAILED (failures=1)
```

[Tristan Cacqueray (tristan-cacqueray)]

Clay, I pulled that info from your openstack community member profile ( https://www.openstack.org/profile/ ).

Other than that, is the impact description accurate enough ?

Title: Unauthorized delete of versioned Swift object
Reporter: Clay Gerrard (SwiftStack)
Products: Swift
Affects: up to version 2.2.2

Description:
Clay Gerrard from SwiftStack reported a vulnerability in Swift object versioning. An authenticated user can delete the most recent version of any versioned object who's name is known if the user have listing access to the x-versions-location container. Only Swift setups with allow_version setting are affected.

[Christian Schwede (cschwede)]

@Alistair: yes, you're right - the reason is that each test uses the same container, but that container is not cleared between the tests and there is still one object stored after running the new test. Fixed in the attachment.

[John Dickinson (notmyname)]

Tristan, is there a mailing list for notifying deployers before this is made public?

I'm working on the timing for the release of this.

[Tristan Cacqueray (tristan-cacqueray)]

John, for bugs under embargo, we do notify stakeholders before disclosure, it's part of the vulnerability management process. Though there is no mailing list, it's a simple recipient list maintained by the VMT.

However for this bug we still need:
* patch (for master and impacted stable branchs) to be reviewed and approved
* impact description to be approved
* CVE (to be requested with the approved impact description)

Then we can move on to choosing a proper disclosure date and send the advance notification.

[John Dickinson (notmyname)]

Thanks for the info.

Yes, i know there's some other stuff to complete. But I wanted to understand what happens. I'd prefer that (once the other things have happened) we let the affected parties know, then release a couple of weeks after that so they can have time to patch.

I'm working on the other parts now.

> On Mar 12, 2015, at 11:28 AM, Tristan Cacqueray <email address hidden> wrote:
>
> John, for bugs under embargo, we do notify stakeholders before
> disclosure, it's part of the vulnerability management process. Though
> there is no mailing list, it's a simple recipient list maintained by the
> VMT.
>
> However for this bug we still need:
> * patch (for master and impacted stable branchs) to be reviewed and approved
> * impact description to be approved
> * CVE (to be requested with the approved impact description)
>
> Then we can move on to choosing a proper disclosure date and send the
> advance notification.
>
> --
> You received this bug notification because you are subscribed to
> OpenStack Object Storage (swift).
> https://bugs.launchpad.net/bugs/1430645
>
> Title:
> unauthorized delete from container with x-version-location
>
> Status in OpenStack Security Advisories:
> Confirmed
> 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 as to the bug as attachments.
> --
>
> The handling of object versions wrt container ACL's has been an area
> of interest lately and some questionable authorization behaviors have
> come to light:
>
> https://etherpad.openstack.org/p/object_version_and_ACL_use_cases
>
> Unfortunately I just discovered another one that actually seems
> damaging. Ability to destroy data without write access.
>
> Any authenticated used can overwrite the most recent versions of any
> versioned object who's name is known in a container with the X
> -Versions-Location metadata field set if they have listing access to
> the x-versions-location container - regardless of their write
> authorization to the container. Basically if you can list an x
> -versions-location container you can overwrite all the current data in
> the source container (if you know it's name) with old copies even if
> you don't have write (or read) access to the source container.
>
> Basically we're creating a preauthorized COPY from from the x
> -versions-location container (assuming the user has listing access to
> the x-versions-location contain...

[Christian Schwede (cschwede)]

I'm adding part of the review for the versioned write refactoring patch here to keep it private for now.

Thiago, thanks for updating the patch ( https://review.openstack.org/#/c/134347). I have one question: in https://review.openstack.org/#/c/134347/25/test/functional/tests.py line 2738 ff. we should ensure that none of the versioned objects itself is deleted (because account2 has no write permission there). So account2 might be able to delete the original object (which needs to be discussed if there is no write access to the version container), but the test needs to ensure nothing is deleted in the version container. Or am I wrong?

[Thiago da Silva (thiagodasilva)]

On Fri, 2015-03-13 at 16:16 +0000, Christian Schwede wrote:
> I'm adding part of the review for the versioned write refactoring patch
> here to keep it private for now.
>
> Thiago, thanks for updating the patch (
> https://review.openstack.org/#/c/134347). I have one question: in
> https://review.openstack.org/#/c/134347/25/test/functional/tests.py line
> 2738 ff. we should ensure that none of the versioned objects itself is
> deleted (because account2 has no write permission there). So account2
> might be able to delete the original object (which needs to be discussed
> if there is no write access to the version container), but the test
> needs to ensure nothing is deleted in the version container. Or am I
> wrong?
>
Yes, this is correct. I'm adding a test to make sure account2 cannot
delete a previous version of the object (i.e., directly on the versioned
container), but is able to delete the latest object from the "source"
container, because he has write access to it.
In addition, I also added a third user with no access whatsoever to test
deleting from either container.

[John Dickinson (notmyname)]

Here is a combined (fix+tests) against master. If this patch is ok, I'll also create the necessary backport patches.

The only outstanding question I have is the unittests. The test I write counts the number of authorize calls. It fails on master and passes here, but I'm not sure if there is a better way to check the functionality instead of just a call count.

You can apply the patch with `git apply fixver.patch`

[Alistair Coles (alistair-coles)]

-1 for the patch attached to #18

I'm still seeing test coupling: if test_versioning_check_acl fails then it leaves an object in ther versions ocntainer. SInce all tests in this class use the same ocntainers, that stray object causes test_versioning_dlo to fail when asserting object count in the versions container.

See attached modified patch:

I think we need a tearDown method that will clear out the containers between tests.

Also, if test_versioning_check_acl assertion on ResponseError being raises fails, the second account token remains set inthe connection, causing subsequent operations to fail, including teardown. So, wrap request with a try/finally.

Added another unit test suggestion.

[Alistair Coles (alistair-coles)]

Here's the patch!

[Darrell Bishop (darrellb)]

+2 for the latest patch; works like a champ and the tests look fine to me (not too worried about the authorize-call-count-as-proxy bit).

[John Dickinson (notmyname)]

I'm +2 on the last patch as well (Alistair's in #20).

[Christian Schwede (cschwede)]

I'm also +2 on the updated patch from Alistair.

[Tristan Cacqueray (tristan-cacqueray)]

Can we get a properly formated patch (using git patch) in order to keep authorship and have a proper commit message embedded ?

Also the patch fails to apply on stable/icehouse. Can you post separate patchs for Icehouse and Juno too ?

Finally, can someone approve the impact description proposed in comment #11 ?

If we can get these done by the end of the week, we could have a disclosure date set to:
2015-04-08, 1500UTC

Just in time before kilo-rc... what do you think ?

[John Dickinson (notmyname)]

I'll work on stable backports later this week.

For the description, how do we reference the versions affected? I need to check on that, too, this week. (Versioned writes were introduced in 1.5.0, but I don't know yet if the issue goes back that far.)

Now about the disclosure date: Does that give enough time for deployers to patch and deploy before it's announced?

[Tristan Cacqueray (tristan-cacqueray)]

Thanks!

For the description, by default we only reference supported stable version, assuming older version are vulnerable. Though if you can date when the bug was introduced, then it worth a mention in the affected line, e.g.:

Affects: 1.5.0 version through 2.2.2

About the disclosure date, we usually give stakeholders 3-5 business days, excluding Monday/Friday and holiday periods... When is the next version of swift supposed to be released ?

[Alistair Coles (alistair-coles)]

formatted patch attached, exact same code changes as the one i posted in #20

[Thierry Carrez (ttx)]

Please review attached patch on the bug so that we can communicate it (and its backports) to the downstream stakeholders, in time for matching the Swift RC1 !

[Christian Schwede (cschwede)]

Latest patch (#27 from Alistair) is fine with me. +1

[Alistair Coles (alistair-coles)]

In case its not obvious, I'm +1 for patch at #27

[Alistair Coles (alistair-coles)]

If I did this right this patch is for stable/juno, based on commit 70e35c6

[Alistair Coles (alistair-coles)]

and here's one for stable/icehouse

[John Dickinson (notmyname)]

juno version looks good

[John Dickinson (notmyname)]

icehouse version looks good

[Tristan Cacqueray (tristan-cacqueray)]

John, I think the impact description in comment #11 is accurate enough to request a CVE, we can get a more precise affected versions for the final OSSA document.

Can someone please approve it.

[John Dickinson (notmyname)]

The description in #11 is accurate

[Tristan Cacqueray (tristan-cacqueray)]

Proposed disclosure date/time:
2015-04-14, 1500UTC

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/juno
Review: https://review.openstack.org/173363

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/173366

[Tristan Cacqueray (tristan-cacqueray)]

Master (kilo) review is: https://review.openstack.org/173361/

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/173366
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=f6525758ab2456d688430699338993439597a789
Submitter: Jenkins
Branch: stable/icehouse

commit f6525758ab2456d688430699338993439597a789
Author: Alistair Coles <email address hidden>
Date: Fri Apr 3 18:46:20 2015 +0100

    Prevent unauthorized delete in versioned container

    An authenticated user can delete the most recent version of any
    versioned object who's name is known if the user has listing access
    to the x-versions-location container. Only Swift setups with
    allow_version setting are affected.

    This patch closes this bug, tracked as CVE-2015-1856.

    Co-Authored-By: Clay Gerrard <email address hidden>
    Co-Authored-By: Christian Schwede <email address hidden>
    Co-Authored-By: Alistair Coles <email address hidden>

    Closes-Bug: 1430645

    Change-Id: Icde494a9a2c851034813cbc3855a20335b643f09

[OpenStack Infra (hudson-openstack)]

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

commit dd9d97458ea007024220a78dba8dd663e8b425d7
Author: John Dickinson <email address hidden>
Date: Fri Mar 20 10:17:25 2015 +0000

    Prevent unauthorized delete in versioned container

    An authenticated user can delete the most recent version of any
    versioned object who's name is known if the user has listing access
    to the x-versions-location container. Only Swift setups with
    allow_version setting are affected.

    This patch closes this bug, tracked as CVE-2015-1856

    Co-Authored-By: Clay Gerrard <email address hidden>
    Co-Authored-By: Christian Schwede <email address hidden>
    Co-Authored-By: Alistair Coles <email address hidden>

    Closes-Bug: 1430645
    Change-Id: Ibacc7413afe7cb6f77d92e5941dcfdf4768ffa18

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/173363
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=85afe9316570855c87ea731d0627f6f8f2b73264
Submitter: Jenkins
Branch: stable/juno

commit 85afe9316570855c87ea731d0627f6f8f2b73264
Author: Alistair Coles <email address hidden>
Date: Fri Apr 3 17:05:36 2015 +0100

    Prevent unauthorized delete in versioned container

    An authenticated user can delete the most recent version of any
    versioned object who's name is known if the user has listing access
    to the x-versions-location container. Only Swift setups with
    allow_version setting are affected.

    This patch closes this bug, tracked as CVE-2015-1856.

    Co-Authored-By: Clay Gerrard <email address hidden>
    Co-Authored-By: Christian Schwede <email address hidden>
    Co-Authored-By: Alistair Coles <email address hidden>

    Closes-Bug: 1430645

    Change-Id: I74448c12bc4d4cd07d4300f452cf3dd6f66ca70a

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: feature/crypto
Review: https://review.openstack.org/175866

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/175866
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=5bb7c286ebb4a54e4d2bd5a02845644d1c651183
Submitter: Jenkins
Branch: feature/crypto

commit e440d6aed8a40848584767ed36811bf09c738838
Author: Kota Tsuyuzaki <email address hidden>
Date: Wed Apr 15 11:25:13 2015 -0700

    Fix best response to return correct status

    Current best response could return "503 Internal Server Error".
    However, "503" means "Service Unavailable". (The status int of
    Internal Server Error is 500)

    This patch fixes the response status as "503 Service Unavailable"

    Change-Id: I88b8c52c26b19e9e76ba3375f1e16ced555ed54c

commit 57011d5699d49a47ae89073ff27b39140ab4d652
Author: Ricardo Ferreira <email address hidden>
Date: Thu Mar 12 23:18:33 2015 +0000

    More user-friendly output for object metadata

    Split out system, user and other metadata in swift-object-info. Print
    every position line by line instead of raw dict representation, so it
    would be easier to parse with tools such as grep.

    Co-Authored-By: Ricardo Ferreira <email address hidden>
    Co-Authored-By: Kamil Rykowski <email address hidden>

    Change-Id: Ia78da518c18f7e26016700aee87efb534fbd2040
    Closes-Bug: #1428866

commit a162c2bdd7be12daa29dd07230f84efcaf1cab37
Author: OpenStack Proposal Bot <email address hidden>
Date: Thu Apr 16 06:06:35 2015 +0000

    Imported Translations from Transifex

    For more information about this automatic import see:
    https://wiki.openstack.org/wiki/Translations/Infrastructure

    Change-Id: I48ba06f4801ff2d7856d67e74d2b1f76c550fcf4

commit 52b102163e48dc04a6a492a3430efa1f7778d7ec
Author: Clay Gerrard <email address hidden>
Date: Wed Apr 15 15:31:06 2015 -0700

    Don't apply the wrong Etag validation to rebuilt fragments

    Because of the object-server's interaction with ssync sender's
    X-Backend-Replication-Headers when a object (or fragment archive) is
    pushed unmodified to another node it's ETag value is duped into the
    recieving ends metadata as Etag. This interacts poorly with the
    reconstructor's RebuildingECDiskFileStream which can not know ahead of
    time the ETag of the fragment archive being rebuilt.

    Don't send the Etag from the local source fragment archive being used as
    the basis for the rebuilt fragent archive's metadata along to ssync.

    Change-Id: Ie59ad93a67a7f439c9a84cd9cff31540f97f334a

commit 46bd6716ffae28aef53f15af170fd2df01b49843
Author: Kota Tsuyuzaki <email address hidden>
Date: Tue Apr 14 23:22:14 2015 -0700

    Small minor refactor on ec diskfile

    To be more helpful, add an inline comment and remove
    unnecessary assignment.

    Change-Id: Ia9c6993dfa03c238736955de8b0f5c1a7d5d1b65

commit 193fe9a5f906a2344bb5d328ad55b881e4086caa
Author: Lorcan <email address hidden>
Date: Wed Apr 15 11:32:32 2015 +0100

    Update swift-recon doc with more options

    The swit-recon tool has had several functional additions
    added recently but not all of these have been added to the docs.

    This change add...