Tighten down ZMI frame source logic to only allow site-local sources [4.x]

CHANGES.rst

@@ -10,7 +10,8 @@ https://zope.readthedocs.io/en/2.13/CHANGES.html
 4.8.10 (unreleased)
 -------------------
 
-- Nothing changed yet.
+- Tighten down the ZMI frame source logic to only allow site-local sources.
+  Problem reported by Miguel Segovia Gil.
 
 
 4.8.9 (2023-09-05)

src/App/dtml/manage.dtml

@@ -19,7 +19,7 @@
 		name="manage_menu"
 		marginwidth="2" marginheight="2"
 		/>
-	<frame src="<dtml-var "REQUEST.get('came_from',None)!=None and REQUEST.get('came_from',None) or '%s/manage_workspace'%(REQUEST.URL1)" html_quote>"
+	<frame src="<dtml-var "getZMIMainFrameTarget(REQUEST)" html_quote>"
 		name="manage_main"
 		marginwidth="2" marginheight="2"
 		/>

src/OFS/Application.py

@@ -17,12 +17,15 @@
 import sys
 from logging import getLogger
 
+from six.moves.urllib.parse import urlparse
+
 import Products
 import transaction
 from AccessControl import ClassSecurityInfo
 from AccessControl.class_init import InitializeClass
 from AccessControl.Permission import ApplicationDefaultPermissions
 from AccessControl.Permissions import view_management_screens
+from AccessControl.tainted import TaintedString
 from Acquisition import aq_base
 from App import FactoryDispatcher
 from App.ApplicationManager import ApplicationManager
@@ -105,6 +108,49 @@ def Redirect(self, destination, URL1):
 
     ZopeRedirect = Redirect
 
+    @security.protected(view_management_screens)
+    def getZMIMainFrameTarget(self, REQUEST):
+        """Utility method to get the right hand side ZMI frame source URL
+
+        For cases where JavaScript is disabled the ZMI uses a simple REQUEST
+        variable ``came_from`` to set the source URL for the right hand side
+        ZMI frame. Since this value can be manipulated by the user it must be
+        sanity-checked first.
+        """
+        parent_url = REQUEST['URL1']
+        default = '{}/manage_workspace'.format(parent_url)
+        came_from = REQUEST.get('came_from', None)
+
+        if not came_from:
+            return default
+
+        # When came_from contains suspicious code, it will not be a string,
+        # but an instance of AccessControl.tainted.TaintedString.
+        # Passing this to urlparse, gives:
+        # AttributeError: 'str' object has no attribute 'decode'
+        # This is good, but let's check explicitly.
+        if isinstance(came_from, TaintedString):
+            return default
+        try:
+            parsed_came_from = urlparse(came_from)
+        except AttributeError:
+            return default
+        parsed_parent_url = urlparse(parent_url)
+
+        # Only allow a passed-in ``came_from`` URL if it is local (just a path)
+        # or if the URL scheme and hostname are the same as our own
+        if (parsed_parent_url.scheme == parsed_came_from.scheme
+                and parsed_parent_url.netloc == parsed_came_from.netloc):
+            return came_from
+        if (not parsed_came_from.scheme and not parsed_came_from.netloc):
+            # This is only a path.  But some paths can be misinterpreted
+            # by browsers.
+            if parsed_came_from.path.startswith("//"):
+                return default
+            return came_from
+
+        return default
+
     def __bobo_traverse__(self, REQUEST, name=None):
         if name is None:
             # Make this more explicit, otherwise getattr(self, name)

src/OFS/tests/testApplication.py

@@ -1,5 +1,6 @@
 import unittest
 
+from AccessControl.tainted import TaintedString
 from Testing.ZopeTestCase import FunctionalTestCase
 
 
@@ -139,6 +140,67 @@ def test_ZopeVersion(self):
         self.assertEqual(app.ZopeVersion(),
                          (pkg_version + ((2 - pkg_version.count('.')) * '.0')))
 
+    def test_getZMIMainFrameTarget(self):
+        app = self._makeOne()
+
+        for URL1 in ('http://nohost', 'https://nohost/some/path'):
+            request = {'URL1': URL1}
+
+            # No came_from at all
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/manage_workspace'.format(URL1))
+
+            # Empty came_from
+            request['came_from'] = ''
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/manage_workspace'.format(URL1))
+            request['came_from'] = None
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/manage_workspace'.format(URL1))
+
+            # Local (path only) came_from
+            request['came_from'] = '/new/path'
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '/new/path')
+
+            # Tainted local path.  came_from can be marked as 'tainted' if it
+            # suspicious contents.  It is not accepted then.
+            request['came_from'] = TaintedString('/new/path')
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/manage_workspace'.format(URL1))
+
+            # came_from URL outside our own server
+            request['came_from'] = 'https://www.zope.dev/index.html'
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/manage_workspace'.format(URL1))
+
+            # came_from with wrong scheme
+            request['came_from'] = URL1.replace('http', 'ftp')
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/manage_workspace'.format(URL1))
+
+            # acceptable came_from
+            request['came_from'] = '{}/added/path'.format(URL1)
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/added/path'.format(URL1))
+
+            # Anything beginning with '<script>' should already be marked as
+            # 'tainted'
+            request['came_from'] = TaintedString(
+                '<script>alert("hi");</script>'
+            )
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/manage_workspace'.format(URL1))
+
+            # double slashes as path should not be accepted.
+            # Try a few forms.
+            request['came_from'] = '//www.example.org'
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/manage_workspace'.format(URL1))
+            request['came_from'] = '////www.example.org'
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             '{}/manage_workspace'.format(URL1))
+
 
 class ApplicationPublishTests(FunctionalTestCase):