Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 40 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisesbpo-39503: Fix urllib basic auth regex #18284
Conversation
The AbstractBasicAuthHandler class of the 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 Matt Schwager.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mschwager
commented
Jan 30, 2020
|
This fix looks good to me! |
| @@ -937,7 +937,7 @@ 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]+' | |||
| rx = re.compile('(?:[^,]*,)*[ \t]*([^ \t]+)[ \t]+' | |||
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Jan 30, 2020
Member
(?:.*,)* is equivalent to (?:.*,)?.
But since this regular expresion is only used with search(). (?:.*,)*[ \t]* can be removed at all.
I'll analyze whether it is correct or there is an error in the regular expression.
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Jan 30, 2020
Member
Well, I am cannot say that I completely understand the code, but to give it some sense we can either
- Replace
rx.search()withrx.match()and replace(?:.*,)*with(?:.*,)?.
or
- Keep
rx.search()and replace(?:.*,)*with(?:^|,).
Do not keep (?:[^,]*,)*. It is a waster of resources.
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Jan 30, 2020
Member
Humm, options 1 and 2 are not equivalent if the field value contains more than one challenge. Option 2 is closer to the current behavior. But correct support of more than one challenge need rewriting the code.
This comment has been minimized.
This comment has been minimized.
|
Would be nice to add a test. |
This comment has been minimized.
This comment has been minimized.
mschwager
commented
Jan 30, 2020
|
Just a heads up, CVE-2020-8492 has been created. I'm not sure how Python CVEs are generally tracked, but it may be useful to include the information on the bug tracker issue |
vstinner commentedJan 30, 2020
•
edited by bedevere-bot
The AbstractBasicAuthHandler class of the 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 Matt Schwager.
https://bugs.python.org/issue39503