Skip to content

Commit

Permalink
Fix security issue related to RFC 9112
Browse files Browse the repository at this point in the history
By default reject requests which contains headers `content-length` and
`transfer-encoding` at the same time.

That's not allowed by RFC 9112 and that could lead to
potential security attacks.

If the `reject_bad_request` option is turned off, then similar requests
will be processed even if they are bad formed. That will allow
compatibility with old server that can't be updated.

https://www.rfc-editor.org/rfc/rfc9112#section-6.1-15

This is an extract of the RFC:

> A server MAY reject a request that contains both Content-Length and
> Transfer-Encoding or process such a request in accordance with the
> Transfer-Encoding alone. Regardless, the server MUST close the
> connection after responding to such a request to avoid the potential
> attacks.

> A server or client that receives an HTTP/1.0 message containing
> a Transfer-Encoding header field MUST treat the message as if the
> framing is faulty, even if a Content-Length is present, and close the
> connection after processing the message. The message sender might have
> retained a portion of the message, in buffer, that could be
> misinterpreted by further use of the connection.

The following request would lead to this scenario:

```
POST / HTTP/1.1
Host: a.com
Transfer-Encoding: chunked
Content-Length: 0
Content-Type: application/x-"##-form-urlencoded
14
id=1'or sleep(1);###
0
```

With these changes, when this kind of request is received the connection is
closed and an error 400 is returned.

This scenario can be tested by using the following process:

