Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Werkzeug incorrectly handles multiline headers #1080

Closed
ngaya-ll opened this issue Mar 10, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@ngaya-ll
Copy link

commented Mar 10, 2017

According to RFC 2616:

HTTP/1.1 header field values can be folded onto multiple lines if the continuation line begins with a space or horizontal tab. All linear white space, including folding, has the same semantics as SP. A recipient MAY replace any linear white space with a single SP before interpreting the field value or forwarding the message downstream.

However, werkzeug does not accept header values with newlines, even if they abide by this convention.

>>> import werkzeug
>>> werkzeug.Headers().add('foo', 'bar\n baz')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../venv/local/lib/python2.7/site-packages/werkzeug/datastructures.py", line 1136, in add
    self._validate_value(_value)
  File ".../venv/local/lib/python2.7/site-packages/werkzeug/datastructures.py", line 1143, in _validate_value
    raise ValueError('Detected newline in header value.  This is '
ValueError: Detected newline in header value.  This is a potential security problem

Also, this restriction is applied inconsistently.

>>> werkzeug.Headers([('foo', 'bar\n baz')])
Headers([('foo', 'bar\n baz')])

I ran into this issue when trying to write test cases relating to nginx forwarding of client certificates via headers, so there is a real use case for supporting this properly.

@untitaker

This comment has been minimized.

Copy link
Member

commented Mar 10, 2017

HTTP headers do not allow newlines. The section you're quoting talks about folding, which ought to remove newlines from the unfolded value. This:

"""foo
     bar"""

should get unfolded to something like this:

"foo bar"

Apart from that, Werkzeug doesn't parse HTTP at this level, this is the job of the WSGI server. The only reason it rejects newlines when parsing requests is to catch security issues.

@untitaker untitaker closed this Mar 10, 2017

@ngaya-ll

This comment has been minimized.

Copy link
Author

commented Mar 13, 2017

This ticket was motivated by the real-life behavior of Flask in development mode behind an nginx proxy forwarding client certs. With that setup, I observed newlines in the headers being passed to the application. But when I attempted to replicate this in a unit test I got the above ValueError when building the request headers.

I did a bit more research on this issue and found the following:

  • The HTTP spec (RFC 2616) states that newlines in headers MAY be replaced with a single space, not that they are guaranteed to be (see quote above).

  • The CGI spec (RFC 3875), which WSGI extends, requires newlines in request headers to be replaced:

    Similarly, a header field that spans multiple lines MUST be merged onto a single line.

  • The WSGI spec (PEP 333) also prohibits newlines in response headers:

    Each header_value must not include any control characters, including carriage returns or linefeeds, either embedded or at the end.

So this means that there are two bugs here:

  • The werkzeug development server does not correctly normalize folded strings in request headers.

  • The Headers object is inconsistent about accepting newlines in header values. Newlines are accepted in the constructor, but not in the add() method. It's probably better for the Headers object to remain permissive and perform the validation when actually constructing the WSGI response in BaseResponse. That way forbidding newlines in the constructor won't break compatibility with possibly non-conforming WSGI servers (such as werkzeug's own development server).

@untitaker

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

Fair enough. There's also #1070 which plays into this.

@untitaker untitaker reopened this Mar 14, 2017

@davidism

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

Fairly sure this was fixed with the fix for #1070. If not, please let me know with a reproducible example.

@davidism davidism closed this Jan 20, 2019

@ngaya-ll

This comment has been minimized.

Copy link
Author

commented Jan 21, 2019

@davidism As I mentioned in a previous comment, there are actually two bugs here, neither of which has been fixed on the current master branch.


The first bug involves how the werkzeug development server handles line-wrapped headers. It can be reproduced with the following server code, which prints the value of the X-Example header:

from werkzeug.serving import run_simple
from werkzeug.wrappers import Request, Response

def app(environ, start_response):
    request = Request(environ)
    print(repr(request.headers.get('X-Example')))
    response = Response(status=204)
    return response(environ, start_response)

run_simple('localhost', 8080, app)

We can then send it a request with a header spanning multiple lines:

GET / HTTP/1.1
Host: localhost:8080
Connection: close
X-Example: foo
 bar

Expected server output:
Header value is merged onto a single line

'foo bar'

Actual server output (Python 2):

----------------------------------------
Exception happened during processing of request from ('127.0.0.1', 57361)
Traceback (most recent call last):
  File "/usr/lib/python2.7/SocketServer.py", line 295, in _handle_request_noblock
    self.process_request(request, client_address)
  File "/usr/lib/python2.7/SocketServer.py", line 321, in process_request
    self.finish_request(request, client_address)
  File "/usr/lib/python2.7/SocketServer.py", line 334, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/lib/python2.7/SocketServer.py", line 649, in __init__
    self.handle()
  File "/home/.../venv/local/lib/python2.7/site-packages/werkzeug/serving.py", line 320, in handle
    rv = BaseHTTPRequestHandler.handle(self)
  File "/usr/lib/python2.7/BaseHTTPServer.py", line 340, in handle
    self.handle_one_request()
  File "/home/.../venv/local/lib/python2.7/site-packages/werkzeug/serving.py", line 355, in handle_one_request
    return self.run_wsgi()
  File "/home/.../venv/local/lib/python2.7/site-packages/werkzeug/serving.py", line 238, in run_wsgi
    self.environ = environ = self.make_environ()
  File "/home/.../venv/local/lib/python2.7/site-packages/werkzeug/serving.py", line 217, in make_environ
    for key, value in self.get_header_items():
  File "/home/.../venv/local/lib/python2.7/site-packages/werkzeug/serving.py", line 441, in get_header_items
    key, value = header[0:-2].split(":", 1)
ValueError: need more than 1 value to unpack
----------------------------------------

Actual server output (Python 3):
Header value contains newline characters disallowed by WSGI spec

'foo\r\n bar'

The second bug has to do with how the Headers object handles newline values.

>>> from werkzeug import Headers
>>> h1 = Headers([('X-Example', 'foo\r\n bar')])
>>> h2 = Headers()
>>> h2.add('X-Example', 'foo\r\n bar')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/.../venv3/lib/python3.6/site-packages/werkzeug/datastructures.py", line 1166, in add
    self._validate_value(_value)
  File "/home/.../venv3/lib/python3.6/site-packages/werkzeug/datastructures.py", line 1173, in _validate_value
    raise ValueError('Detected newline in header value.  This is '
ValueError: Detected newline in header value.  This is a potential security problem

Expected result:
Both operations should have the same result, whether a ValueError or success.

Actual result:
Headers constructor allows newlines while Headers.add() raises ValueError.

@davidism davidism reopened this Jan 22, 2019

@bertomaniac

This comment has been minimized.

Copy link

commented Mar 27, 2019

I was seeing the ValueError recently in one of our projects (Python 2.7 with Flask) which happens to sit behind an nginx proxy. I ended up reverting to 0.14.1 (overriding the version bundled with Flask) and my error went away. Thought I'd add since it seems the 0.15.x branch introduced this problem (or potentially created a new problem with how it was handling request headers).

------UPDATE-------:

I tracked down what headers we were passing and one of them was a multi-line cert in pem format i.e.:

SSL_CLIENT_CERT: -----BEGIN CERTIFICATE-----
    MIIFHzCCAwegAwIBAgICEDgwDQYJKoZIhvcNAQELBQAwajELMAkGA1UEBhMCdXMx
    GDAWBgNVBAoMD3Uucy4gZ292ZXJubWVudDEPMA0GA1UECwwGcGVvcGxlMQwwCgYD
    VQQLDANkYWUxEDAOBgNVBAsMB2NoaW1lcmExEDAOBgNVBAMMB0ludGVyQ0EwHhcN
    MTcwODMxMTUwMzEwWhcNMjcwODI5MTUwMzEwWjBwMQswCQYDVQQGEwJVUzEYMBYG
    A1UECgwPVS5TLiBHb3Zlcm5tZW50MRAwDgYDVQQLDAdjaGltZXJhMQwwCgYDVQQL
    ....  
    -----END CERTIFICATE-----

Our nginx server was configured like so:

proxy_set_header SSL_CLIENT_CERT $ssl_client_cert;

We should probably be using $ssl_client_escaped_cert instead of $ssl_client_cert (since this is deprecated anyway). Not sure if that change will fix the header parsing problem though.

Hoping this helps anyone else that is running into this problem. It appears that 0.15.1 doesn't properly handle a multi-line header like a PEM cert at this time with Python 2.7.

@aenglander

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

This is a 2.7 issue again caused by the header processing of the development server. I am adding the ability to process folding of headers to the 2.7 compatibility code for request headers.

aenglander added a commit to aenglander/werkzeug that referenced this issue May 6, 2019

Update the request header compatibility code for Python 2.7 to proper…
…ly fold headers persuant to RFC 2616.

Resolves pallets#1080

@davidism davidism closed this May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.