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
Conversation
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 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? |
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'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Add prefix "CVE-2019-9948: "
…sies in DummyURLopener test, and simplify mitigation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the requested changes; please review again.
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'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you compare function objects instead of simply name == "open_local_file"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur with Christian, please test directly the name, remove getattr().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove getattr().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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':
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed it. Please review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the requested changes; please review again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Modify the object to string in check method name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the # in "# bpo-35907: # disallow ...". Is it a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh Sorry, my mistake. I will correct it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed the typo.
Fix typo
Thanks @push0ebp for the fix! |
Thank you for your accept. |
https://bugs.python.org/issue35907