From ad32c218850ad40972dcef57beb460f8c979dd6d Mon Sep 17 00:00:00 2001 From: Erik Romijn Date: Mon, 12 May 2014 09:42:06 -0400 Subject: [PATCH] [1.5.x] Added additional checks in is_safe_url to account for flexible parsing. This is a security fix. Disclosure following shortly. --- django/contrib/auth/tests/views.py | 12 ++++++++---- django/utils/http.py | 12 ++++++++++++ tests/regressiontests/utils/http.py | 29 +++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/django/contrib/auth/tests/views.py b/django/contrib/auth/tests/views.py index e39dc0ea80bc8..ba2b48a98bd9e 100644 --- a/django/contrib/auth/tests/views.py +++ b/django/contrib/auth/tests/views.py @@ -326,8 +326,10 @@ def test_security_check(self, password='password'): # Those URLs should not pass the security check for bad_url in ('http://example.com', + 'http:///example.com', 'https://example.com', 'ftp://exampel.com', + '///example.com', '//example.com', 'javascript:alert("XSS")'): @@ -349,8 +351,8 @@ def test_security_check(self, password='password'): '/view/?param=https://example.com', '/view?param=ftp://exampel.com', 'view/?param=//example.com', - 'https:///', - 'HTTPS:///', + 'https://testserver/', + 'HTTPS://testserver/', '//testserver/', '/url%20with%20spaces/'): # see ticket #12534 safe_url = '%(url)s?%(next)s=%(good_url)s' % { @@ -521,8 +523,10 @@ def test_security_check(self, password='password'): # Those URLs should not pass the security check for bad_url in ('http://example.com', + 'http:///example.com', 'https://example.com', 'ftp://exampel.com', + '///example.com', '//example.com', 'javascript:alert("XSS")'): nasty_url = '%(url)s?%(next)s=%(bad_url)s' % { @@ -542,8 +546,8 @@ def test_security_check(self, password='password'): '/view/?param=https://example.com', '/view?param=ftp://exampel.com', 'view/?param=//example.com', - 'https:///', - 'HTTPS:///', + 'https://testserver/', + 'HTTPS://testserver/', '//testserver/', '/url%20with%20spaces/'): # see ticket #12534 safe_url = '%(url)s?%(next)s=%(good_url)s' % { diff --git a/django/utils/http.py b/django/utils/http.py index f376b1c1171e0..67912f7aaa819 100644 --- a/django/utils/http.py +++ b/django/utils/http.py @@ -237,6 +237,18 @@ def is_safe_url(url, host=None): """ if not url: return False + # Chrome treats \ completely as / + url = url.replace('\\', '/') + # Chrome considers any URL with more than two slashes to be absolute, but + # urlaprse is not so flexible. Treat any url with three slashes as unsafe. + if url.startswith('///'): + return False url_info = urllib_parse.urlparse(url) + # Forbid URLs like http:///example.com - with a scheme, but without a hostname. + # In that URL, example.com is not the hostname but, a path component. However, + # Chrome will still consider example.com to be the hostname, so we must not + # allow this syntax. + if not url_info.netloc and url_info.scheme: + return False return (not url_info.netloc or url_info.netloc == host) and \ (not url_info.scheme or url_info.scheme in ['http', 'https']) diff --git a/tests/regressiontests/utils/http.py b/tests/regressiontests/utils/http.py index 03706ca68db4f..87a6ba41923c9 100644 --- a/tests/regressiontests/utils/http.py +++ b/tests/regressiontests/utils/http.py @@ -91,6 +91,35 @@ def test_base36(self): self.assertEqual(http.int_to_base36(n), b36) self.assertEqual(http.base36_to_int(b36), n) + def test_is_safe_url(self): + for bad_url in ('http://example.com', + 'http:///example.com', + 'https://example.com', + 'ftp://exampel.com', + r'\\example.com', + r'\\\example.com', + r'/\\/example.com', + r'\\\example.com', + r'\\example.com', + r'\\//example.com', + r'/\/example.com', + r'\/example.com', + r'/\example.com', + 'http:///example.com', + 'http:/\//example.com', + 'http:\/example.com', + 'http:/\example.com', + 'javascript:alert("XSS")'): + self.assertFalse(http.is_safe_url(bad_url, host='testserver'), "%s should be blocked" % bad_url) + for good_url in ('/view/?param=http://example.com', + '/view/?param=https://example.com', + '/view?param=ftp://exampel.com', + 'view/?param=//example.com', + 'https://testserver/', + 'HTTPS://testserver/', + '//testserver/', + '/url%20with%20spaces/'): + self.assertTrue(http.is_safe_url(good_url, host='testserver'), "%s should be allowed" % good_url) class ETagProcessingTests(unittest.TestCase): def testParsing(self):