-
Notifications
You must be signed in to change notification settings - Fork 2
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
00366-CVE-2021-3733.patch #22
Conversation
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.
LGTM.
In Python 3, it was decided to not only change the regex to avoid the worst case performance, but also to "fix" the behavior since the old one was not reliable nor correct. See python#18284 discussion about RFC, web browser behavior, etc. In Python 2.7, we can make a different trade-off: we can only make the regex more efficient without changing the behavior (pick the last realm when the web server offers multiple Basic realms). |
00366 # CVE-2021-3733: Fix ReDoS in urllib AbstractBasicAuthHandler Fix Regular Expression Denial of Service (ReDoS) vulnerability in urllib2.AbstractBasicAuthHandler. The ReDoS-vulnerable regex has quadratic worst-case complexity and it allows cause a denial of service when identifying crafted invalid RFCs. This ReDoS issue is on the client side and needs remote attackers to control the HTTP server. Backported from Python 3 together with another backward-compatible improvement of the regex from fix for CVE-2020-8492. Co-authored-by: Yeting Li <liyt@ios.ac.cn>
67ed90a
to
78a8416
Compare
Thanks Victor. I've incorporated the efficiency improvement proposed into the patch and I'll test it now. I will let you know about the results. |
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.
LGTM. If possible, the change should be validated with a benchmark.
Simple reproducer: from urllib2 import AbstractBasicAuthHandler
AbstractBasicAuthHandler.rx.search(
"basic " + ("," * 25) + "A"
) Official one using pyperf: from urllib2 import AbstractBasicAuthHandler
import pyperf
class AuthHandler(AbstractBasicAuthHandler):
handler = None
def retry_http_basic_auth(self, host, req, realm):
self.realm = realm
return None
realm = 'realm@example.com'
simple = 'Basic realm="{}"'.format(realm)
repeat_10 = '' + ',' * 10 + 'A'
repeat_10_2 = '' + ',' * (10 ** 2)
repeat_10_4 = '' + ',' * (10 ** 4)
class Headers:
def __init__(self, header):
self.header = header
def get(self, *ignored_args):
return self.header
def get_all(self, *ignored_args):
return [self.header]
def func(handler, headers):
try:
handler.http_error_auth_reqed("WWW-Authenticate", None, None, headers)
except ValueError:
pass
runner = pyperf.Runner()
handler = AuthHandler()
for name, header in (
('simple', simple),
('repeat 10', repeat_10),
('repeat 10^2', repeat_10_2),
('repeat 10^4', repeat_10_4),
):
headers = Headers(header)
runner.bench_func(name, func, handler, headers) Old version:
There is not a single dot for 10² in the benchmark above in more than 25 minutes. New version:
Benchmark is not as fast as it is for the new Pythons but it's able to finish testing in less than 9 minutes and provides reasonable results also for 10⁴. I think this can be approved. |
LGTM. https://bugs.python.org/issue39503 and https://bugs.python.org/issue43075 are about exponential complexity, something like: O(n^2). Here, it looks more like O(n) complexity which is the expected result. |
00368 # CVE-2021-3737: http client infinite line reading (DoS) after a HTTP 100 Continue Fixes http.client potential denial of service where it could get stuck reading lines from a malicious server after a 100 Continue response. Backported from Python 3. Co-authored-by: Gregory P. Smith <greg@krypto.org> Co-authored-by: Gen Xu <xgbarry@gmail.com>
I've added one more patch here for CVE-2021-3737. The fix also contains a test and it passes. It is not possible to build Python 2.7 in rawhide now but I've managed to do that in mock with downgraded openssl to version 1.1 and here is verification of the fix using the official reproducer: Old version:
New version:
@vstinner could you please take a look once more? No rush, I guess we have to wait for openssl 1.1 packages in rawhide. |
Hum, you may mention that it's a backport of: python@47895e3 I checked the backport: yes, it LGTM ;-) |
For the record why I merged this: I've accidentally merged the Fedora PR, so I went ahead and merged this one as well. |
I'm gonna test this on RPM level.
00366 #
CVE-2021-3733: Fix ReDoS in urllib AbstractBasicAuthHandler
Fix Regular Expression Denial of Service (ReDoS) vulnerability in
urllib2.AbstractBasicAuthHandler. The ReDoS-vulnerable regex
has quadratic worst-case complexity and it allows cause a denial of
service when identifying crafted invalid RFCs. This ReDoS issue is on
the client side and needs remote attackers to control the HTTP server.
Backported from Python 3.