[3.8] bpo-43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. (GH-25595) #25726

miss-islington · 2021-04-29T17:17:17Z

issue43882 - urllib.parse should sanitize urls containing ASCII newline and tabs.

Co-authored-by: Gregory P. Smith [email protected]
Co-authored-by: Serhiy Storchaka [email protected]
(cherry picked from commit 76cd81d)

Co-authored-by: Senthil Kumaran [email protected]

https://bugs.python.org/issue43882

…e and tabs. (pythonGH-25595) * issue43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. Co-authored-by: Gregory P. Smith <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]> (cherry picked from commit 76cd81d) Co-authored-by: Senthil Kumaran <[email protected]>

miss-islington · 2021-04-29T17:51:33Z

@orsenthil: Status check is done, and it's a failure ❌ .

Lib/urllib/parse.py

gpshead · 2021-05-01T17:19:44Z

Lib/urllib/parse.py

@@ -443,6 +451,7 @@ def urlsplit(url, scheme='', allow_fragments=True):
            if '?' in url:
                url, query = url.split('?', 1)
            _checknetloc(netloc)
+            url = _remove_unsafe_bytes_from_url(url)


I think we need to do this up front right after _coerce_args(url, ...) (and should do the same in 3.9 and 3.10).

by this point in 3.8 we've potentially allowed characters to slip through into query and fragment of http: urls.

at a minumum if we weren't do this right after _coerce_args before looking in the cache, this needs to be done right before any splitting happens in this branch of code.

the test case should be updated to include an invalid character in each of the five portions of the url.

allowed characters to slip through into query and fragment

I had a doubt initially, and I thought (from practice) that newlines and tabs are percent-encoded when in query/fragment than they are removed upfront.

We will have find a spec that will unambiguously state what should be done.

We've already got that unambiguous spec. https://url.spec.whatwg.org/#concept-basic-url-parser step 3. strip these three characters no matter what before the parsing state machine starts.

bedevere-bot · 2021-05-01T17:20:53Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

ambv · 2021-05-03T09:07:12Z

Note: this will miss 3.8.10 but as a security fix will be included in 3.8.11 later in the year.

…che key.

Lib/urllib/parse.py

orsenthil · 2021-05-03T13:12:52Z

Lib/test/test_urlparse.py

+        self.assertEqual(p.geturl(), "x-new-scheme://www.python.org/javascript:alert('msg')/?query=something#fragment")
+
+        # Remove ASCII tabs and newlines from input as bytes, any scheme.
+        url = b"x-new-scheme\t://www.python.org/java\nscript:\talert('msg\r\n')/?query\n=\tsomething#frag\nment"


Updated test cases to verify removal in all parts of the URL.

Lib/test/test_urlparse.py

orsenthil · 2021-05-03T13:14:41Z

I have made the requested changes; please review again.

miss-islington · 2021-05-03T20:54:59Z

Sorry, I can't merge this PR. Reason: You're not authorized to push to this branch. Visit https://docs.github.com/articles/about-protected-branches/ for more information..

orsenthil · 2021-05-05T16:45:52Z

This can be merged now, @ambv . Thank you.

bedevere-bot · 2021-05-05T17:25:32Z

@ambv: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2021-05-05T17:25:32Z

Thanks @miss-islington for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

ambv · 2021-05-05T17:25:40Z

Thanks! ✨ 🍰 ✨

…newline and tabs. (pythonGH-25595) (pythonGH-25726) Co-authored-by: Gregory P. Smith <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]> (cherry picked from commit 76cd81d) Co-authored-by: Senthil Kumaran <[email protected]> Co-authored-by: Senthil Kumaran <[email protected]> (cherry picked from commit 515a7bc) Co-authored-by: Miss Islington (bot) <[email protected]>

bedevere-bot · 2021-05-05T17:25:49Z

GH-25923 is a backport of this pull request to the 3.7 branch.

…newline and tabs. (pythonGH-25595) (pythonGH-25726) Co-authored-by: Gregory P. Smith <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]> (cherry picked from commit 76cd81d) Co-authored-by: Senthil Kumaran <[email protected]> Co-authored-by: Senthil Kumaran <[email protected]> (cherry picked from commit 515a7bc) Co-authored-by: Miss Islington (bot) <[email protected]>

bedevere-bot · 2021-05-05T17:25:55Z

GH-25924 is a backport of this pull request to the 3.6 branch.

