[2.7] bpo-35907: Avoid file reading as disallowing the unnecessary URL scheme in urllib (GH-11842) by push0ebp · Pull Request #11842 · python/cpython · GitHub

https://bugs.python.org/issue35907

…me in urllib

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Is this patch the accepted resolution for CVE-2019-9948? If so, when is it expected to be merged?

Please simplify the fix. There is no need to check for _urlopener here. Just block all access to local_file:// schema. We don't need special cases for subclasses or special instances.

If it does not check the instance type of self, all of overridden open_local_file method will be blocked.
will be okay?
like this

 self.assertRaises(IOError, DummyURLopener().open
            'local_file://example')
 self.assertRaises(IOError, DummyURLopener().open
            'local-file://example')

I think that it needs to check open_local_file is overridden.
What do you think?

I think it has to expect this test result.

def test_local_file_open(self):
        class DummyURLopener(urllib.URLopener):
            def open_local_file(self, url):
                return url

        class DummyURLopener2(urllib.URLopener):
            def open_test(self, url):
                return url

        opener = DummyURLopener()
        opener2 = DummyURLopener2()

        for url in ('local-file://example', 'local_file://example'):    
            self.assertEqual(opener.open(url), '//example')
            self.assertRaises(IOError, urllib.urlopen, url)
            self.assertRaises(IOError, opener2.open, url)
        self.assertEqual(opener2.open('test://example'), '//example')

but Considering the overriding method and instance type check was complex.
If you have a better idea. let me know.

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.

Add prefix "CVE-2019-9948: "

…sies in DummyURLopener test, and simplify mitigation

I have made the requested changes; please review again.

If it does not check the instance type of self, all of overridden open_local_file method will be blocked.
will be okay?
like this

 self.assertRaises(IOError, DummyURLopener().open
            'local_file://example')
 self.assertRaises(IOError, DummyURLopener().open
            'local-file://example')

I think that it needs to check open_local_file is overridden.
What do you think?

I think it has to expect this test result.

def test_local_file_open(self):
        class DummyURLopener(urllib.URLopener):
            def open_local_file(self, url):
                return url

        class DummyURLopener2(urllib.URLopener):
            def open_test(self, url):
                return url

        opener = DummyURLopener()
        opener2 = DummyURLopener2()

        for url in ('local-file://example', 'local_file://example'):    
            self.assertEqual(opener.open(url), '//example')
            self.assertRaises(IOError, urllib.urlopen, url)
            self.assertRaises(IOError, opener2.open, url)
        self.assertEqual(opener2.open('test://example'), '//example')

but Considering the overriding method and instance type check was complex.
If you have a better idea. let me know.

