Skip to content
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-35907, CVE-2019-9948: urllib rejects local_file:// scheme #13474

Merged
merged 1 commit into from May 22, 2019

Conversation

@vstinner
Copy link
Member

vstinner commented May 21, 2019

CVE-2019-9948: Avoid file reading as disallowing the unnecessary URL
scheme in URLopener().open() of urllib.request.

Co-Authored-By: SH push0ebp@gmail.com

https://bugs.python.org/issue35907

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented May 21, 2019

This PR is very close to PR #11842, I just added the unit test:

             self.assertRaises(IOError, urllib.request.URLopener().open, url) 
@vstinner vstinner requested a review from tiran May 21, 2019
@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented May 21, 2019

@tiran: Would you mind to review this forward port of PR #11842? Fix for https://bugs.python.org/issue35907

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented May 22, 2019

@tirkarthi @gpshead: Would you mind to review this HTTP security fix?

for url in ('local_file://example', 'local-file://example'):
self.assertRaises(IOError, urllib.request.urlopen, url)
self.assertRaises(IOError, urllib.request.URLopener().open, url)
self.assertRaises(IOError, DummyURLopener().open, url)

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi May 22, 2019

Contributor

Are we making this change to the deprecated legacy interface urllib.request.URLopener().retrieve too? If so I would prefer adding below test since it also now throws IOError that used to return a result. I guess Python 2 also has urlretrieve which now forbids local_file

Note : urlretrieve was untested but documented that it used to throw NameError (https://bugs.python.org/issue36948) . Please also check Windows compatibility if urlretrieve tests have to be added since I mistakenly broke two Windows buildbots in the issue.

self.assertRaises(urllib.request.URLopener().rertrieve, url)

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi May 22, 2019

Contributor

Sorry, my test was wrong. urlretrieve requires a file. Maybe above urlretrieve tests can be used to just check for local-file:// too? Change in behavior as below

Before PR :

cpython git:(master) ✗ touch /tmp/a.txt
cpython git:(master) ✗ ./python.exe -q
>>> import urllib.request
>>> urllib.request.URLopener().retrieve('local_file://localhost/tmp/a.txt')
sys:1: DeprecationWarning: URLopener style of invoking requests is deprecated. Use newer urlopen functions/methods
('/var/folders/2b/mhgtnnpx4z943t4cc9yvw4qw0000gn/T/tmpmx6km86p.txt', <email.message.Message object at 0x1066a2a00>)

After PR :

cpython git:(master) ✗ touch /tmp/a.txt
cpython git:(master) ✗ ./python.exe -q
>>> import urllib.request
>>> urllib.request.URLopener().retrieve('local_file://localhost/tmp/a.txt')
sys:1: DeprecationWarning: URLopener style of invoking requests is deprecated. Use newer urlopen functions/methods
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/urllib/request.py", line 1789, in retrieve
    fp = self.open(url, data)
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/urllib/request.py", line 1752, in open
    return self.open_unknown(fullurl, data)
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/urllib/request.py", line 1766, in open_unknown
    raise OSError('url error', 'unknown url type', type)
OSError: [Errno url error] unknown url type: 'local_file'

This comment has been minimized.

Copy link
@vstinner

vstinner May 22, 2019

Author Member

I didn't know URLopener().retrieve :-) My PR impacts it as well.

I added requested tests.

CVE-2019-9948: Avoid file reading as disallowing the unnecessary URL
scheme in URLopener().open() and URLopener().retrieve()
of urllib.request.

Co-Authored-By: SH <push0ebp@gmail.com>
@vstinner vstinner force-pushed the vstinner:local_file branch from af50efe to 7b04bc2 May 22, 2019
@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented May 22, 2019

I rebased my PR to modify the commit message: mention also URLopener().retrieve().

Copy link
Contributor

tirkarthi left a comment

LGTM. Thanks :)

@vstinner vstinner merged commit 0c2b6a3 into python:master May 22, 2019
5 checks passed
5 checks passed
Azure Pipelines PR #20190522.101 succeeded
Details
bedevere/issue-number Issue number 35907 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented May 22, 2019

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖 I'm not a witch! I'm not a witch!

