Blacklisting characters in swift names

Bug #926048 reported by Eamonn O'Toole
264
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Fix Released
High
Eamonn O'Toole

Bug Description

This is the second of two bugs split-out of bug 903232. From that bug:

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

Revision history for this message
Eamonn O'Toole (eamonn-otoole) wrote :

From Thierry Carrez:

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

From John Dickinson:

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.

Thierry Carrez (ttx)
Changed in swift:
importance: Undecided → High
Revision history for this message
Thierry Carrez (ttx) wrote :

@John: any progress on that ? Would it be something you'd consider fixing in your next release ?

Revision history for this message
Eamonn O'Toole (eamonn-otoole) wrote :

@John @Thierry:

We'd be very happy to pitch in here and do the work. Just let us know.

Revision history for this message
Eamonn O'Toole (eamonn-otoole) wrote :

Just one update for the moment: we intend to implement the character blacklist as a separate filter, as John suggested earlier. The filter will default to off, again as John has requested.

Revision history for this message
Thierry Carrez (ttx) wrote :

@Eamonn: dounds very good to me. Ideally we would have this ready for inclusion in 1.4.7 (mid-March) so that it's part of the Essex common release.

Revision history for this message
Eamonn O'Toole (eamonn-otoole) wrote :

I'm putting the finishing touches to the filter and unit tests, and should submit the change tomorrow. The filter is really simple. It checks for a defined list of forbidden characters in the name. It also checks that the name is not greater than a defined length. And a user can just leave it out of the pipeline if they want to.

Revision history for this message
Eamonn O'Toole (eamonn-otoole) wrote :

@Thierry: I'm at the stage where I'm issuing a "git review" to submit my changes. I get the following message:

Via web-proxy.europe.hp.com:8080 -> review.openstack.org:29418

fatal: A Contributor Agreement must be completed before uploading:

  http://wiki.openstack.org/HowToContribute

fatal: The remote end hung up unexpectedly

So there isn't a record of my having signed the Contributor Agreement. I've done this, once a few months ago and twice today to no avail. Which means that I might have messed up along the way. Who should I contact about this?

Revision history for this message
Thierry Carrez (ttx) wrote :

I should be able to sort that one out.
Following http://wiki.openstack.org/HowToContribute, you should:

* Sign the CLA (I suspect you've done this)
* Add yourself to the contributors wiki (not done yet, looking at http://wiki.openstack.org/Contributors)
* Request membership at: https://launchpad.net/~openstack-cla/+join

If you do the last two steps, I should be able to clear your application to openstack-cla group, which is what the script tests.

Revision history for this message
Eamonn O'Toole (eamonn-otoole) wrote :

Thanks Thierry. I've done the last two steps.

Revision history for this message
Thierry Carrez (ttx) wrote :

I checked and your application to ~openstack-cla has been reviewed and accepted. You should be all set.

Changed in swift:
assignee: nobody → Eamonn O'Toole (eamonn-otoole)
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to swift (master)

Fix proposed to branch: master
Review: https://review.openstack.org/4918

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: master
Review: https://review.openstack.org/4919

Revision history for this message
Eamonn O'Toole (eamonn-otoole) wrote : Re: Security issue: character encoding

Thanks Thierry. For reference, this is the response that I got:

remote: New Changes:
remote: https://review.openstack.org/4918
remote: https://review.openstack.org/4919
remote: https://review.openstack.org/4920

And I just remembered that I forgot to include the bug number in the last of these. I'm really sorry about that. I don't know if I can fix it from my end.

The last change (4920) is really minor, I just added a couple of characters (< and >) to the default list of forbidden characters.

Revision history for this message
Thierry Carrez (ttx) wrote :

Now that code is publicly proposed and references the bug, I think it's better to open this bug publicly. Does everyone agree ?

Revision history for this message
Russell Bryant (russellb) wrote :

@ttx: Agreed on opening this up.

@eamonn-otoole: You can update the reviews to include the bug number. Just update your commit message using "git commit --amend" and then run "git review" again. As long as the "Change-Id" in the commit message stays the same, it will update the existing review and not open a new one.

Revision history for this message
Eamonn O'Toole (eamonn-otoole) wrote :

@russellb: Thanks, & done.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to swift (master)

Fix proposed to branch: master
Review: https://review.openstack.org/4925

Thierry Carrez (ttx)
summary: - Security issue: character encoding
+ Blacklisting characters in swift names
visibility: private → public
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: master
Review: https://review.openstack.org/4962

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

Reviewed: https://review.openstack.org/4918
Committed: http://github.com/openstack/swift/commit/cf1aa3c309a6fcb57755da6732226b494f0bf69b
Submitter: Jenkins
Branch: master

commit cf1aa3c309a6fcb57755da6732226b494f0bf69b
Author: Eamonn O'Toole <email address hidden>
Date: Fri Mar 2 11:23:17 2012 +0000

    Adds name_check filter

    Bug 926048.

    Filter checks path for user-defined forbidden characters, and for
    user-defined maximum length.

    Includes changes to reflect gholt's latest comments to Patch Set 4
    Also includes a change to a unit-test, renames another unit-test,
    and removes one superfluous unit-test.

    Added section to the example proxy config

    Fixed-up unit test pep8 warnings

    Changed error response code to 400 (Bad Request)

    Change-Id: Iace719d6a3d00fb3dda1b9d0bc185b8c4cbc00ca

Changed in swift:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in swift:
milestone: none → 1.4.8
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

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