[3.8] bpo-43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. (GH-25595) by miss-islington · Pull Request #25726 · python/cpython · GitHub

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

Co-authored-by: Gregory P. Smith greg@krypto.org
Co-authored-by: Serhiy Storchaka storchaka@gmail.com
(cherry picked from commit 76cd81d)

Co-authored-by: Senthil Kumaran senthil@uthcode.com

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 <greg@krypto.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit 76cd81d) Co-authored-by: Senthil Kumaran <senthil@uthcode.com>

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

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.

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!

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.

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

I have made the requested changes; please review again.

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..

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

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

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

Thanks! ✨ 🍰 ✨

…newline and tabs. (pythonGH-25595) (pythonGH-25726) Co-authored-by: Gregory P. Smith <greg@krypto.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit 76cd81d) Co-authored-by: Senthil Kumaran <senthil@uthcode.com> Co-authored-by: Senthil Kumaran <skumaran@gatech.edu> (cherry picked from commit 515a7bc) Co-authored-by: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com>

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 <greg@krypto.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit 76cd81d) Co-authored-by: Senthil Kumaran <senthil@uthcode.com> Co-authored-by: Senthil Kumaran <skumaran@gatech.edu> (cherry picked from commit 515a7bc) Co-authored-by: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com>

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

Comment thread Lib/urllib/parse.py

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

Comment thread Lib/urllib/parse.py

orsenthil reviewed May 3, 2021

View reviewed changes

Comment thread Lib/test/test_urlparse.py

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

                           if '?' in url:
                               url, query = url.split('?', 1)
                           _checknetloc(netloc)
+                          url = _remove_unsafe_bytes_from_url(url)

+                      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"

[3.8] bpo-43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. (GH-25595) by miss-islington · Pull Request #25726 · python/cpython · GitHub

Repository navigation

[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)#25726ambv merged 5 commits intopython:3.8python/cpython:3.8from miss-islington:backport-76cd81d-3.8miss-islington/cpython:backport-76cd81d-3.8Copy head branch name to clipboard

Conversation

miss-islington commented Apr 29, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miss-islington commented Apr 29, 2021

Uh oh!

Uh oh!

gpshead May 1, 2021

Choose a reason for hiding this comment

Uh oh!

gpshead May 1, 2021

Choose a reason for hiding this comment

Uh oh!

orsenthil May 1, 2021

Choose a reason for hiding this comment

Uh oh!

gpshead May 2, 2021

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 1, 2021

Uh oh!

ambv commented May 3, 2021

Uh oh!

Uh oh!

orsenthil May 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orsenthil commented May 3, 2021

Uh oh!

miss-islington commented May 3, 2021

Uh oh!

orsenthil commented May 5, 2021

Uh oh!

bedevere-bot commented May 5, 2021

Uh oh!

miss-islington commented May 5, 2021

Uh oh!

ambv commented May 5, 2021

Uh oh!

bedevere-bot commented May 5, 2021

Uh oh!

bedevere-bot commented May 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[3.8] bpo-43882 - urllib.parse should sanitize urls containing ASCII newline and tabs. (GH-25595)#25726
ambv merged 5 commits into
python:3.8from
miss-islington:backport-76cd81d-3.8

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