-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
bpo-39503: CVE-2020-8492: Fix AbstractBasicAuthHandler #18284
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -937,8 +937,15 @@ class AbstractBasicAuthHandler: | |||||||||||
|
||||||||||||
# allow for double- and single-quoted realm values | ||||||||||||
# (single quotes are a violation of the RFC, but appear in the wild) | ||||||||||||
rx = re.compile('(?:.*,)*[ \t]*([^ \t]+)[ \t]+' | ||||||||||||
'realm=(["\']?)([^"\']*)\\2', re.I) | ||||||||||||
rx = re.compile('(?:^|,)' # start of the string or ',' | ||||||||||||
'[ \t]*' # optional whitespaces | ||||||||||||
'([^ \t]+)' # scheme like "Basic" | ||||||||||||
'[ \t]+' # mandatory whitespaces | ||||||||||||
# realm=xxx | ||||||||||||
# realm='xxx' | ||||||||||||
# realm="xxx" | ||||||||||||
'realm=(["\']?)([^"\']*)\\2', | ||||||||||||
re.I) | ||||||||||||
|
||||||||||||
# XXX could pre-emptively send auth info already accepted (RFC 2617, | ||||||||||||
# end of section 2, and section 1.2 immediately after "credentials" | ||||||||||||
|
@@ -950,27 +957,51 @@ def __init__(self, password_mgr=None): | |||||||||||
self.passwd = password_mgr | ||||||||||||
self.add_password = self.passwd.add_password | ||||||||||||
|
||||||||||||
def _parse_realm(self, header): | ||||||||||||
# parse WWW-Authenticate header: accept multiple challenges per header | ||||||||||||
found_challenge = False | ||||||||||||
for mo in AbstractBasicAuthHandler.rx.finditer(header): | ||||||||||||
scheme, quote, realm = mo.groups() | ||||||||||||
if quote not in ['"', "'"]: | ||||||||||||
warnings.warn("Basic Auth Realm was unquoted", | ||||||||||||
UserWarning, 3) | ||||||||||||
|
||||||||||||
yield (scheme, realm) | ||||||||||||
|
||||||||||||
found_challenge = True | ||||||||||||
|
||||||||||||
if not found_challenge: | ||||||||||||
if header: | ||||||||||||
scheme = header.split()[0] | ||||||||||||
else: | ||||||||||||
scheme = '' | ||||||||||||
Comment on lines
+974
to
+977
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe simplify it?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the header is an empty string, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right! There is a subtle difference between We can write scheme = (header.split() or [''])[0] but I do not think it will look clearer. 😉 |
||||||||||||
yield (scheme, None) | ||||||||||||
|
||||||||||||
def http_error_auth_reqed(self, authreq, host, req, headers): | ||||||||||||
# host may be an authority (without userinfo) or a URL with an | ||||||||||||
# authority | ||||||||||||
# XXX could be multiple headers | ||||||||||||
authreq = headers.get(authreq, None) | ||||||||||||
headers = headers.get_all(authreq) | ||||||||||||
if not headers: | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is not necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. get_all() returns None if no header is found. We cannot iterate on None. I didn't want to try to pass a second argument to get_all(), since I didn't want to check if it's accepted in all Python versions (since this security fix should be applied to many Python versions). I prefer an explicit test on the result which should be safe. |
||||||||||||
# no header found | ||||||||||||
return | ||||||||||||
|
||||||||||||
if authreq: | ||||||||||||
scheme = authreq.split()[0] | ||||||||||||
if scheme.lower() != 'basic': | ||||||||||||
raise ValueError("AbstractBasicAuthHandler does not" | ||||||||||||
" support the following scheme: '%s'" % | ||||||||||||
scheme) | ||||||||||||
else: | ||||||||||||
mo = AbstractBasicAuthHandler.rx.search(authreq) | ||||||||||||
if mo: | ||||||||||||
scheme, quote, realm = mo.groups() | ||||||||||||
if quote not in ['"',"'"]: | ||||||||||||
warnings.warn("Basic Auth Realm was unquoted", | ||||||||||||
UserWarning, 2) | ||||||||||||
if scheme.lower() == 'basic': | ||||||||||||
return self.retry_http_basic_auth(host, req, realm) | ||||||||||||
unsupported = None | ||||||||||||
for header in headers: | ||||||||||||
for scheme, realm in self._parse_realm(header): | ||||||||||||
if scheme.lower() != 'basic': | ||||||||||||
unsupported = scheme | ||||||||||||
continue | ||||||||||||
|
||||||||||||
if realm is not None: | ||||||||||||
# Use the first matching Basic challenge. | ||||||||||||
# Ignore following challenges even if they use the Basic | ||||||||||||
# scheme. | ||||||||||||
return self.retry_http_basic_auth(host, req, realm) | ||||||||||||
|
||||||||||||
if unsupported is not None: | ||||||||||||
raise ValueError("AbstractBasicAuthHandler does not " | ||||||||||||
"support the following scheme: %r" | ||||||||||||
% (scheme,)) | ||||||||||||
|
||||||||||||
def retry_http_basic_auth(self, host, req, realm): | ||||||||||||
user, pw = self.passwd.find_user_password(realm, host) | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
:class:`~urllib.request.AbstractBasicAuthHandler` of :mod:`urllib.request` | ||
now parses all WWW-Authenticate HTTP headers and accepts multiple challenges | ||
per header: use the realm of the first Basic challenge. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
CVE-2020-8492: The :class:`~urllib.request.AbstractBasicAuthHandler` class of the | ||
:mod:`urllib.request` module uses an inefficient regular expression which can | ||
be exploited by an attacker to cause a denial of service. Fix the regex to | ||
prevent the catastrophic backtracking. Vulnerability reported by Ben Caller | ||
and Matt Schwager. |
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.
What was the motivation of adding a
digest
test case here?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 tried to write a more realistic test than "Otherscheme xxx". The Digest challenge uses "realm" and the test ensures that it's skipped. It uses use multiple fields separated by commas, some use quotes: test that the parser handles that properly.
I picked the example from Wikipedia :-) https://fr.wikipedia.org/wiki/Authentification_HTTP#Demande_d'identification
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.
perfect. sounds good to me. :)