homepage

We received the following on the security list. With the OP's permission I am now filing a public bug with a patch, with the intent to submit the patch ASAP (in time for MvL's planned April security release of Python 2.5).
The OP's description is below; I will attach a patch to this issue as soon as I have figured out how.
description:
--------------------
The Python urllib and urllib2 modules are typically used to fetch web
pages but by default also contains handlers for ftp:// and file:// URL
schemes.
Now unfortunately it appears that it is possible for a web server to
redirect (HTTP 302) a urllib request to any of the supported
schemes. Examples on how this could turn bad:
 1) File disclosure: A web application, that normally fetches and
 displays a web page, is redirected to file:///etc/passwd and
 discloses it.
 2) Denial of Service: An application is redirected to a system device
 (e.g. file:///dev/zero) which will result in excessive CPU/memory/disk
 usage.
Affected versions:
------------------
The urllib and urllib2 modules of python 2.4.6 and 2.6.5 where tested
but this likely affects all versions.
Possible solution:
------------------
The default handlers could be reduced but this will probably break
existing python scripts.
Alternatively the default HTTPRedirectHandler behaviour can be changed
to only allow redirects to HTTP, HTTPS and FTP by checking the scheme
of the location URL (this seems to be a common practise in browsers)

>> HTTPRedirectHandler behaviour can be changed
>> to only allow redirects to HTTP, HTTPS and FTP by checking the scheme
>> of the location URL (this seems to be a common practise in browsers)
This would be the way to go.

Repository URL is incorrect (missing http:/ prefix). The commit:
http://hg.python.org/sandbox/guido/rev/dd852a0f92d6 

Please review the patch that I created. (Now why doesn't it have a "review" link?) Note that the patch currently only allows http and https.

>> why doesn't it have a "review" link?
Perhaps, as it is not against the 'default'?
Let's try my hg sandbox link which has a fix committed. Let's see if it gives the review link.

The patch has no test. You may read our new "Python Developer’s Guide" for new contributors:
http://docs.python.org/devguide/runtests.html#writing 

Oddly, I now see a review link for my own diff but not for orsenthil's. Maybe there's a delay?
I could use help with the tests.
I suppose orsenthil's patch is for Python 3?

Which patch should be reviewed? They seem to be different. Senthil's patch allows a redirect to ftp while Guido's doesn't.
Senthil's patch doesn't seem to fix urllib-inherited code, only urllib2- (see FancyURLopener.redirect_internal()).
Guido's patch doesn't close the file (fp.close()) when the redirect is denied.
Both patches apparently return silently (?), while it might be better to raise an exception.
Both would deserve a test :)

c6a4d267fe88.diff: This patch doesn't explain why other scheme are not allowed. I like Guido's comment:
# For security reasons we do not allow redirects to protocols
# other than HTTP or HTTPS.

Yes there is a delay. The cron job that creates the link runs every two minutes. Not sure why the delay seems to be longer than that, though.

> Which patch should be reviewed? They seem to be different.
Both. Mine's for the Python 2 line while Senthil seems to deal with
Python 3. (However the presence of Senthil's patch somehow overrode my
patch in Rietveld. It looks like Martin didn't think of this use
case.) I'd like to have agreement over the Python 2 patch first, then
we can think about forward porting.
> Senthil's patch allows a redirect to ftp while Guido's doesn't.
That is a good question. Should we? It doesn't look like ftp:
participates in the vulnerability, but I'm not sure how useful it is
either.
> Senthil's patch doesn't seem to fix urllib-inherited code, only urllib2- (see FancyURLopener.redirect_internal()).
Right, that's for Python 3.
> Guido's patch doesn't close the file (fp.close()) when the redirect is denied.
But the calling code does. Note that when there is no URI or Location
header, redirect_internal() also returns without closing the file; if
the error handler returns no result, http_error() will call
http_error_default() which closes the file.
> Both patches apparently return silently (?), while it might be better to raise an exception.
This follows the tradition of returning silently when no URI or
Location header is found. The 302 error will be treated the same as
any other error.
> Both would deserve a test :)
If someone would contribute one I'd appreciate it. Otherwise I will
get on it myself.

Here is a more complete patch with tests. Please review this. Yes, it is against the default branch (3.x codeline). We can backport this behavior to 2.x codeline.
I have raised an URLError exception when the direct to invalid_schemes is detected.
Also, ftp redirection should be allowed. It is common to see ISO download mirrors which will redirect itself to an ftp url. Also the security report says about allowing to http, https and ftp.
Thanks.

