Merge pull request #46 from zopefoundation/str-format-master

Backport str.format fix from 2.13 to master

CHANGES.rst

@@ -6,6 +6,10 @@ For changes before version 3.0, see ``HISTORY.rst``.
 4.0a8 (unreleased)
 ------------------
 
+- Security fix: In ``str.format``, check the security for attributes that are
+  accessed. (Ported from 2.13).
+
+- Port ``override_container`` context manager here from 2.13.
 
 
 4.0a7 (2017-05-17)

src/AccessControl/SimpleObjectPolicies.py

@@ -43,6 +43,7 @@
 or in ZopeSecurityPolicy. :(
 '''
 
+from contextlib import contextmanager
 
 from BTrees.IIBTree import IIBTree
 from BTrees.IIBTree import IIBucket
@@ -61,6 +62,7 @@
 from BTrees.OOBTree import OOSet
 
 _noroles = []  # this is imported in various places
+_marker = object()
 
 # ContainerAssertions are used by cAccessControl to check access to
 # attributes of container types, like dict, list, or string.
@@ -126,3 +128,17 @@ def allow_type(Type, allowed=1):
     if has_values:
         assert key_type is type(tree.values())
         assert key_type is type(tree.items())
+
+
+@contextmanager
+def override_containers(type_, assertions):
+    """Temporarily override the container assertions."""
+    orig_container = Containers(type_, _marker)
+    ContainerAssertions[type_] = assertions
+    try:
+        yield
+    finally:
+        if orig_container is _marker:
+            del ContainerAssertions[type_]
+        else:
+            ContainerAssertions[type_] = orig_container

src/AccessControl/__init__.py

@@ -28,7 +28,9 @@
 from AccessControl.unauthorized import Unauthorized
 from AccessControl.ZopeGuards import full_write_guard
 from AccessControl.ZopeGuards import safe_builtins
+from AccessControl.safe_formatter import safe_format
 
+import six
 
 ModuleSecurityInfo('AccessControl').declarePublic('getSecurityManager')
 
@@ -38,3 +40,17 @@
     ModuleSecurityInfo(name).setDefaultAccess('allow')
 
 ModuleSecurityInfo('DateTime').declarePublic('DateTime')
+
+# We want to allow all methods on string type except "format".
+# That one needs special handling to avoid access to attributes.
+rules = dict([(m, True) for m in dir(str) if not m.startswith('_')])
+rules['format'] = safe_format
+allow_type(str, rules)
+
+if six.PY2:
+    # Same for unicode instead on Python 2:
+    rules = dict([(m, True) for m in dir(unicode) if not m.startswith('_')])
+    rules['format'] = safe_format
+    allow_type(unicode, rules)
+
+del six

src/AccessControl/safe_formatter.py

@@ -0,0 +1,85 @@
+from AccessControl.ZopeGuards import guarded_getattr, guarded_getitem
+from collections import Mapping
+
+import string
+import six
+
+try:
+    # Python 3
+    import _string
+except ImportError:
+    pass
+
+
+def formatter_field_name_split(field_name):
+    if six.PY3:
+        return _string.formatter_field_name_split(field_name)
+    else:
+        return field_name._formatter_field_name_split()
+
+
+class _MagicFormatMapping(Mapping):
+    """Pulled from Jinja2.
+
+    This class implements a dummy wrapper to fix a bug in the Python
+    standard library for string formatting.
+
+    See http://bugs.python.org/issue13598 for information about why
+    this is necessary.
+    """
+
+    def __init__(self, args, kwargs):
+        self._args = args
+        self._kwargs = kwargs
+        self._last_index = 0
+
+    def __getitem__(self, key):
+        if key == '':
+            idx = self._last_index
+            self._last_index += 1
+            try:
+                return self._args[idx]
+            except LookupError:
+                pass
+            key = str(idx)
+        return self._kwargs[key]
+
+    def __iter__(self):
+        return iter(self._kwargs)
+
+    def __len__(self):
+        return len(self._kwargs)
+
+
+class SafeFormatter(string.Formatter):
+    """Formatter using guarded access."""
+
+    def __init__(self, value):
+        self.value = value
+        super(SafeFormatter, self).__init__()
+
+    def get_field(self, field_name, args, kwargs):
+        """Get the field value using guarded methods."""
+        first, rest = formatter_field_name_split(field_name)
+
+        obj = self.get_value(first, args, kwargs)
+
+        # loop through the rest of the field_name, doing
+        #  getattr or getitem as needed
+        for is_attr, i in rest:
+            if is_attr:
+                obj = guarded_getattr(obj, i)
+            else:
+                obj = guarded_getitem(obj, i)
+
+        return obj, first
+
+    def safe_format(self, *args, **kwargs):
+        """Safe variant of `format` method."""
+        kwargs = _MagicFormatMapping(args, kwargs)
+        return self.vformat(self.value, args, kwargs)
+
+
+def safe_format(inst, method):
+    """Use our SafeFormatter that uses guarded_getattr for attribute access."""
+    return SafeFormatter(inst).safe_format

src/AccessControl/tests/testZopeSecurityPolicy.py

@@ -347,6 +347,26 @@ def testUnicodeRolesForPermission(self):
         v = self.policy.checkPermission(u'View', r_item, o_context)
         self.assert_(v, '_View_Permission should grant access to theowner')
 
+    def testContainersContextManager(self):
+        from AccessControl.SimpleObjectPolicies import override_containers
+        from AccessControl.SimpleObjectPolicies import ContainerAssertions
+        from AccessControl.SimpleObjectPolicies import Containers
+        from types import TracebackType
+        # Surely we have no assertions for this type:
+        self.assertNotIn(TracebackType, ContainerAssertions)
+        with override_containers(TracebackType, 1):
+            self.assertIn(TracebackType, ContainerAssertions)
+            self.assertEqual(Containers(TracebackType), 1)
+            # Override it again.
+            with override_containers(TracebackType, {}):
+                self.assertEqual(Containers(TracebackType), {})
+            # We are outside the nested override, so the first override should
+            # have been restored.
+            self.assertEqual(Containers(TracebackType), 1)
+        # We are outside all overrides, so the type should no longer be in the
+        # assertions.
+        self.assertNotIn(TracebackType, ContainerAssertions)
+
     def testAqNames(self):
         policy = self.policy
         names = {
@@ -356,12 +376,17 @@ def testAqNames(self):
             'aq_explicit': 1,
             'aq_inner': 1,
         }
-        for name, allowed in names.items():
-            if not allowed:
-                self.assertRaises(Unauthorized, policy.validate,
-                                  '', '', name, '', None)
-            else:
-                policy.validate('', '', name, '', None)
+        from AccessControl.SimpleObjectPolicies import override_containers
+        # By default we allow all access to str, but this may have been
+        # overridden to disallow some access of str.format.  So we temporarily
+        # restore the default of allowing all access.
+        with override_containers(str, 1):
+            for name, allowed in names.items():
+                if not allowed:
+                    self.assertRaises(Unauthorized, policy.validate,
+                                      '', '', name, '', None)
+                else:
+                    policy.validate('', '', name, '', None)
 
     def testProxyRoleScope(self):
         self.a.subobject = ImplictAcqObject()
@@ -394,7 +419,12 @@ def testProxyRoleScope(self):
 
     def testUnicodeName(self):
         policy = self.policy
-        assert policy.validate('', '', u'foo', '', None)
+        from AccessControl.SimpleObjectPolicies import override_containers
+        # By default we allow all access to str, but this may have been
+        # overridden to disallow some access of str.format.  So we temporarily
+        # restore the default of allowing all access.
+        with override_containers(str, 1):
+            assert policy.validate('', '', u'foo', '', None)
 
     if 0:
         # This test purposely generates a log entry.

src/AccessControl/tests/test_safe_formatter.py

@@ -0,0 +1,42 @@
+from zExceptions import Unauthorized
+import unittest
+
+
+class FormatterTest(unittest.TestCase):
+    """Test SafeFormatter and SafeStr.
+
+    There are some integration tests in Zope2 itself.
+    """
+
+    def test_positional_argument_regression(self):
+        """Testing fix of http://bugs.python.org/issue13598 issue."""
+        from AccessControl.safe_formatter import SafeFormatter
+        self.assertEqual(
+            SafeFormatter('{} {}').safe_format('foo', 'bar'),
+            'foo bar'
+        )
+
+        self.assertEqual(
+            SafeFormatter('{0} {1}').safe_format('foo', 'bar'),
+            'foo bar'
+        )
+        self.assertEqual(
+            SafeFormatter('{1} {0}').safe_format('foo', 'bar'),
+            'bar foo'
+        )
+
+    def test_prevents_bad_string_formatting(self):
+        from AccessControl.safe_formatter import safe_format
+        with self.assertRaises(Unauthorized) as err:
+            safe_format('{0.__class__}', None)(1)
+        self.assertEqual(
+            "You are not allowed to access '__class__' in this context",
+            str(err.exception))
+
+    def test_prevents_bad_unicode_formatting(self):
+        from AccessControl.safe_formatter import safe_format
+        with self.assertRaises(Unauthorized) as err:
+            safe_format(u'{0.__class__}', None)(1)
+        self.assertEqual(
+            "You are not allowed to access '__class__' in this context",
+            str(err.exception))