Another fix is to rename the open_local_file() as _open_local_file(). But... In Python 2.7, there is a risk that the method is called directly in 3rd party code for whatever reason :-(

Why do you compare function objects instead of simply name == "open_local_file"?

@tiran
If someone rename open_local_file() method and also forgets to modify name == "open_local_file", it may be bypassed.
So I made the mitigation compare the object to raise syntax error.
Should I do change back?

I concur with Christian, please test directly the name, remove getattr().

@vstinner
I agree with you. I also considered renaming the open_local_file(). I think that this fix can guarantees availability to the developers who want to override this method. But I chose another fix to minimize side effects for third party codes.

Please remove getattr().

Yes. I will do it. I want suggest you other fix.

        if not hasattr(self, name):
            if proxy:
                return self.open_unknown_proxy(proxy, fullurl, data)
            else:
                return self.open_unknown(fullurl, data)
        if name == 'open_local_file': 
                raise IOError, ('url error', 'invalid url type', urltype)
        try:

I think that it is better to raise exception with explicit error message for developers to overriding open_local_file().
they can easily recognize local_file is invalid type.
What do you think about it?
or should I use open_unknown()?

if not hasattr(self, name) or \
    name == 'open_local_file':

# bpo-35907: disallow the file reading with the type not allowed 
if not hasattr(self, name) or name == 'open_local_file': ...

LGTM.

        if name == 'open_local_file': 
                raise IOError, ('url error', 'invalid url type', urltype)

That's wrong exception type, don't do that.

I have changed it. Please review.

I have made the requested changes; please review again.

LGTM.

Modify the object to string in check method name.

LGTM. Just curious about the # which looks like a typo.

I don't understand the # in "# bpo-35907: # disallow ...". Is it a typo?

Oh Sorry, my mistake. I will correct it.

I have fixed the typo.

Fix typo

Thanks @push0ebp for the fix!

Thank you for your accept.
related to #13474

bpo-35907: Avoid file reading as disallowing the unnecessary URL sche…

8f99cc7

…me in urllib

bedevere-bot added the awaiting review label Feb 13, 2019

the-knights-who-say-ni added the CLA not signed label Feb 13, 2019

📜🤖 Added by blurb_it.

b863925

push0ebp changed the title bpo-35907: Avoid file reading as disallowing the unnecessary URL scheme in urllib [2.7] bpo-35907: Avoid file reading as disallowing the unnecessary URL scheme in urllib (GH-11842) Feb 13, 2019

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 13, 2019

hroncok approved these changes May 7, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 7, 2019

vstinner reviewed May 13, 2019

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2019-02-13-17-21-10.bpo-35907.ckk2zg.rst Outdated

vstinner reviewed May 13, 2019

View reviewed changes

Comment thread Lib/test/test_urllib.py Outdated

tiran suggested changes May 13, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels May 13, 2019

push0ebp added 2 commits May 17, 2019 02:31

Update 2019-02-13-17-21-10.bpo-35907.ckk2zg.rst

f20a31c

Add prefix "CVE-2019-9948: "

Update test_urllib.py and urllib.py\nchange assertEqual into assertRa…

179a5f7

…sies in DummyURLopener test, and simplify mitigation

push0ebp commented May 19, 2019

View reviewed changes

csabella requested a review from tiran May 19, 2019 11:13

vstinner reviewed May 20, 2019

View reviewed changes

vstinner approved these changes May 20, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels May 20, 2019

Update urllib.py

3cda03c

Modify the object to string in check method name.

vstinner approved these changes May 21, 2019

View reviewed changes

Update urllib.py

8b7d7ab

Fix typo

vstinner merged commit b15bde8 into python:2.7 May 21, 2019

bedevere-bot removed the awaiting merge label May 21, 2019

vstinner mentioned this pull request May 21, 2019

bpo-35907, CVE-2019-9948: urllib rejects local_file:// scheme #13474

Merged

This was referenced Feb 26, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#646

Closed

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#944

Closed

github-actions Bot mentioned this pull request Mar 10, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#1254

Closed

github-actions Bot mentioned this pull request Mar 24, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#1691

Closed

This was referenced Apr 7, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#2124

Closed

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#2385

Closed

github-actions Bot mentioned this pull request Apr 14, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#2642

Closed

github-actions Bot mentioned this pull request Apr 21, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#2899

Closed

github-actions Bot mentioned this pull request Apr 28, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#3153

Closed

github-actions Bot mentioned this pull request May 5, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#3408

Closed

github-actions Bot mentioned this pull request May 12, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#3663

Closed

github-actions Bot mentioned this pull request May 19, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#3920

Closed

github-actions Bot mentioned this pull request May 26, 2025

Directory Traversal SNYK-DEBIAN8-PYTHON27-341356 RADAR-base/radar-helm-charts#4178

Closed

+                      # bpo-35907: # disallow the file reading with the type not allowed
+                      if not hasattr(self, name) or \
+                          (self == _urlopener and name == 'open_local_file'):

[2.7] bpo-35907: Avoid file reading as disallowing the unnecessary URL scheme in urllib (GH-11842) by push0ebp · Pull Request #11842 · python/cpython · GitHub

Repository navigation

[2.7] bpo-35907: Avoid file reading as disallowing the unnecessary URL scheme in urllib (GH-11842) - #11842

[2.7] bpo-35907: Avoid file reading as disallowing the unnecessary URL scheme in urllib (GH-11842)#11842vstinner merged 6 commits intopython:2.7python/cpython:2.7from push0ebp:bpo-35907push0ebp/cpython:bpo-35907Copy head branch name to clipboard

Conversation

push0ebp commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 13, 2019

Uh oh!

tomashek commented Apr 8, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 13, 2019

Uh oh!

push0ebp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

push0ebp May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented May 21, 2019

Uh oh!

push0ebp commented May 22, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[2.7] bpo-35907: Avoid file reading as disallowing the unnecessary URL scheme in urllib (GH-11842)#11842
vstinner merged 6 commits into
python:2.7from
push0ebp:bpo-35907

push0ebp commented Feb 13, 2019 •
edited
Loading

push0ebp May 20, 2019 •
edited
Loading