classification
Title: Redirect vulnerability in urllib/urllib2
Type: security Stage: patch review
Components: Library (Lib) Versions: Python 3.4, Python 3.3, Python 3.2, Python 3.1, Python 2.7, Python 2.6, Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gvanrossum Nosy List: Arfrever, Henri.Salo, barry, benjamin.peterson, georg.brandl, gvanrossum, haypo, jcea, orsenthil, pitrou, python-dev, r.david.murray
Priority: release blocker Keywords: patch

Created on 2011-03-24 15:06 by gvanrossum, last changed 2011-05-20 20:52 by barry. This issue is now closed.

Files
File name Uploaded Description Edit
dd852a0f92d6.diff gvanrossum, 2011-03-24 15:09 review
c6a4d267fe88.diff orsenthil, 2011-03-24 15:25 review
3c07ea6a176a.diff orsenthil, 2011-03-24 17:02 review
ca3b117c40f3.diff gvanrossum, 2011-03-24 17:45 review
ff71c4416cde.diff orsenthil, 2011-03-25 02:45 review
f03e2acb9826.diff gvanrossum, 2011-03-28 20:54 review
Repositories containing patches
http://hg.python.org/sandbox/guido#2.5
http://hg.python.org/sandbox/orsenthil/
Messages (26)
msg131981 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-24 15:06
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)
msg131982 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-03-24 15:09
>> 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.
msg131983 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-03-24 15:10
Repository URL is incorrect (missing http:/ prefix). The commit:
http://hg.python.org/sandbox/guido/rev/dd852a0f92d6
msg131984 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-24 15:11
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.
msg131988 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-03-24 15:24
>> 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.
msg131989 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-03-24 15:29
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
msg131990 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-24 15:35
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?
msg131991 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-24 15:36
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 :)
msg131992 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-03-24 15:38
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.
msg131993 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-03-24 15:39
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.
msg132008 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-24 16:52
> 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.
msg132009 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-03-24 17:07
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.
msg132010 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-24 17:08
> > 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).
msg132011 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-24 17:32
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.
msg132068 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-03-25 02:42
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.
msg132405 - (view) Author: Henri Salo (Henri.Salo) Date: 2011-03-28 16:27
CVE-2011-1521 has been assigned to this issue.
msg132419 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-28 20:48
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.
msg132487 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-29 16:43
This issue was first reported by Niels Heinen from the Google Security Team.
msg132488 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-29 16:48
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
msg132495 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-29 18:22
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.
msg132496 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-29 18:23
Also for the Python 3 family it's best to backport Senthil's patch. I will try that in my tree as well.
msg132500 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-29 19:06
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?
msg132501 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-03-29 19:10
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
msg132517 - (view) Author: Roundup Robot (python-dev) Date: 2011-03-29 20:10
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
msg132518 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-03-29 20:12
Ok, merged into the central repo. Let me know where I screwed up.
msg136401 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2011-05-20 20:52
I think this is another patch that needs to be cross-ported to the 2.6 svn branch (which I'll do).
History
Date User Action Args
2011-05-20 20:52:14barrysetmessages: + msg136401
2011-04-25 10:45:10jceasetnosy: + jcea
2011-03-29 20:26:14Arfreversetnosy: + Arfrever
2011-03-29 20:12:20gvanrossumsetstatus: open -> closed
resolution: fixed
messages: + msg132518
2011-03-29 20:10:56python-devsetnosy: + python-dev
messages: + msg132517
2011-03-29 19:10:59benjamin.petersonsetmessages: + msg132501
2011-03-29 19:06:11gvanrossumsetmessages: + msg132500
2011-03-29 18:23:47gvanrossumsetmessages: + msg132496
2011-03-29 18:22:29gvanrossumsetnosy: - serdar.dalgic

messages: + msg132495
versions: + Python 3.4
2011-03-29 16:48:32pitrousetmessages: + msg132488
versions: - Python 3.4
2011-03-29 16:43:53gvanrossumsetmessages: + msg132487
2011-03-29 11:18:20serdar.dalgicsetnosy: + serdar.dalgic
2011-03-28 20:58:10gvanrossumsetfiles: - 9d06d5eb1a7e.diff
2011-03-28 20:54:36gvanrossumsetfiles: + f03e2acb9826.diff
2011-03-28 20:48:58gvanrossumsetmessages: + msg132419
2011-03-28 20:47:50gvanrossumsetfiles: + 9d06d5eb1a7e.diff
2011-03-28 16:27:20Henri.Salosetnosy: + Henri.Salo
messages: + msg132405
2011-03-25 02:45:06orsenthilsetfiles: + ff71c4416cde.diff
2011-03-25 02:42:05orsenthilsetmessages: + msg132068
2011-03-24 17:45:28gvanrossumsetfiles: + ca3b117c40f3.diff
2011-03-24 17:32:42gvanrossumsetmessages: + msg132011
2011-03-24 17:08:38pitrousetmessages: + msg132010
2011-03-24 17:07:07orsenthilsetmessages: + msg132009
2011-03-24 17:02:16orsenthilsetfiles: + 3c07ea6a176a.diff
2011-03-24 16:52:35gvanrossumsetmessages: + msg132008
2011-03-24 15:39:09r.david.murraysetnosy: + r.david.murray
messages: + msg131993
2011-03-24 15:38:04hayposetmessages: + msg131992
2011-03-24 15:36:27pitrousetnosy: + pitrou
messages: + msg131991
2011-03-24 15:35:04gvanrossumsetmessages: + msg131990
2011-03-24 15:29:17hayposetmessages: + msg131989
2011-03-24 15:25:01orsenthilsetfiles: + c6a4d267fe88.diff
2011-03-24 15:24:30orsenthilsethgrepos: + hgrepo7
messages: + msg131988
2011-03-24 15:11:43gvanrossumsetmessages: + msg131984
2011-03-24 15:10:30hayposetnosy: + haypo
messages: + msg131983
2011-03-24 15:09:37gvanrossumsetfiles: + dd852a0f92d6.diff
keywords: + patch
2011-03-24 15:09:30orsenthilsetnosy: + orsenthil
messages: + msg131982
2011-03-24 15:06:57gvanrossumcreate