From c4e5ff7fdb5fce447675e90291fd33fddd052b3c Mon Sep 17 00:00:00 2001 From: Andreas Hug Date: Tue, 24 Jul 2018 16:18:17 -0400 Subject: [PATCH] [2.1.x] Fixed CVE-2018-14574 -- Fixed open redirect possibility in CommonMiddleware. --- django/middleware/common.py | 3 +++ django/urls/resolvers.py | 6 ++---- django/utils/http.py | 11 +++++++++++ docs/releases/1.11.15.txt | 13 +++++++++++++ docs/releases/2.0.8.txt | 13 +++++++++++++ tests/middleware/tests.py | 19 +++++++++++++++++++ tests/middleware/urls.py | 2 ++ tests/utils_tests/test_http.py | 19 +++++++++++++++---- 8 files changed, 78 insertions(+), 8 deletions(-) diff --git a/django/middleware/common.py b/django/middleware/common.py index bea3f7448a..a18fbe7b47 100644 --- a/django/middleware/common.py +++ b/django/middleware/common.py @@ -7,6 +7,7 @@ from django.core.mail import mail_managers from django.http import HttpResponsePermanentRedirect from django.urls import is_valid_path from django.utils.deprecation import MiddlewareMixin +from django.utils.http import escape_leading_slashes class CommonMiddleware(MiddlewareMixin): @@ -79,6 +80,8 @@ class CommonMiddleware(MiddlewareMixin): POST, PUT, or PATCH. """ new_path = request.get_full_path(force_append_slash=True) + # Prevent construction of scheme relative urls. + new_path = escape_leading_slashes(new_path) if settings.DEBUG and request.method in ('POST', 'PUT', 'PATCH'): raise RuntimeError( "You called this URL via %(method)s, but the URL doesn't end " diff --git a/django/urls/resolvers.py b/django/urls/resolvers.py index ce8c7ffa32..5bfab0c067 100644 --- a/django/urls/resolvers.py +++ b/django/urls/resolvers.py @@ -17,7 +17,7 @@ from django.core.checks.urls import check_resolver from django.core.exceptions import ImproperlyConfigured from django.utils.datastructures import MultiValueDict from django.utils.functional import cached_property -from django.utils.http import RFC3986_SUBDELIMS +from django.utils.http import RFC3986_SUBDELIMS, escape_leading_slashes from django.utils.regex_helper import normalize from django.utils.translation import get_language @@ -592,9 +592,7 @@ class URLResolver: # safe characters from `pchar` definition of RFC 3986 url = quote(candidate_pat % text_candidate_subs, safe=RFC3986_SUBDELIMS + '/~:@') # Don't allow construction of scheme relative urls. - if url.startswith('//'): - url = '/%%2F%s' % url[2:] - return url + return escape_leading_slashes(url) # lookup_view can be URL name or callable, but callables are not # friendly in error messages. m = getattr(lookup_view, '__module__', None) diff --git a/django/utils/http.py b/django/utils/http.py index 4558c6874a..e33ec7362b 100644 --- a/django/utils/http.py +++ b/django/utils/http.py @@ -433,3 +433,14 @@ def limited_parse_qsl(qs, keep_blank_values=False, encoding='utf-8', value = unquote(value, encoding=encoding, errors=errors) r.append((name, value)) return r + + +def escape_leading_slashes(url): + """ + If redirecting to an absolute path (two leading slashes), a slash must be + escaped to prevent browsers from handling the path as schemaless and + redirecting to another host. + """ + if url.startswith('//'): + url = '/%2F{}'.format(url[2:]) + return url diff --git a/docs/releases/1.11.15.txt b/docs/releases/1.11.15.txt index 397681d52e..fca551e772 100644 --- a/docs/releases/1.11.15.txt +++ b/docs/releases/1.11.15.txt @@ -5,3 +5,16 @@ Django 1.11.15 release notes *August 1, 2018* Django 1.11.15 fixes a security issue in 1.11.14. + +CVE-2018-14574: Open redirect possibility in ``CommonMiddleware`` +================================================================= + +If the :class:`~django.middleware.common.CommonMiddleware` and the +:setting:`APPEND_SLASH` setting are both enabled, and if the project has a +URL pattern that accepts any path ending in a slash (many content management +systems have such a pattern), then a request to a maliciously crafted URL of +that site could lead to a redirect to another site, enabling phishing and other +attacks. + +``CommonMiddleware`` now escapes leading slashes to prevent redirects to other +domains. diff --git a/docs/releases/2.0.8.txt b/docs/releases/2.0.8.txt index 8a03071075..849f80d3f8 100644 --- a/docs/releases/2.0.8.txt +++ b/docs/releases/2.0.8.txt @@ -6,6 +6,19 @@ Django 2.0.8 release notes Django 2.0.8 fixes a security issue and several bugs in 2.0.7. +CVE-2018-14574: Open redirect possibility in ``CommonMiddleware`` +================================================================= + +If the :class:`~django.middleware.common.CommonMiddleware` and the +:setting:`APPEND_SLASH` setting are both enabled, and if the project has a +URL pattern that accepts any path ending in a slash (many content management +systems have such a pattern), then a request to a maliciously crafted URL of +that site could lead to a redirect to another site, enabling phishing and other +attacks. + +``CommonMiddleware`` now escapes leading slashes to prevent redirects to other +domains. + Bugfixes ======== diff --git a/tests/middleware/tests.py b/tests/middleware/tests.py index f3c8b9ca06..88e33348e6 100644 --- a/tests/middleware/tests.py +++ b/tests/middleware/tests.py @@ -130,6 +130,25 @@ class CommonMiddlewareTest(SimpleTestCase): self.assertEqual(r.status_code, 301) self.assertEqual(r.url, '/needsquoting%23/') + @override_settings(APPEND_SLASH=True) + def test_append_slash_leading_slashes(self): + """ + Paths starting with two slashes are escaped to prevent open redirects. + If there's a URL pattern that allows paths to start with two slashes, a + request with path //evil.com must not redirect to //evil.com/ (appended + slash) which is a schemaless absolute URL. The browser would navigate + to evil.com/. + """ + # Use 4 slashes because of RequestFactory behavior. + request = self.rf.get('////evil.com/security') + response = HttpResponseNotFound() + r = CommonMiddleware().process_request(request) + self.assertEqual(r.status_code, 301) + self.assertEqual(r.url, '/%2Fevil.com/security/') + r = CommonMiddleware().process_response(request, response) + self.assertEqual(r.status_code, 301) + self.assertEqual(r.url, '/%2Fevil.com/security/') + @override_settings(APPEND_SLASH=False, PREPEND_WWW=True) def test_prepend_www(self): request = self.rf.get('/path/') diff --git a/tests/middleware/urls.py b/tests/middleware/urls.py index 8c6621d059..d623e7d6af 100644 --- a/tests/middleware/urls.py +++ b/tests/middleware/urls.py @@ -6,4 +6,6 @@ urlpatterns = [ url(r'^noslash$', views.empty_view), url(r'^slash/$', views.empty_view), url(r'^needsquoting#/$', views.empty_view), + # Accepts paths with two leading slashes. + url(r'^(.+)/security/$', views.empty_view), ] diff --git a/tests/utils_tests/test_http.py b/tests/utils_tests/test_http.py index 86fcff9d8e..d2a68ccd69 100644 --- a/tests/utils_tests/test_http.py +++ b/tests/utils_tests/test_http.py @@ -5,10 +5,10 @@ from django.test import SimpleTestCase, ignore_warnings from django.utils.datastructures import MultiValueDict from django.utils.deprecation import RemovedInDjango30Warning from django.utils.http import ( - base36_to_int, cookie_date, http_date, int_to_base36, is_safe_url, - is_same_domain, parse_etags, parse_http_date, quote_etag, urlencode, - urlquote, urlquote_plus, urlsafe_base64_decode, urlsafe_base64_encode, - urlunquote, urlunquote_plus, + base36_to_int, cookie_date, escape_leading_slashes, http_date, + int_to_base36, is_safe_url, is_same_domain, parse_etags, parse_http_date, + quote_etag, urlencode, urlquote, urlquote_plus, urlsafe_base64_decode, + urlsafe_base64_encode, urlunquote, urlunquote_plus, ) @@ -271,3 +271,14 @@ class HttpDateProcessingTests(unittest.TestCase): def test_parsing_asctime(self): parsed = parse_http_date('Sun Nov 6 08:49:37 1994') self.assertEqual(datetime.utcfromtimestamp(parsed), datetime(1994, 11, 6, 8, 49, 37)) + + +class EscapeLeadingSlashesTests(unittest.TestCase): + def test(self): + tests = ( + ('//example.com', '/%2Fexample.com'), + ('//', '/%2F'), + ) + for url, expected in tests: + with self.subTest(url=url): + self.assertEqual(escape_leading_slashes(url), expected)