> > Senthil's patch allows a redirect to ftp while Guido's doesn't.
> 
> That is a good question. Should we? It doesn't look like ftp:
> participates in the vulnerability, but I'm not sure how useful it is
> either.
I would say accept it anyway. That way we minimize potential for
compatibility breakage.
(do we support "ftps" as well? I don't think so)
> > Senthil's patch doesn't seem to fix urllib-inherited code, only
> urllib2- (see FancyURLopener.redirect_internal()).
> 
> Right, that's for Python 3.
FancyURLopener is still present in Python 3 (even though we would like
to deprecate it in 3.3).

I am okay with adding FTP to the list.
I still don't think we should raise URLError on the bad redirect; we should treat it the same as a missing URI/Location header, and it will raise HTTPError.

On Thu, Mar 24, 2011 at 05:32:42PM +0000, Guido van Rossum wrote:
> I still don't think we should raise URLError on the bad redirect; we
> should treat it the same as a missing URI/Location header, and it
> will raise HTTPError.
Agreed. Updated the hg repository by raising HTTPError instead of
URLError.
Thing to note - HTTPError does not change anything from the
redirection. It would still give the code as 302 with an additional
message saying that Redirection to 'newurl' is not allowed.

CVE-2011-1521 has been assigned to this issue.

Aha. I now see the point of raising an exception instead of just returning None. I have backported Senthil's patch to the 2.5 branch. Please review.

This issue was first reported by Niels Heinen from the Google Security Team.

I don't have a 2.5 checkout to test but the patch looks ok to me.
Under 2.7 I get a test failure, I suppose you'll have some merging work to do:
test test_urllib2 failed -- Traceback (most recent call last):
 File "/home/antoine/cpython/27/Lib/test/test_urllib2.py", line 990, in test_invalid_redirect
 MockHeaders({"location": valid_url}))
 File "/home/antoine/cpython/27/Lib/urllib2.py", line 616, in http_error_302
 return self.parent.open(new, timeout=req.timeout)
 File "/home/antoine/cpython/27/Lib/urllib2.py", line 219, in __getattr__
 raise AttributeError, attr
AttributeError: timeout

I have the final version of the patch for Python 2 in the 2.5, 2.6 and 2.7 branches in my repo (http://hg.python.org/sandbox/guido).
What's the next step? Just push this to the central repo? There are a few separate changes:
summary: Merge urllib/urllib2 security fix from 2.6 branch.
summary: Merge urllib/urllib2 security fix from 2.5 branch.
summary: Adding .hgignore (copied from default branch).
summary: Add CVE number to urllib/urllib2 news item.
summary: Add tests for the urllib[2] vulnerability. Change to raise exceptions.
summary: Add FTP to the allowed url schemes. Add Misc/NEWS.
summary: Issue 22663: fix redirect vulnerability in urllib/urllib2.

Also for the Python 3 family it's best to backport Senthil's patch. I will try that in my tree as well.

The fix is now also in the 3.1, 3.2 and default branches of my repo
(http://hg.python.org/sandbox/guido).
Maybe I should just merge the whole bunch into the root repo and be
done with it?

2011年3月29日 Guido van Rossum <report@bugs.python.org>:
>
> Guido van Rossum <guido@python.org> added the comment:
>
> The fix is now also in the 3.1, 3.2 and default branches of my repo
> (http://hg.python.org/sandbox/guido).
>
> Maybe I should just merge the whole bunch into the root repo and be
> done with it?
Sounds good.
BTW, you should probably put your name your hg username messages by
setting username in .hgrc to
username = "Guido van Rossum <guido@python.org>"
or so

New changeset 5937d2119a20 by guido in branch '3.1':
Issue 11662: Fix vulnerability in urllib/urllib2.
http://hg.python.org/cpython/rev/5937d2119a20
New changeset 96a6c128822b by guido in branch '3.2':
Merge Issue 11662 from 3.1 branch.
http://hg.python.org/cpython/rev/96a6c128822b
New changeset a778b963eae3 by guido in branch 'default':
Merge Issue 11662 from 3.2 branch.
http://hg.python.org/cpython/rev/a778b963eae3
New changeset 9eeda8e3a13f by Guido van Rossum in branch '2.6':
Merge issue 11662 from 2.5.
http://hg.python.org/cpython/rev/9eeda8e3a13f
New changeset b2934d98dac1 by Guido van Rossum in branch '2.7':
Merge issue 11662 from 2.6.
http://hg.python.org/cpython/rev/b2934d98dac1
New changeset 3dc90ebc540a by Guido van Rossum in branch '3.1':
Merge issue 11662.
http://hg.python.org/cpython/rev/3dc90ebc540a
New changeset 968bca2cab60 by Guido van Rossum in branch '3.2':
Merge issue 11662.
http://hg.python.org/cpython/rev/968bca2cab60
New changeset c8701b9256cf by Guido van Rossum in branch 'default':
Merge issue 11662.
http://hg.python.org/cpython/rev/c8701b9256cf 

Ok, merged into the central repo. Let me know where I screwed up.

I think this is another patch that needs to be cross-ported to the 2.6 svn branch (which I'll do).