@vstinner vstinner deleted the vstinner:local_file branch May 22, 2019
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented May 22, 2019

Sorry, @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0c2b6a3943aa7b022e8eb4bfd9bffcddebf9a587 3.7

vstinner added a commit to vstinner/cpython that referenced this pull request May 22, 2019
…GH-13474)

CVE-2019-9948: Avoid file reading as disallowing the unnecessary URL
scheme in URLopener().open() and URLopener().retrieve()
of urllib.request.

Co-Authored-By: SH <push0ebp@gmail.com>
(cherry picked from commit 0c2b6a3)
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented May 22, 2019

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

vstinner added a commit to vstinner/cpython that referenced this pull request May 22, 2019
…GH-13474)

CVE-2019-9948: Avoid file reading as disallowing the unnecessary URL
scheme in URLopener().open() and URLopener().retrieve()
of urllib.request.

Co-Authored-By: SH <push0ebp@gmail.com>
(cherry picked from commit 0c2b6a3)
vstinner added a commit that referenced this pull request May 22, 2019
…) (GH-13505)

CVE-2019-9948: Avoid file reading as disallowing the unnecessary URL
scheme in URLopener().open() and URLopener().retrieve()
of urllib.request.

Co-Authored-By: SH <push0ebp@gmail.com>
(cherry picked from commit 0c2b6a3)
vstinner added a commit to vstinner/cpython that referenced this pull request May 22, 2019
…GH-13474) (pythonGH-13505)

CVE-2019-9948: Avoid file reading as disallowing the unnecessary URL
scheme in URLopener().open() and URLopener().retrieve()
of urllib.request.

Co-Authored-By: SH <push0ebp@gmail.com>
(cherry picked from commit 0c2b6a3)
(cherry picked from commit 34bab21)
vstinner added a commit to vstinner/cpython that referenced this pull request May 22, 2019
…GH-13474) (pythonGH-13505)

CVE-2019-9948: Avoid file reading as disallowing the unnecessary URL
scheme in URLopener().open() and URLopener().retrieve()
of urllib.request.

Co-Authored-By: SH <push0ebp@gmail.com>
(cherry picked from commit 0c2b6a3)
(cherry picked from commit 34bab21)
@@ -0,0 +1,2 @@
CVE-2019-9948: Avoid file reading as disallowing the unnecessary URL scheme in

This comment has been minimized.

Copy link
@gpshead

gpshead May 22, 2019

Member

This text doesn't make sense as written. "as disallowing the unnecessary URL scheme" says nothing. WHAT url scheme did we just break our users applications by refusing to support? Those need to be listed in the NEWS entry.

CVE-2019-9948: Drop support for the unintended local_file:// URL scheme in ...
vstinner added a commit to vstinner/cpython that referenced this pull request May 27, 2019
…GH-13474) (pythonGH-13505)

CVE-2019-9948: Avoid file reading by disallowing local-file:// and
local_file:// URL schemes in URLopener().open() and
URLopener().retrieve() of urllib.request.

Co-Authored-By: SH <push0ebp@gmail.com>
(cherry picked from commit 0c2b6a3)
(cherry picked from commit 34bab21)
vstinner added a commit to vstinner/cpython that referenced this pull request May 27, 2019
…GH-13474) (pythonGH-13505)

CVE-2019-9948: Avoid file reading by disallowing local-file:// and
local_file:// URL schemes in URLopener().open() and
URLopener().retrieve() of urllib.request.

Co-Authored-By: SH <push0ebp@gmail.com>
(cherry picked from commit 0c2b6a3)
(cherry picked from commit 34bab21)
larryhastings added a commit that referenced this pull request Jul 14, 2019
…) (GH-13505) (#13510)

CVE-2019-9948: Avoid file reading by disallowing local-file:// and
local_file:// URL schemes in URLopener().open() and
URLopener().retrieve() of urllib.request.

Co-Authored-By: SH <push0ebp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.