From 7eead067898852ebd3e0f143bc51295928528dfa Mon Sep 17 00:00:00 2001 From: Jens Vagelpohl Date: Tue, 23 Feb 2021 12:23:37 +0100 Subject: [PATCH] - Fix open redirect issue in redirect handling --- CHANGES.rst | 2 ++ .../PluggableAuthService/plugins/CookieAuthHelper.py | 8 ++++++++ .../plugins/tests/test_CookieAuthHelper.py | 10 +++++++--- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 0a3e4a22..d0683afb 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,6 +4,8 @@ Change Log 2.6.0 (unreleased) ------------------ +- Fix open redirect issue in `Cookie Auth Helper` redirect handling + - Add support for Python 3.9. diff --git a/src/Products/PluggableAuthService/plugins/CookieAuthHelper.py b/src/Products/PluggableAuthService/plugins/CookieAuthHelper.py index 6d5f1402..af86b0be 100644 --- a/src/Products/PluggableAuthService/plugins/CookieAuthHelper.py +++ b/src/Products/PluggableAuthService/plugins/CookieAuthHelper.py @@ -28,6 +28,8 @@ import six from six.moves.urllib.parse import quote from six.moves.urllib.parse import unquote +from six.moves.urllib.parse import urlparse +from six.moves.urllib.parse import urlunparse from AccessControl.class_init import InitializeClass from AccessControl.Permissions import view @@ -225,6 +227,12 @@ def unauthorized(self): # in an endless redirect loop. return 0 + # Sanitize the return URL ``came_from`` and only allow local URLs + # to prevent an open exploitable redirect issue + if came_from: + parsed = urlparse(came_from) + came_from = urlunparse(('', '') + parsed[2:]) + if '?' in url: sep = '&' else: diff --git a/src/Products/PluggableAuthService/plugins/tests/test_CookieAuthHelper.py b/src/Products/PluggableAuthService/plugins/tests/test_CookieAuthHelper.py index 20888a24..2fb9e3a8 100644 --- a/src/Products/PluggableAuthService/plugins/tests/test_CookieAuthHelper.py +++ b/src/Products/PluggableAuthService/plugins/tests/test_CookieAuthHelper.py @@ -128,7 +128,8 @@ def test_extractCredentials_with_deleted_cookie(self): def test_challenge(self): rc, root, folder, object = self._makeTree() response = FauxCookieResponse() - testURL = 'http://test' + testPath = '/some/path' + testURL = 'http://test' + testPath request = FauxRequest(RESPONSE=response, URL=testURL, ACTUAL_URL=testURL) root.REQUEST = request @@ -138,7 +139,9 @@ def test_challenge(self): helper.challenge(request, response) self.assertEqual(response.status, 302) self.assertEqual(len(response.headers), 3) - self.assertTrue(response.headers['Location'].endswith(quote(testURL))) + loc = response.headers['Location'] + self.assertTrue(loc.endswith(quote(testPath))) + self.assertNotIn(testURL, loc) self.assertEqual(response.headers['Cache-Control'], 'no-cache') self.assertEqual(response.headers['Expires'], 'Sat, 01 Jan 2000 00:00:00 GMT') @@ -159,8 +162,9 @@ def test_challenge_with_vhm(self): self.assertEqual(response.status, 302) self.assertEqual(len(response.headers), 3) loc = response.headers['Location'] - self.assertTrue(loc.endswith(quote(actualURL))) + self.assertTrue(loc.endswith(quote('/xxx'))) self.assertFalse(loc.endswith(quote(vhm))) + self.assertNotIn(actualURL, loc) self.assertEqual(response.headers['Cache-Control'], 'no-cache') self.assertEqual(response.headers['Expires'], 'Sat, 01 Jan 2000 00:00:00 GMT')