Merge pull request #87 from zopefoundation/dataflake/security_cleanups

Security cleanups

CHANGES.rst

@@ -4,6 +4,10 @@ Change Log
 2.6.0 (unreleased)
 ------------------
 
+- Fix missing access control on ZODB Role Manager ``enumerateRoles``
+
+- Fix open redirect issue in `Cookie Auth Helper` redirect handling
+
 - Add support for Python 3.9.
 
 

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:

src/Products/PluggableAuthService/plugins/ZODBRoleManager.py

@@ -112,6 +112,7 @@ def getRolesForPrincipal(self, principal, request=None):
     #
     #   IRoleEnumerationPlugin implementation
     #
+    @security.private
     def enumerateRoles(self, id=None, exact_match=False, sort_by=None,
                        max_results=None, **kw):
         """ See IRoleEnumerationPlugin.

src/Products/PluggableAuthService/plugins/tests/test_CookieAuthHelper.py

@@ -20,7 +20,6 @@
 import unittest
 
 import six
-from six.moves.urllib.parse import quote
 
 from ...tests.conformance import IChallengePlugin_conformance
 from ...tests.conformance import ICredentialsResetPlugin_conformance
@@ -128,7 +127,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?arg1=val1&arg2=val2'
+        testURL = 'http://test' + testPath
         request = FauxRequest(RESPONSE=response, URL=testURL,
                               ACTUAL_URL=testURL)
         root.REQUEST = request
@@ -138,7 +138,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)))
+        self.assertEqual(
+            response.headers['Location'],
+            '/login_form?came_from=/some/path%3Farg1%3Dval1%26arg2%3Dval2')
         self.assertEqual(response.headers['Cache-Control'], 'no-cache')
         self.assertEqual(response.headers['Expires'],
                          'Sat, 01 Jan 2000 00:00:00 GMT')
@@ -147,7 +149,7 @@ def test_challenge_with_vhm(self):
         rc, root, folder, object = self._makeTree()
         response = FauxCookieResponse()
         vhm = 'http://localhost/VirtualHostBase/http/test/VirtualHostRoot/xxx'
-        actualURL = 'http://test/xxx'
+        actualURL = 'http://test/xxx?arg1=val1&arg2=val2'
 
         request = FauxRequest(RESPONSE=response, URL=vhm,
                               ACTUAL_URL=actualURL)
@@ -158,9 +160,9 @@ def test_challenge_with_vhm(self):
         helper.challenge(request, response)
         self.assertEqual(response.status, 302)
         self.assertEqual(len(response.headers), 3)
-        loc = response.headers['Location']
-        self.assertTrue(loc.endswith(quote(actualURL)))
-        self.assertFalse(loc.endswith(quote(vhm)))
+        self.assertEqual(
+            response.headers['Location'],
+            '/login_form?came_from=/xxx%3Farg1%3Dval1%26arg2%3Dval2')
         self.assertEqual(response.headers['Cache-Control'], 'no-cache')
         self.assertEqual(response.headers['Expires'],
                          'Sat, 01 Jan 2000 00:00:00 GMT')