Limited dir traversal in DiskFile()

[Thomas Biege]

And another one from Sebastian Krahmer / SUSE.

Sebastian Krahmer 2012-05-07 14:57:33 CEST
The DiskFile() that is constructed in POST/PUT requests etc.
uses user input to construct path names, where "device"
could be ../../../../etc and as a result any created files
are inside /etc or similar directories where it does not belong to.

Sebastian Krahmer 2012-05-16 10:50:49 CEST
(In reply to comment #10)
> re Comment 1:
>
> it can be triggered like this
> curl -X__init__
> "http://192.168.125.11:6000/device/replica/account/container/object"
> (note that the words are literals and do not need to be substituted)
>
> The object server simply sends back a 500 response with a backtrace and lives
> on.
> Note that the object server relies on the proxy server for authorization, so if
> one can access the port, I assume one can play at will.
>
> For example, overwriting a file:
>
> curl -H "x-timestamp: `date +%s`" -d hello
> "http://192.168.125.11:6000/sdb1/200871/AUTH_5e1fd658f9354a5681c174c5b5e5eb8b/Cont1/tucny-novell.png"

Also note that you could add '../' instead of some of these directory
components (sdb1 or AUTH_...) so you can overwrite files that dont
belong to swift or object storage at all.

[Thierry Carrez (ttx)]

I think there are two aspects to this.

(1) DiskFile could be strengthened
You can't really pass device="../../../etc/moo" due to the way split_path works. And the other path elements are not directly used to build a path. You *could* pass device=".." but I doubt that's exploitable due to how DiskFile ultimately maps to a precise file. That said, I agree that preventing even limited traversal is certainly a good thing to do. Keeping this bug open to track that.

(2) Object/Container/Account servers are blindly open to potentially-damaging requests. Anyone with direct access to those can create havoc, since the requests are not authenticated in any way. I think this is by design... suggestions on how to improve that and keep horizontal scalability ?

Depending on how discussion on that second part goes, we may open a separate bug to track that.

[Sebastian Krahmer (krahmer-p)]

Ok, I just want to make sure the passed parameters cant do any harm and are as stripped
down as possible.

For (2) if that's behind a proxy its probably OK, but still, some user that comes in via the frontend
shouldn't be able to damage other users files in the backend. But thats probably unrelated to "..".

[Thierry Carrez (ttx)]

For (2) I think I'll open a separate generic wishlist bug on Swift about no longer considering the internal network as fully trusted, since that's an open door to all types of escalations, but requires major changes in Swift design if we want to change that.

[Sebastian Krahmer (krahmer-p)]

ok

[Vincent Untz (vuntz)]

Even if it doesn't harm outside of device, I think it's fair to consider that someone using ".." (and even ".") in a request is trying to do something fishy. So I'd go with simply raising an invalid path error in split_path, which will likely help fix similar issues in other controllers.

[Vincent Untz (vuntz)]

Here's a simple patch that should cover the issue for directory traversals.

[Thierry Carrez (ttx)]

Since this is not directly exploitable (although still needs to be fixed!), I suggest we open this bug to the public and push the fix through the public channels as a welcome strengthening addition. Will do so if nobody objects in the next 24 hours.

[Thomas Biege (thomas-suse-deactivatedaccount)]

Made it public now.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

commit cc1907eef5a8ba7f6c6a53e5f44752ae88111e6b
Author: Vincent Untz <email address hidden>
Date: Tue Jun 19 12:11:06 2012 +0200

    Validate devices and partitions to avoid directory traversals

    swift.common.utils.validate_device_partition is a new function to check
    that a device and a partition are valid. This means that they don't
    contain '/' and are not '.' or '..'.

    We use this new function every time we get devices and partitions from a
    request.

    Fix bug 1005908

    Change-Id: Ia545ba8f877e85b4b576d6d7d09d890877ea6d34

[OpenStack Infra (hudson-openstack)]

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

commit faff4ae769f71f79f066774f959028a4391fe689
Author: Vincent Untz <email address hidden>
Date: Thu Jul 5 15:43:14 2012 +0200

    Forbid substrings based on a regexp in name_filter middleware

    In comments from https://review.openstack.org/8798 it was raised that it
    might make sense to forbid some substrings in the name_filter
    middleware.

    There is now a new forbidden_regexp option for the name_filter
    middleware to specify which substrings to forbid. The default is
    "/\./|/\.\./|/\.$|/\.\.$" (or in a non-regexp language: the /./ and /../
    substrings as well as strings ending with /. or /..).

    This can be useful for extra paranoia to avoid directory traversals
    (bug 1005908), or for more general filtering.

    Change-Id: I39bf2de45b9dc7d3ca4d350d24b3f2276e958a62
    DocImpact: new forbidden_regexp option for the name_filter middleware