1. run a wsgi server either by using the wsgi sample in official examples
   (http://eventlet.net/doc/examples.html#wsgi-server)
2. send the following HTTP request to the running server:
```
curl -d "param1=value1&param2=value2" -X POST -H 'Transfer-Encoding:
chunked' -H 'Content-Length: 0' --http1.1 http://0.0.0.0:8090 -i
```

The previous curl command display returned headers and status code.
You can observe that now, with these changes, bad requests are rejected.

These changes also remove `content-lenght` from the `chunk` tests
to avoid reflecting something that's not a bad practice.

This security issue was originally discovered by Keran Mu
(mkr22@mails.tsinghua.edu.cn) and Jianjun Chen
(jianjun@tsinghua.edu.cn), from Tsinghua University and
Zhongguancun Laboratory

Thanks to them for raising our attention about this security problem.
  • Loading branch information
4383 committed Dec 18, 2023
1 parent 33e05ca commit 1d314a2
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 7 deletions.
1 change: 1 addition & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Unreleased
==========

* Dropped support for Python 3.7 and earlier.
* Fix security issue in the wsgi module related to RFC 9112 https://github.com/eventlet/eventlet/pull/826

0.33.3
======
Expand Down
34 changes: 33 additions & 1 deletion eventlet/wsgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,25 @@ def readline(self, size=-1):


class HttpProtocol(BaseHTTPServer.BaseHTTPRequestHandler):
"""This class is used to handle the HTTP requests that arrive
at the server.
The handler will parse the request and the headers, then call a method
specific to the request type. The method name is constructed from the
request. For example, for the request method SPAM, the do_SPAM() method
will be called with no arguments. All of the relevant information is
stored in instance variables of the handler.
:param conn_state: The given connection status.
:param server: The server accessible by the request handler.
:param reject_bad_requests: Rejection policy.
If True, or not specified, queries defined as non-compliant,
by example with the RFC 9112, will automatically rejected.
Else, if False, even if a request is bad formed, the query
will be processed. Functionning this way, default to the
more-secure behavior, and allow working with old clients that
cannot be updated.
"""
protocol_version = 'HTTP/1.1'
minimum_chunk_size = MINIMUM_CHUNK_SIZE
capitalize_response_headers = True
Expand All @@ -340,11 +359,12 @@ class HttpProtocol(BaseHTTPServer.BaseHTTPRequestHandler):
# so before going back to unbuffered, remove any usage of `writelines`.
wbufsize = 16 << 10

def __init__(self, conn_state, server):
def __init__(self, conn_state, server, reject_bad_requests=True):
self.request = conn_state[1]
self.client_address = conn_state[0]
self.conn_state = conn_state
self.server = server
self.reject_bad_requests = reject_bad_requests

Check warning on line 367 in eventlet/wsgi.py

View check run for this annotation

Codecov / codecov/patch

eventlet/wsgi.py#L367

Added line #L367 was not covered by tests
self.setup()
try:
self.handle()
Expand Down Expand Up @@ -444,6 +464,7 @@ def handle_one_request(self):
self.rfile = orig_rfile

content_length = self.headers.get('content-length')
transfer_encoding = self.headers.get('transfer-encoding')

Check warning on line 467 in eventlet/wsgi.py

View check run for this annotation

Codecov / codecov/patch

eventlet/wsgi.py#L467

Added line #L467 was not covered by tests
if content_length is not None:
try:
if int(content_length) < 0:
Expand All @@ -456,6 +477,17 @@ def handle_one_request(self):
self.close_connection = 1
return

if transfer_encoding is not None:
if reject_bad_requests:
msg = b"Content-Length and Transfer-Encoding are not allowed together\n"
self.wfile.write(

Check warning on line 483 in eventlet/wsgi.py

View check run for this annotation

Codecov / codecov/patch

eventlet/wsgi.py#L482-L483

Added lines #L482 - L483 were not covered by tests
b"HTTP/1.0 400 Bad Request\r\n"
b"Connection: close\r\n"
b"Content-Length: %d\r\n"
b"\r\n%s" % (len(msg), msg))
self.close_connection = 1
return

Check warning on line 489 in eventlet/wsgi.py

View check run for this annotation

Codecov / codecov/patch

eventlet/wsgi.py#L488-L489

Added lines #L488 - L489 were not covered by tests

self.environ = self.get_environ()
self.application = self.server.app
try:
Expand Down
28 changes: 22 additions & 6 deletions tests/wsgi_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1922,6 +1922,22 @@ def test_close_idle_connections(self):
except Exception:
assert False, self.logfile.getvalue()

def test_rfc9112_reject_bad_request(self):
# (hberaud): Transfer-Encoding and Content-Length in the
# same headerare not allowed by rfc9112.
# Requests containing both headers MAY be rejected to
# avoid potential attack.
self.site.application = use_write
sock = eventlet.connect(self.server_addr)
sock.send(
b'GET / HTTP/1.1\r\n'
b'Transfer-Encoding: chunked\r\n'
b'Content-Length: 0'
b'Host: localhost\r\n'
b'\r\n')
result = read_http(sock)
self.assertRaises(ConnectionClosed, read_http, sock)


def read_headers(sock):
fd = sock.makefile('rb')
Expand Down Expand Up @@ -2102,8 +2118,8 @@ def test_dirt(self):

def test_chunked_readline(self):
body = self.body()
req = "POST /lines HTTP/1.1\r\nContent-Length: %s\r\n" \
"transfer-encoding: Chunked\r\n\r\n%s" % (len(body), body)
req = "POST /lines HTTP/1.1\r\n" \
"transfer-encoding: Chunked\r\n\r\n%s" % (body)

fd = self.connect()
fd.sendall(req.encode())
Expand All @@ -2112,8 +2128,8 @@ def test_chunked_readline(self):

def test_chunked_readline_from_input(self):
body = self.body()
req = "POST /readline HTTP/1.1\r\nContent-Length: %s\r\n" \
"transfer-encoding: Chunked\r\n\r\n%s" % (len(body), body)
req = "POST /readline HTTP/1.1\r\n" \
"transfer-encoding: Chunked\r\n\r\n%s" % (body)

fd = self.connect()
fd.sendall(req.encode())
Expand All @@ -2122,8 +2138,8 @@ def test_chunked_readline_from_input(self):

def test_chunked_readlines_from_input(self):
body = self.body()
req = "POST /readlines HTTP/1.1\r\nContent-Length: %s\r\n" \
"transfer-encoding: Chunked\r\n\r\n%s" % (len(body), body)
req = "POST /readlines HTTP/1.1\r\n" \
"transfer-encoding: Chunked\r\n\r\n%s" % (body)

fd = self.connect()
fd.sendall(req.encode())
Expand Down

0 comments on commit 1d314a2

Please sign in to comment.