Skip to content

/ cpython

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

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

Merged
merged 6 commits into from May 21, 2019
Merged

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

merged 6 commits into from May 21, 2019

Conversation

@push0ebp
Copy link
Contributor

push0ebp commented Feb 13, 2019 •
edited

https://bugs.python.org/issue35907
push0ebp push0ebp
bpo-35907: Avoid file reading as disallowing the unnecessary URL sche…
Loading status checks…
8f99cc7 
…me in urllib
@the-knights-who-say-ni

This comment has been minimized.

Sign in to view
Copy link

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

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!
@blurb-it
📜🤖 Added by blurb_it.
Loading status checks…
b863925
@push0ebp 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
@tomashek

This comment has been minimized.

Sign in to view
Copy link

tomashek commented Apr 8, 2019

Is this patch the accepted resolution for CVE-2019-9948? If so, when is it expected to be merged?
@hroncok
hroncok approved these changes May 7, 2019
View changes
@vstinner
vstinner reviewed May 13, 2019
View changes
Misc/NEWS.d/next/Library/2019-02-13-17-21-10.bpo-35907.ckk2zg.rst Outdated Show resolved Hide resolved
@vstinner
vstinner reviewed May 13, 2019
View changes
Lib/test/test_urllib.py Outdated Show resolved Hide resolved
@tiran
tiran requested changes May 13, 2019
View changes
Lib/urllib.py Outdated

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

This comment has been minimized.

Sign in to view
Copy link
@tiran

tiran May 13, 2019

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 19, 2019

Author Contributor

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?

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 19, 2019

Author Contributor

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.
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented May 13, 2019

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.
push0ebp added 2 commits May 16, 2019
@push0ebp
Update 2019-02-13-17-21-10.bpo-35907.ckk2zg.rst
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f20a31c 
Add prefix "CVE-2019-9948: "
@push0ebp
Update test_urllib.py and urllib.py\nchange assertEqual into assertRa…
Loading status checks…
179a5f7 
…sies in DummyURLopener test, and simplify mitigation
@push0ebp
push0ebp reviewed May 19, 2019
View changes
Copy link
Contributor Author

push0ebp left a comment

I have made the requested changes; please review again.
Lib/test/test_urllib.py Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Library/2019-02-13-17-21-10.bpo-35907.ckk2zg.rst Outdated Show resolved Hide resolved
Lib/urllib.py Outdated

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

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 19, 2019

Author Contributor

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?
Lib/urllib.py Outdated

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

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 19, 2019

Author Contributor

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.
@csabella csabella requested a review from tiran May 19, 2019
@vstinner
vstinner reviewed May 20, 2019
View changes
Lib/urllib.py Outdated

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

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 20, 2019

Member

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 :-(

This comment has been minimized.

Sign in to view
Copy link
@tiran

tiran May 20, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 20, 2019 •
edited

Author Contributor

@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?

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 20, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 20, 2019

Author Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 21, 2019

Member

Please remove getattr().

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 21, 2019

Author Contributor

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':

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 21, 2019

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

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 21, 2019

Author Contributor

I have changed it. Please review.

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 21, 2019

Author Contributor

I have made the requested changes; please review again.
@vstinner
vstinner approved these changes May 20, 2019
View changes
Copy link
Member

vstinner left a comment

LGTM.
@push0ebp
Update urllib.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
3cda03c 
Modify the object to string in check method name.
@vstinner
vstinner approved these changes May 21, 2019
View changes
Copy link
Member

vstinner left a comment

LGTM. Just curious about the # which looks like a typo.
Lib/urllib.py Outdated
@@ -203,7 +203,9 @@ def open(self, fullurl, data=None):
name = 'open_' + urltype
self.type = urltype
name = name.replace('-', '_')
if not hasattr(self, name):

# bpo-35907: # disallow the file reading with the type not allowed

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 21, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 21, 2019

Author Contributor

Oh Sorry, my mistake. I will correct it.

This comment has been minimized.

Sign in to view
Copy link
@push0ebp

push0ebp May 21, 2019

Author Contributor

I have fixed the typo.
@push0ebp
Update urllib.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8b7d7ab 
Fix typo
@vstinner vstinner merged commit b15bde8 into python:2.7 May 21, 2019
5 checks passed
5 checks passed
bedevere/issue-number Issue number 35907 found
Details
bedevere/maintenance-branch-pr Valid maintenance branch PR title.
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
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member

vstinner commented May 21, 2019

Thanks @push0ebp for the fix!
@vstinner vstinner mentioned this pull request May 21, 2019
Merged
@push0ebp

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

push0ebp commented May 22, 2019

Thank you for your accept.
related to #13474
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner

@hroncok hroncok

@tiran tiran

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@push0ebp @the-knights-who-say-ni @tomashek @bedevere-bot @vstinner @tiran @hroncok
You can’t perform that action at this time.