-
Notifications
You must be signed in to change notification settings - Fork 320
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
Fix security issue related to RFC 9112 #826
Conversation
287f8b5
to
236a5f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the RFC:
Early implementations of Transfer-Encoding would occasionally send both a chunked transfer coding for message framing and an estimated Content-Length header field for use by progress bars. This is why Transfer-Encoding is defined as overriding Content-Length, as opposed to them being mutually incompatible.
Given that, should we maybe make this in some way configurable, but defaulting to the more-secure behavior? That would allow us an escape hatch for working with old clients that cannot be updated (for example, because their source code may have been lost), particularly if the operator and/or application developer know that the service will not be forwarding any messages.
I'd also like to note that since we're relying on stdlib's HTTP parsing, there are still cases where a client may send both headers but we don't 400. See also: python/cpython#81274 and #574 (which unfortunately I never properly followed up on).
That said, I agree, the danger is certainly real.
tests/wsgi_test.py
Outdated
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 MUST be rejected to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"MAY"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
eventlet/wsgi.py
Outdated
if transfer_encoding is not None: | ||
self.wfile.write( | ||
b"HTTP/1.0 400 Bad Request - rfc9112 Content-Length and " | ||
b"Transfer-Encoding are not allowed together\r\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a long status line -- any reason not to do something more like
msg = b"Content-Length and Transfer-Encoding are not allowed together\n"
self.wfile.write(
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))
? Maybe even throw in a Content-Type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, you are right, the body of the request is more suited to forward error details. Will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a subsidiary observation, personally I prefer using HTTP/1.0 according to the max_http_version
option documentation:
Set to "HTTP/1.0" to make the server pretend it only supports HTTP 1.0. This can help with applications or clients that don't behave properly using HTTP 1.1.
However, during my development I seen that, in responses, the HTTP protocole is hardcoded and sometime the version is set to HTTP 1.0
(the majority of the time) and sometime to HTTP 1.1
:
- https://github.com/eventlet/eventlet/blob/master/eventlet/wsgi.py#L746
- https://github.com/eventlet/eventlet/blob/master/eventlet/wsgi.py#L28
- https://github.com/eventlet/eventlet/blob/master/eventlet/wsgi.py#L433
- https://github.com/eventlet/eventlet/blob/master/eventlet/wsgi.py#L439
- https://github.com/eventlet/eventlet/blob/master/eventlet/wsgi.py#L454
- and so on...
Still, according to the previous doc sentence, I think that everything should be set to HTTP/1.0, right?
This could be fixed later through a follow up patch.
Just want to confirm the current case related to my patch.
Totally agree with the "configurable but defaulting to the more secure behavior" path. I'll rework my patch to introduce an option.
Thanks for the heads up.
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #826 +/- ##
=====================================
- Coverage 54% 54% -1%
=====================================
Files 88 88
Lines 9744 9752 +8
Branches 1814 1816 +2
=====================================
- Hits 5296 5295 -1
- Misses 4082 4089 +7
- Partials 366 368 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
1d314a2
to
d0a06f7
Compare
@@ -2070,7 +2086,7 @@ def test_short_read_with_content_length(self): | |||
def test_short_read_with_zero_content_length(self): | |||
body = self.body() | |||
req = "POST /short-read HTTP/1.1\r\ntransfer-encoding: Chunked\r\n" \ | |||
"Content-Length:0\r\n\r\n" + body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed to do not reflect and encourage bad practices, I wonder if this test itself could be removed.
Another option, would be to remove the transfer-encoding
and rename the test and keep it alive.
Any opinion?
@@ -2058,7 +2074,7 @@ def ping(self, fd): | |||
def test_short_read_with_content_length(self): | |||
body = self.body() | |||
req = "POST /short-read HTTP/1.1\r\ntransfer-encoding: Chunked\r\n" \ | |||
"Content-Length:1000\r\n\r\n" + body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed to do not reflect and encourage bad practices, I wonder if this test itself could be removed.
Another option, would be to remove the transfer-encoding
and rename the test and keep it alive.
Any opinion?
@@ -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" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed to do not reflect and encourage bad practices.
@@ -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" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed to do not reflect and encourage bad practices.
@@ -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" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed to do not reflect and encourage bad practices.
codecov is a bit flaky... |
@@ -340,11 +356,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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this level of configuration is enough for you?
c619d15
to
e994005
Compare
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¶m2=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.
@tipabu is the latest version is ok for you? |
If no answer in a few days should probably just proceed, given it's a security issue. |
yes |
Several important and urgent fixes are released there. 0.34.3 ====== eventlet/eventlet#875 * Fix security issue in the wsgi module related to RFC 9112 eventlet/eventlet#826 * Fix segfault, a new approach for greening existing locks eventlet/eventlet#866 * greendns: fix getaddrinfo parameter name eventlet/eventlet#809 * Fix deprecation warning on ssl.PROTOCOL_TLS eventlet/eventlet#872 * Pytests, fix error at teardown of TestGreenSocket.test_full_duplex eventlet/eventlet#871 * Skip test which uses Py cgi module eventlet/eventlet#865 * Drop old code based on python < 3.7.34.2 ====== eventlet/eventlet#861 * Allowing inheritance of GreenSSLSocket without overriding the __new_ method eventlet/eventlet#796 * [bug] Fix broken API related to `__version__` removal eventlet/eventlet#859 * [doc] Fix pypi broken link eventlet/eventlet#857 0.34.1 ====== eventlet/eventlet#842 * [bug] Fix memory leak in greendns eventlet/eventlet#810 * [infra] Fix OIDC authentication failure eventlet/eventlet#855 * [bug] Ignore asyncore and asynchat for Python 3.12+ eventlet/eventlet#804 0.34.0 (Not released on Pypi but landed with 0.34.1) ==================================================== * Dropped support for Python 3.6 and earlier. * Fix Python 3.13 compat by adding missing attibute '_is_main_interpreter' eventlet/eventlet#847 * Add support of Python 3.12 eventlet/eventlet#817 * Drop unmaintained and unused stdlib tests eventlet/eventlet#820 * Fix tests and CI for Python 3.7 and higher eventlet/eventlet#831 and eventlet/eventlet#832 * Stop claiming to create universal wheels eventlet/eventlet#841 * Fix green logging locks for Python versions <= 3.10 eventlet/eventlet#754 Change-Id: Ib2e59a207b86ae90fa391bf1dff7819851dc9c9b
* Update requirements from branch 'master' to 827a86739e66ffb537b5ea7a65a3cc74eb3fabf1 - Merge "Update eventlet to 0.34.3" - Update eventlet to 0.34.3 Several important and urgent fixes are released there. 0.34.3 ====== eventlet/eventlet#875 * Fix security issue in the wsgi module related to RFC 9112 eventlet/eventlet#826 * Fix segfault, a new approach for greening existing locks eventlet/eventlet#866 * greendns: fix getaddrinfo parameter name eventlet/eventlet#809 * Fix deprecation warning on ssl.PROTOCOL_TLS eventlet/eventlet#872 * Pytests, fix error at teardown of TestGreenSocket.test_full_duplex eventlet/eventlet#871 * Skip test which uses Py cgi module eventlet/eventlet#865 * Drop old code based on python < 3.7.34.2 ====== eventlet/eventlet#861 * Allowing inheritance of GreenSSLSocket without overriding the __new_ method eventlet/eventlet#796 * [bug] Fix broken API related to `__version__` removal eventlet/eventlet#859 * [doc] Fix pypi broken link eventlet/eventlet#857 0.34.1 ====== eventlet/eventlet#842 * [bug] Fix memory leak in greendns eventlet/eventlet#810 * [infra] Fix OIDC authentication failure eventlet/eventlet#855 * [bug] Ignore asyncore and asynchat for Python 3.12+ eventlet/eventlet#804 0.34.0 (Not released on Pypi but landed with 0.34.1) ==================================================== * Dropped support for Python 3.6 and earlier. * Fix Python 3.13 compat by adding missing attibute '_is_main_interpreter' eventlet/eventlet#847 * Add support of Python 3.12 eventlet/eventlet#817 * Drop unmaintained and unused stdlib tests eventlet/eventlet#820 * Fix tests and CI for Python 3.7 and higher eventlet/eventlet#831 and eventlet/eventlet#832 * Stop claiming to create universal wheels eventlet/eventlet#841 * Fix green logging locks for Python versions <= 3.10 eventlet/eventlet#754 Change-Id: Ib2e59a207b86ae90fa391bf1dff7819851dc9c9b
0.34.3 * Fix security issue in the wsgi module related to RFC 9112 eventlet#826 * Fix segfault, a new approach for greening existing locks eventlet#866 * greendns: fix getaddrinfo parameter name eventlet#809 * Fix deprecation warning on ssl.PROTOCOL_TLS eventlet#872 * Pytests, fix error at teardown of TestGreenSocket.test_full_duplex eventlet#871 * Skip test which uses Py cgi module eventlet#865 * Drop old code based on python < 3.7
See eventlet/eventlet#826 and its follow-up, eventlet/eventlet#890 Change-Id: I7dff5342013a3f31f19cb410a9f3f6d4b60938f1
Recently, upper-constraints updated eventlet. Unfortunately, there was a bug which breaks our unit tests which was not discovered during the cross-project testing because the affected unit tests require an XFS temp dir. The requirements change has since been reverted, but we ought to have tests that cover the problematic behavior that will actually run as part of cross-project testing. See eventlet/eventlet#826 for the eventlet change that introduced the bug; it has since been fixed on master in eventlet/eventlet#890 (though we still need https://review.opendev.org/c/openstack/swift/+/905796 to be able to work with eventlet master). Change-Id: I4a6d79317b65f746ee29d2d25073b8c3859cd6a0
* Update swift from branch 'master' to 03b033f70f0f5cd200fbfd0067b14d81ad316bb0 - Merge "Work with latest eventlet (again)" - Work with latest eventlet (again) See eventlet/eventlet#826 and its follow-up, eventlet/eventlet#890 Change-Id: I7dff5342013a3f31f19cb410a9f3f6d4b60938f1
* Update swift from branch 'master' to 52321866d960703c1ef4152440117ad6d84068cd - Merge "tests: Exercise recent eventlet breakage without XFS" - tests: Exercise recent eventlet breakage without XFS Recently, upper-constraints updated eventlet. Unfortunately, there was a bug which breaks our unit tests which was not discovered during the cross-project testing because the affected unit tests require an XFS temp dir. The requirements change has since been reverted, but we ought to have tests that cover the problematic behavior that will actually run as part of cross-project testing. See eventlet/eventlet#826 for the eventlet change that introduced the bug; it has since been fixed on master in eventlet/eventlet#890 (though we still need https://review.opendev.org/c/openstack/swift/+/905796 to be able to work with eventlet master). Change-Id: I4a6d79317b65f746ee29d2d25073b8c3859cd6a0
* Update requirements from branch 'master' to 08f829d8375b4059af365191e0907069be9fb739 - Update eventlet to 0.35.0 0.35.0 ====== eventlet/eventlet#897 * [fix] fix truncate size nullable eventlet/eventlet#789 * [fix] Handle transport endpoint shutdown in conditions eventlet/eventlet#884 * [fix] Rework reject_bad_requests option eventlet/eventlet#890 * [fix] Fix NameError introduced by #826 eventlet/eventlet#890 * [feature] Support awaiting GreenThread in an `async def` context eventlet/eventlet#889 * [feature] Asyncio hub support for Python 3.7 to 3.9 eventlet/eventlet#886 * [fix] Fix bad exceptions handlings eventlet/eventlet#883 * [feature] Support using asyncio coroutines from inside greenlets eventlet/eventlet#877 * [removal] Remove deprecated CGIHTTPServer and SimpleHTTPServer eventlet/eventlet#881 * [feature] Add an asyncio hub for eventlet eventlet/eventlet#870 0.34.3 ====== eventlet/eventlet#875 * Fix security issue in the wsgi module related to RFC 9112 eventlet/eventlet#826 * Fix segfault, a new approach for greening existing locks eventlet/eventlet#866 * greendns: fix getaddrinfo parameter name eventlet/eventlet#809 * Fix deprecation warning on ssl.PROTOCOL_TLS eventlet/eventlet#872 * Pytests, fix error at teardown of TestGreenSocket.test_full_duplex eventlet/eventlet#871 * Skip test which uses Py cgi module eventlet/eventlet#865 * Drop old code based on python < 3.7.34.2 ====== eventlet/eventlet#861 * Allowing inheritance of GreenSSLSocket without overriding the __new_ method eventlet/eventlet#796 * [bug] Fix broken API related to `__version__` removal eventlet/eventlet#859 * [doc] Fix pypi broken link eventlet/eventlet#857 0.34.1 ====== eventlet/eventlet#842 * [bug] Fix memory leak in greendns eventlet/eventlet#810 * [infra] Fix OIDC authentication failure eventlet/eventlet#855 * [bug] Ignore asyncore and asynchat for Python 3.12+ eventlet/eventlet#804 0.34.0 (Not released on Pypi but landed with 0.34.1) ==================================================== * Dropped support for Python 3.6 and earlier. * Fix Python 3.13 compat by adding missing attibute '_is_main_interpreter' eventlet/eventlet#847 * Add support of Python 3.12 eventlet/eventlet#817 * Drop unmaintained and unused stdlib tests eventlet/eventlet#820 * Fix tests and CI for Python 3.7 and higher eventlet/eventlet#831 and eventlet/eventlet#832 * Stop claiming to create universal wheels eventlet/eventlet#841 * Fix green logging locks for Python versions <= 3.10 eventlet/eventlet#754 Change-Id: I909be1d1812eaed574525866dbc123083684571d
0.35.0 ====== eventlet/eventlet#897 * [fix] fix truncate size nullable eventlet/eventlet#789 * [fix] Handle transport endpoint shutdown in conditions eventlet/eventlet#884 * [fix] Rework reject_bad_requests option eventlet/eventlet#890 * [fix] Fix NameError introduced by #826 eventlet/eventlet#890 * [feature] Support awaiting GreenThread in an `async def` context eventlet/eventlet#889 * [feature] Asyncio hub support for Python 3.7 to 3.9 eventlet/eventlet#886 * [fix] Fix bad exceptions handlings eventlet/eventlet#883 * [feature] Support using asyncio coroutines from inside greenlets eventlet/eventlet#877 * [removal] Remove deprecated CGIHTTPServer and SimpleHTTPServer eventlet/eventlet#881 * [feature] Add an asyncio hub for eventlet eventlet/eventlet#870 0.34.3 ====== eventlet/eventlet#875 * Fix security issue in the wsgi module related to RFC 9112 eventlet/eventlet#826 * Fix segfault, a new approach for greening existing locks eventlet/eventlet#866 * greendns: fix getaddrinfo parameter name eventlet/eventlet#809 * Fix deprecation warning on ssl.PROTOCOL_TLS eventlet/eventlet#872 * Pytests, fix error at teardown of TestGreenSocket.test_full_duplex eventlet/eventlet#871 * Skip test which uses Py cgi module eventlet/eventlet#865 * Drop old code based on python < 3.7.34.2 ====== eventlet/eventlet#861 * Allowing inheritance of GreenSSLSocket without overriding the __new_ method eventlet/eventlet#796 * [bug] Fix broken API related to `__version__` removal eventlet/eventlet#859 * [doc] Fix pypi broken link eventlet/eventlet#857 0.34.1 ====== eventlet/eventlet#842 * [bug] Fix memory leak in greendns eventlet/eventlet#810 * [infra] Fix OIDC authentication failure eventlet/eventlet#855 * [bug] Ignore asyncore and asynchat for Python 3.12+ eventlet/eventlet#804 0.34.0 (Not released on Pypi but landed with 0.34.1) ==================================================== * Dropped support for Python 3.6 and earlier. * Fix Python 3.13 compat by adding missing attibute '_is_main_interpreter' eventlet/eventlet#847 * Add support of Python 3.12 eventlet/eventlet#817 * Drop unmaintained and unused stdlib tests eventlet/eventlet#820 * Fix tests and CI for Python 3.7 and higher eventlet/eventlet#831 and eventlet/eventlet#832 * Stop claiming to create universal wheels eventlet/eventlet#841 * Fix green logging locks for Python versions <= 3.10 eventlet/eventlet#754 Change-Id: I909be1d1812eaed574525866dbc123083684571d
Reject requests which contains headers
content-length
andtransfer-encoding
at the same time.That's not allowed by RFC 9112 and that could lead to potential security attacks.
https://www.rfc-editor.org/rfc/rfc9112#section-6.1-15
This is an extract of the RFC:
The following request would lead to this scenario:
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:
$ eventlet_wsgi_example
)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 thechunk
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.