Security issues

Hello Eamonn, thanks for reporting those issues.

As a first step, for confirmation of the issues, I added John Dickinson, the technical lead for Swift, to the list of people that can access this bug. Could you post your proposed patch as an attachment to this bug ? Then it will be discussed by a few Swift core developers, then we'll coordinate a public release with the downstream distributions and public cloud providers (and get CVEs assigned for those).

From the description of the issues so far they appear rather serious, so please don't leak information about these issues beyond this bug discussion, we can add more people to this bug subscriber list if more access is needed.

Fixes and tests attached.

-Eamonn.

Thanks for your input and interest. This morning I had a good discussion with the other swift core devs about this. The summary is that we'd be happy to review a patch for this. However, below is a more general response.

I agree that while this isn't an issue with swift directly, it could be an issue with people who use swift. I acknowledge the issue. But if someone is displaying or using user-generated content (including URLs), it must be sanitized. This is not unique to swift, of course. Not sanitizing user-generated content is a bad practice.

Secondly, adding a filter to entity names in swift can limit data portability. For example, if HP were to limit names and Rackspace does not, then Rackspace customers can't migrate to HP (but HP customers could easily migrate to Rackspace).

Also, adding in filtering to swift implies some level of liability. A provider enabling this feature would have to be careful with what is promised and implied. Where does it end? You can get to pretty silly extremes like adding virus scanning to each object.

All that being said, I think there are two ways to implement this, both of which we'd be happy to review for inclusion into swift. One is to add disallowed characters, substrings, or patterns into swift.common.constraints and then filet of PUTs. The other is to have a filter middleware that looks at each request for name validity. Either way, the feature must be toggle-able and default to off.

Overall, the swift core devs don't think this is a major issue that needs to be solved in swift itself. However, if you'd like to submit a patch, we wouldn't be opposed to seeing this sort of functionality available. We'd prefer to see the middleware patch rather than a constraints patch.

For easier consumption, here are the proposed changes as a diff with 1.4.3, which looks like the version the fixes were applied to...

I'm still a bit confused with your description of the impact.

> command injection (quotation marks allowed as part of folder name)
This sounds like an issue that would be triggered in client tools that do not escape characters, not in Swift. Do you confirm ? If yes, then I agree with John that it sounds like an optional additional layer of security rather than a vulnerability in Swift.

> risk of Swift storage corruption (account and container servers don't check input parameters for length, quotation marks)
Could you elaborate on that one ? I'm failing to grasp the attack vector and exact impact.

> negative content length (req.content_length isn't checked to see if it is < -1)
What is the impact of this one ? How can it be exploited and do what effect ?

> This sounds like an issue that would be triggered in client tools that do not escape characters, not in Swift. Do you confirm ? If yes, then I agree with John that it sounds like an optional additional layer of security rather than a vulnerability in Swift.

I have to disagree here. A service cannot rely on its clients to sanitize inputs, that is the responsibility of the service itself. If a service allows any request (from any client) that causes the service to perform actions that are not part of intended operations, then it is a vulnerability in the service.

John:

We agree with your assessment that these changes are best implemented as a separate filter. We will work on this, although it may take a while before me have something ready to submit.

Thierry:

I'm not a security expert so I've asked our security team to provide answers to your questions, which I'll supply as soon as I get them.

-Eamonn.

@Jarret: in that precise case, the client would never "cause the service to perform actions that are not part of intended operations".

The issue we are talking about is that Swift will happily store objects with a name like "; rm -rf /". If a client retrieves that object, he gets a file named "; rm -rf /", which is completely alright. However you *could* imagine that a vulnerability exists in one of the tools that will manipulate the resulting file on the client side, and would end up executing that as a shell command...

My position (as well as John's) is that you can't as a server protect your client systems against all dumb things client tools can do if *they* have blatant vulnerabilities. It's not a vulnerability in Swift. You /can/, however, optionally reduce the range of characters allowed in object names, as a deployment option.

Ahh understood, my misunderstanding.

@Eamonn: any news from "your security team" ?

Hi Thierry,

Many apologies but no news at present. I understand that some members of our security team have been in contact with some of the Swift principals on this topic. I'll keep pushing.

-Eamonn.

Should this just be closed out until there is further input? It appears that until then, Thierry and John's position is that there are no changes to be made here to resolve a vulnerability. For what it's worth, I agree with their assessment, as well.

I've gotten a response from our security team, and I need to subscribe Jason Hullinger from this team to this bug. I'm just waiting on his details.

I've just subscribed Jason Hullinger to this bug.

I think there are valid points on both sides. On one hand I think Thierry is correct in:

"This sounds like an issue that would be triggered in client tools that do not escape characters, not in Swift. Do you confirm ? If yes, then I agree with John that it sounds like an optional additional layer of security rather than a vulnerability in Swift."

I mostly agree that this should be an optional addition to Swift, and that in itself it is not a vulnerability within Swift. However, there are other circumstances where blacklisting certain characters or character ranges may be needed. For example, currently Swift allows any character for the name of an object or container, including control characters such as 0x01. When Swift outputs a container listing in XML it does so as XML 1.0 and prints out the literal character for 0x01 (start of heading). This will break nearly all XML 1.0 parsers because most control characters are not allowed in XML 1.0. See Character Ranges: http://www.w3.org/TR/REC-xml/#charsets In this example the problem isn't caused by the client but rather the output from Swift that will cause the error in the XML parser on the client. Additionally there could also be other ranges that a Swift provider may not wish to support due to back end systems or Python not being able to handler certain Unicode ranges.

Regarding the negative length of the Content-Length header: this could cause a denial of service because the service would never be able to read until '-1' bytes and the server would be forced to timeout. Of course this is really no different than a user sending a POST with a Content-Length of 1 and never sending any data also causing a timeout. However, the difference here is that this could potentially be used by another yet to be found or in the future to be introduced vulnerability such as an integer overflow where having a negative Content-Length could potentially cause a greater vulnerability to occur. Since it's invalid to have a negative content length and it has the potential to cause problems later it would be best to fix it now.

A fix is certainly required, I consider the XML character abuse to be particularly worrisome right now.

I'd like to see the XML issue fixed as a priority, content length can probably be rolled into the normal release cycle.

As this is an old bug now, I'll bring Russel in for some fresh eyes.

@Jason: thanks for the precisions. I'd agree that some minimal filtering (including control characters) would be a good default. We could have three settings: "all" (CloudFiles current deployment), "safe" (all but a few problematic characters) and "paranoid" (base64 or something), with default to "safe".

That doesn't really address the migration issue -- though a conservatively-crafted "safe" mode should be mostly compatible with "all". It might be better to not have a "paranoid" mode, in order to encourage data portability.

@John: thoughts ?

A few thoughts:

1) This report should be broken up into separate issues so each can be disclosed and patched independently.

2) We are also looking in to ways to address the XML response. There are a few options. We'd like to return a 1.0 version but with the 1.1 encoding of characters (like S3, at least the last time we tested it). However, any number of solutions can be provided with middleware on a particular deployment. I do not want to encourage a fracturing of functionality in the codebase that prevents different deployments from working together (eg filtering would prevent HP and Rackspace deployments from using container sync between the clusters). If a particular deployer needs to have specific restrictions, those things should be managed in their own middleware and not included in the swift codebase.

3) The Content-Length issue should be resolved by returning an error (Content-Length required). SInce we're using an evented server, it won't cause any DoS issues.

I've split this bug in two, and filed separate bugs: See Bug #926048 and Bug #926046.

Marking this one invalid, let's continue the discussion on the other two bugs.