Redirect vulnerability in urllib/urllib2

[gvanrossum]

BPO	11662
Nosy	@gvanrossum, @warsaw, @birkenfeld, @jcea, @orsenthil, @pitrou, @vstinner, @benjaminp, @bitdancer
Files	dd852a0f92d6.diff c6a4d267fe88.diff 3c07ea6a176a.diff ca3b117c40f3.diff ff71c4416cde.diff f03e2acb9826.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gvanrossum'
closed_at = <Date 2011-03-29.20:12:20.192>
created_at = <Date 2011-03-24.15:06:57.398>
labels = ['type-security', 'library', 'release-blocker']
title = 'Redirect vulnerability in urllib/urllib2'
updated_at = <Date 2011-05-20.20:52:14.152>
user = 'https://github.com/gvanrossum'

bugs.python.org fields:

activity = <Date 2011-05-20.20:52:14.152>
actor = 'barry'
assignee = 'gvanrossum'
closed = True
closed_date = <Date 2011-03-29.20:12:20.192>
closer = 'gvanrossum'
components = ['Library (Lib)']
creation = <Date 2011-03-24.15:06:57.398>
creator = 'gvanrossum'
dependencies = []
files = ['21372', '21374', '21376', '21377', '21388', '21442']
hgrepos = ['6', '7']
issue_num = 11662
keywords = ['patch']
message_count = 26.0
messages = ['131981', '131982', '131983', '131984', '131988', '131989', '131990', '131991', '131992', '131993', '132008', '132009', '132010', '132011', '132068', '132405', '132419', '132487', '132488', '132495', '132496', '132500', '132501', '132517', '132518', '136401']
nosy_count = 12.0
nosy_names = ['gvanrossum', 'barry', 'georg.brandl', 'jcea', 'orsenthil', 'pitrou', 'vstinner', 'benjamin.peterson', 'Arfrever', 'r.david.murray', 'Henri.Salo', 'python-dev']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue11662'
versions = ['Python 2.6', 'Python 2.5', 'Python 3.1', 'Python 2.7', 'Python 3.2', 'Python 3.3', 'Python 3.4']

[gvanrossum]

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)

[orsenthil]

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

[vstinner]

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

[gvanrossum]

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.

[orsenthil]

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

[vstinner]

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

[gvanrossum]

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?

[pitrou]

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

[vstinner]

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.

[bitdancer]

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.

[gvanrossum]

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.