Redirect crashes if path contains CRLF

[leplatrem]

With the following example:

from pyramid.httpexceptions import HTTPTemporaryRedirect
from pyramid.config import Configurator
from wsgiref.simple_server import make_server


def redirect(request):
    path = '/v1' + request.matchdict['path']
    raise HTTPTemporaryRedirect(path)


def main(**settings):
    config = Configurator(settings=settings)
    config.add_route(name='redirect', pattern=r'/{path:(?!v1).*}')
    config.add_view(view=redirect, route_name='redirect')
    return config.make_wsgi_app()

if __name__ == '__main__':
    server = make_server('0.0.0.0', 8080, main())
    server.serve_forever()

• pyramid==1.7.3
• WebOb==1.6.2

If I pass some CRLF character in the path, the app crashes:

>>> import requests
>>> requests.get("http://localhost:8080/crlftest%0DSet-Cookie:test%3Dtest%3Bdomain%3D.yelp.com")
<Response [500]>

Traceback (most recent call last):
  File "/usr/lib/python2.7/wsgiref/handlers.py", line 85, in run
    self.result = application(self.environ, self.start_response)
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/router.py", line 236, in __call__
    response = self.invoke_subrequest(request, use_tweens=True)
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/router.py", line 211, in invoke_subrequest
    response = handle_request(request)
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/tweens.py", line 62, in excview_tween
    reraise(*attrs['exc_info'])
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/tweens.py", line 22, in excview_tween
    response = handler(request)
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/router.py", line 158, in handle_request
    view_name
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/view.py", line 547, in _call_view
    response = view_callable(context, request)
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/viewderivers.py", line 413, in viewresult_to_response
    result = view(context, request)
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/viewderivers.py", line 147, in _requestonly_view
    response = view(request)
  File "redirect.py", line 8, in redirect
    raise HTTPTemporaryRedirect(path)
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/httpexceptions.py", line 493, in __init__
    body_template=body_template, location=location, **kw)
  File "/path/to/.venv/local/lib/python2.7/site-packages/pyramid/httpexceptions.py", line 221, in __init__
    Response.__init__(self, status=status, **kw)
  File "/path/to/.venv/local/lib/python2.7/site-packages/webob/response.py", line 153, in __init__
    setattr(self, name, value)
  File "/path/to/.venv/local/lib/python2.7/site-packages/webob/descriptors.py", line 142, in fset
    raise ValueError('Header value may not contain control characters')
ValueError: Header value may not contain control characters

I could fix my route pattern with something like /{path:(?!v1)[^\r]*} but don't you think the URL should be truncated on CRLF when passed as redirect?

I would be happy to work on a patch if you think it's relevant. Otherwise don't hesitate to close the issue.

[tseaver]

@leplatrem Thanks for the report! I think this is a WebOb issue, similar to Pylons/webob#231. ISTM that the ideal response to such a request would be a "400 Bad Request".

[mmerickel]

You are passing user input directly into a http response without sanitizing the input. Historically this has never really been webob's stance to handle these things automatically. The best you could ask for is maybe a custom exception that webob would raise and you would have to handle and turn into a 4xx.

[digitalresistor]

In this case WebOb is handling it. It's telling you you are setting a header to an invalid value because it contains control characters.

It won't truncate things for you automatically though... that would IMHO be bad.

@tseaver That issue you linked is not similar at all. That is upon touching request. This issue here is related to Response.

There were security seat belts added in header manipulation here: Pylons/webob#229 to avoid being vulnerable to HTTP Response splitting (which is exactly what the user above is being protected against).

Now even if this were not caught by WebOb, and we would allow the setting of the Location header with a CRLF in it, it would be caught by Waitress or mod_wsgi or others that protect against that. Waitress's protection is located here: Pylons/waitress#117 and Pylons/waitress#124

Should WebOb raise a different error rather than ValueError? I don't think so. The accepted way of assigning the location is:

resp.location = '/something'

Setting an attribute should raise a ValueError if it is an invalid value, no? Could the exception classes (in both WebOb and Pyramid) handle this better when they set the location and an exception is raised, probably, but I don't think it is WebOb's duty to replace ValueError with another exception type.

---

This is not a WebOb or Pyramid issue. The location parameter passed to HTTPRedirection and sub-classes should be sanitized, and it should conform to what is allowed in the HTTP spec for a Location: header. WebOb does the bare minimum to make sure that there are no control characters in the headers to avoid the HTTP header splitting issue (which I added because a real life project out there did the same thing as OP and was thus vulnerable).

[digitalresistor]

https://www.owasp.org/index.php/HTTP_Response_Splitting

[mmerickel]

I can't see a scenario in which I don't close this ticket right now. Anyone wanna make an argument / offer a possible solution? To me this is simply not our responsibility to sanitize user input. I think it's great that we're even validating it to avoid the response splitting exploit.

[ztane]

redirect requires an URL, and that is no URL there.

[digitalresistor]

@ztane That is incorrect. Redirect can either be a full URL, or it can be a path. WebOb will make it a full URL on the way out the door.

[ztane]

Path, or a relative URL? A relative URL is still a URL., but something with unescaped CRLF's is not. And BTW, as per https://tools.ietf.org/html/rfc7231, a relative URL is now allowed in Location.