possible HTTP header injection

[Thomas Biege]

Hi,
here is another one from Sebastian. This one wasn't tested and just an idea of an attack vector. What do you think about it?
Thanks!

Sebastian Krahmer 2012-05-16 10:24:54 CEST
I am not very familar with the GreenPile and web framework that the
proxy is using, but it seems like the proxy handler
unquotes a lot of user input from HTTP request:

class ObjectController(Controller):
    """WSGI controller for object requests."""
    server_type = _('Object')

    def __init__(self, app, account_name, container_name, object_name,
                 **kwargs):
        Controller.__init__(self, app)
        self.account_name = unquote(account_name)
        self.container_name = unquote(container_name)
        self.object_name = unquote(object_name)
[...]

And later on uses these _names unquoted and passes it to new HTTP
requests at various places, for example
in PUT requests via path_info and many more.

I expect that unquote() makes \r\n from %0d%0a etc, so eventually
you can inject your own headers in the newly formed request?
(depends on what the framework does), so you can pass any
HTTP request through the proxy to swift backend and trigger above
bugs.

[Thierry Carrez (ttx)]

Hmm, I don't think this vector is valid.
Even though you can inject \r\n in webob's PATH_INFO, it's not used as-is in real request headers, it gets quoted before it's put to good use:

>>> a='/%0d%0a'
>>> b=urllib.unquote(a)
>>> r=webob.Request.blank('i will be overridden by env', environ={})
>>> r.environ['PATH_INFO']=b
>>> r.path_info
'/\r\n'
>>> r.url
'http://localhost/%0D%0A'

So you can't inject \r\n in the resulting request ?

[Sebastian Krahmer (krahmer-p)]

If r.path or r.url is used, you might be right (I am not a webob expert though).

But I am not so certain that this is always the case:

[...]
 resp2 = self.GETorHEAD_base(req, _('Object'), partition,
                    self.iter_nodes(partition, nodes, self.app.object_ring),
                    req.path_info, self.app.object_ring.replica_count)
[...]
is using path_info which is passed as is to http_connect().

Additionally, some of the unquote()d strings are quote()d when building a new header
and some are not. Is it possible to somehow affect trans_id which is passed unquoted to
the header?

[Thierry Carrez (ttx)]

Looking into http_connect, it urllib.quotes the path before using it:
path = quote('/' + device + '/' + str(partition) + path)

On the headers side, they should definitely be a
This definitely should be audited a bit more completely to make sure all cases are covered.

[Thierry Carrez (ttx)]

Oops. I meant:

On the headers side, they should definitely be all quoted before being passed to http_connect, or all be unquoted and have http_connect quote them. So a bit more auditing here can't hurt.

[Thierry Carrez (ttx)]

Added openstack auditors at openstack-ossg so that they can spend a few audit cycles on that.

[Bryan D. Payne (bdpayne)]

Looks like this could potentially be a big problem. The real question is if the resulting values are being quoted everywhere that they are used. Without going through the rest of the code, it is hard to know if this is something that is exploitable today. A code audit seems in order.

In the long run, we should really change how the values are being unquoted. That seems like the wrong way to go about it as it burdens users of this data to get it right every time.

[gholt (gholt)]

Considering folks have been using spaces in their object names for quite some time, and I believe \r\n (I know I've seen \r and \n separately, don't remember if together) all of these would normally (meaning non-exploit) would've caused a bunch of 400 Bad Requests, I don't think this is an issue at all.

However, a actual working exploit would be wonderful with this and any security report -- especially if it would only take 10-15 minutes to do.

[Thierry Carrez (ttx)]

@gholt: Like I explained in previous comments, there is no clear vulnerability for you to fix here, just an area of the code that could use some extra security auditing, and/or code changes to be sure to always quote/unquote rather than relying on the function caller to do so.

Let's see if the auditors can come up with a specific exploitable case first -- if they can't, we'll open up this bug and potentially strengthen the functions around that in public patches.

[John Dickinson (notmyname)]

Since there has been no activity on this bug report for nearly 5 months, and because Swift has been refactored to remove webob, and because there was never any example exploit code produced, I'm closing this bug.

If there is a demonstrable error, please report it. If not, a bug report of "generally look out for X" is not useful.

[Thierry Carrez (ttx)]

Sebastian, Thomas: are you OK with making this bug public ? It's now an openstack-secaudit task, as something that needs more analysis.

[Thomas Biege (thomas-suse-deactivatedaccount)]

Hi Thierry,
I am fine with it.

Thanks,
Thomas

[Thierry Carrez (ttx)]

Public now