bedevere-bot added the awaiting review label Apr 29, 2021

the-knights-who-say-ni added the CLA signed label Apr 29, 2021

bedevere-bot mentioned this pull request Apr 29, 2021

bpo-43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. #25595

Merged

bedevere-bot added type-bug An unexpected behavior, bug, or error type-security A security issue labels Apr 29, 2021

orsenthil approved these changes Apr 29, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Apr 29, 2021

orsenthil self-requested a review April 29, 2021 17:18

Update version to 3.8.10 for 3.8 branch

957ee9d

orsenthil approved these changes Apr 29, 2021

View reviewed changes

Fix 3.8 tests.

9a1d1f7

orsenthil reviewed Apr 30, 2021

View reviewed changes

Lib/urllib/parse.py Show resolved Hide resolved

gpshead requested changes May 1, 2021

View reviewed changes

bedevere-bot removed the awaiting merge label May 1, 2021

bedevere-bot added the awaiting changes label May 1, 2021

Address Review Comment. Strip Unsafe Chars very early, and include ca…

ce95fae

…che key.

orsenthil requested a review from gpshead May 3, 2021 13:08

orsenthil reviewed May 3, 2021

View reviewed changes

Lib/urllib/parse.py Show resolved Hide resolved

orsenthil reviewed May 3, 2021

View reviewed changes

Lib/test/test_urlparse.py Show resolved Hide resolved

orsenthil mentioned this pull request May 3, 2021

[3.9] bpo-43882 Remove the newline, and tab early. From query and fragments. #25853

Merged

Address review comments.

94da00c

gpshead added needs backport to 3.7 labels May 3, 2021

gpshead approved these changes May 3, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels May 3, 2021

nateprewitt mentioned this pull request May 4, 2021

FAIL: test_no_new_lines (tests.unit.test_utils.TestIsValidEndpointURL) ; with Python versions patched for bpo-43882 boto/botocore#2377

Closed

orsenthil assigned ambv May 5, 2021

ambv merged commit 515a7bc into python:3.8 May 5, 2021

bedevere-bot removed the awaiting merge label May 5, 2021

miss-islington deleted the backport-76cd81d-3.8 branch May 5, 2021 17:25

bedevere-bot removed the needs backport to 3.7 label May 5, 2021

bedevere-bot removed the needs backport to 3.6 label May 5, 2021

orsenthil mentioned this pull request Aug 25, 2022

[security] CVE-2022-0391: urllib.parse should sanitize urls containing ASCII newline and tabs. #88048

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.8] bpo-43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. (GH-25595) #25726

[3.8] bpo-43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. (GH-25595) #25726

miss-islington commented Apr 29, 2021 •

edited by bedevere-bot

Loading

miss-islington commented Apr 29, 2021

gpshead May 1, 2021

gpshead May 1, 2021

orsenthil May 1, 2021

gpshead May 2, 2021

bedevere-bot commented May 1, 2021

ambv commented May 3, 2021

orsenthil May 3, 2021

orsenthil commented May 3, 2021

miss-islington commented May 3, 2021

orsenthil commented May 5, 2021

bedevere-bot commented May 5, 2021

miss-islington commented May 5, 2021

ambv commented May 5, 2021

bedevere-bot commented May 5, 2021

bedevere-bot commented May 5, 2021

[3.8] bpo-43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. (GH-25595) #25726

[3.8] bpo-43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. (GH-25595) #25726

Conversation

miss-islington commented Apr 29, 2021 • edited by bedevere-bot Loading

miss-islington commented Apr 29, 2021

gpshead May 1, 2021

Choose a reason for hiding this comment

gpshead May 1, 2021

Choose a reason for hiding this comment

orsenthil May 1, 2021

Choose a reason for hiding this comment

gpshead May 2, 2021

Choose a reason for hiding this comment

bedevere-bot commented May 1, 2021

ambv commented May 3, 2021

orsenthil May 3, 2021

Choose a reason for hiding this comment

orsenthil commented May 3, 2021

miss-islington commented May 3, 2021

orsenthil commented May 5, 2021

bedevere-bot commented May 5, 2021

miss-islington commented May 5, 2021

ambv commented May 5, 2021

bedevere-bot commented May 5, 2021

bedevere-bot commented May 5, 2021

miss-islington commented Apr 29, 2021 •

edited by bedevere-bot

Loading