Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fixed a security issue related to password resets
Full disclosure and new release are forthcoming
  • Loading branch information
ptone committed Oct 17, 2012
1 parent 3e08570 commit 9305c0e
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 1 deletion.
1 change: 1 addition & 0 deletions django/contrib/auth/tests/urls.py
Expand Up @@ -55,6 +55,7 @@ def userpage(request):
(r'^logout/next_page/$', 'django.contrib.auth.views.logout', dict(next_page='/somewhere/')),
(r'^remote_user/$', remote_user_auth_view),
(r'^password_reset_from_email/$', 'django.contrib.auth.views.password_reset', dict(from_email='staffmember@example.com')),
(r'^admin_password_reset/$', 'django.contrib.auth.views.password_reset', dict(is_admin_site=True)),
(r'^login_required/$', login_required(password_reset)),
(r'^login_required_login_url/$', login_required(password_reset, login_url='/somewhere/')),

Expand Down
37 changes: 37 additions & 0 deletions django/contrib/auth/tests/views.py
Expand Up @@ -5,6 +5,7 @@
from django.contrib.sites.models import Site, RequestSite
from django.contrib.auth.models import User
from django.core import mail
from django.core.exceptions import SuspiciousOperation
from django.core.urlresolvers import reverse, NoReverseMatch
from django.http import QueryDict
from django.utils.encoding import force_text
Expand Down Expand Up @@ -103,6 +104,42 @@ def test_email_found_custom_from(self):
self.assertEqual(len(mail.outbox), 1)
self.assertEqual("staffmember@example.com", mail.outbox[0].from_email)

def test_admin_reset(self):
"If the reset view is marked as being for admin, the HTTP_HOST header is used for a domain override."
response = self.client.post('/admin_password_reset/',
{'email': 'staffmember@example.com'},
HTTP_HOST='adminsite.com'
)
self.assertEqual(response.status_code, 302)
self.assertEqual(len(mail.outbox), 1)
self.assertTrue("http://adminsite.com" in mail.outbox[0].body)
self.assertEqual(settings.DEFAULT_FROM_EMAIL, mail.outbox[0].from_email)

def test_poisoned_http_host(self):
"Poisoned HTTP_HOST headers can't be used for reset emails"
# This attack is based on the way browsers handle URLs. The colon
# should be used to separate the port, but if the URL contains an @,
# the colon is interpreted as part of a username for login purposes,
# making 'evil.com' the request domain. Since HTTP_HOST is used to
# produce a meaningful reset URL, we need to be certain that the
# HTTP_HOST header isn't poisoned. This is done as a check when get_host()
# is invoked, but we check here as a practical consequence.
with self.assertRaises(SuspiciousOperation):

This comment has been minimized.

Copy link
@nvictor

nvictor Oct 23, 2012

This statement makes the test fail, because the view won't raise that exception directly since Django will try to handle the display of server errors by default. A quick fix would be to set DEBUG_PROPAGATE_EXCEPTIONS = True in your test settings. But I guess assertContains(response, 'SuspiciousOperation') could work here.

self.client.post('/password_reset/',
{'email': 'staffmember@example.com'},
HTTP_HOST='www.example:dr.frankenstein@evil.tld'
)
self.assertEqual(len(mail.outbox), 0)

def test_poisoned_http_host_admin_site(self):
"Poisoned HTTP_HOST headers can't be used for reset emails on admin views"
with self.assertRaises(SuspiciousOperation):

This comment has been minimized.

Copy link
@nvictor

nvictor Oct 23, 2012

If my note above is valid then this one is concerned too.

self.client.post('/admin_password_reset/',
{'email': 'staffmember@example.com'},
HTTP_HOST='www.example:dr.frankenstein@evil.tld'
)
self.assertEqual(len(mail.outbox), 0)

def _test_confirm_start(self):
# Start by creating the email
response = self.client.post('/password_reset/', {'email': 'staffmember@example.com'})
Expand Down
2 changes: 1 addition & 1 deletion django/contrib/auth/views.py
Expand Up @@ -163,7 +163,7 @@ def password_reset(request, is_admin_site=False,
'request': request,
}
if is_admin_site:
opts = dict(opts, domain_override=request.META['HTTP_HOST'])
opts = dict(opts, domain_override=request.get_host())
form.save(**opts)
return HttpResponseRedirect(post_reset_redirect)
else:
Expand Down
5 changes: 5 additions & 0 deletions django/http/__init__.py
Expand Up @@ -180,6 +180,11 @@ def get_host(self):
server_port = str(self.META['SERVER_PORT'])
if server_port != ('443' if self.is_secure() else '80'):
host = '%s:%s' % (host, server_port)

# Disallow potentially poisoned hostnames.
if set(';/?@&=+$,').intersection(host):
raise SuspiciousOperation('Invalid HTTP_HOST header: %s' % host)

return host

def get_full_path(self):
Expand Down

0 comments on commit 9305c0e

Please sign in to comment.