From 3804fc615b07739fa82c6f5ae8fb5b4bc2ba29bb Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 18 Jan 2017 01:05:30 +0100 Subject: [PATCH] Added security checks for str.format. Part of PloneHotfix20170117. --- CHANGES.rst | 5 +- Products/CMFPlone/__init__.py | 13 ++ Products/CMFPlone/resources/browser/mixins.py | 6 +- .../plone_templates/standard_error_message.py | 4 +- .../tests/normal_zope3_page_template.pt | 2 + .../CMFPlone/tests/test_safe_formatter.py | 216 ++++++++++++++++++ .../tests/using_format_zope3_page_template.pt | 2 + Products/CMFPlone/utils.py | 74 ++++++ 8 files changed, 318 insertions(+), 4 deletions(-) create mode 100644 Products/CMFPlone/tests/normal_zope3_page_template.pt create mode 100644 Products/CMFPlone/tests/test_safe_formatter.py create mode 100644 Products/CMFPlone/tests/using_format_zope3_page_template.pt diff --git a/CHANGES.rst b/CHANGES.rst index 6aaca4741c..235575cd6f 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -30,7 +30,10 @@ New features: Bug fixes: -- Load some patches earlier, instead of in our initialize method. +- Added security checks for ``str.format``. Part of PloneHotfix20170117. [maurits] + +- Load some patches earlier, instead of in our initialize method. [maurits] + - Fixed workflow tests for new ``comment_one_state_workflow``. [maurits] - Fixed sometimes failing search order tests. [maurits] diff --git a/Products/CMFPlone/__init__.py b/Products/CMFPlone/__init__.py index 9a10423115..df2fd2c122 100644 --- a/Products/CMFPlone/__init__.py +++ b/Products/CMFPlone/__init__.py @@ -29,6 +29,7 @@ def initialize(context): from AccessControl import ModuleSecurityInfo from AccessControl import allow_class from AccessControl import allow_module + from AccessControl import allow_type # protect OFS.ObjectManager ModuleSecurityInfo('OFS.ObjectManager').setDefaultAccess(0) @@ -114,6 +115,18 @@ def initialize(context): # Make cgi.escape available TTW ModuleSecurityInfo('cgi').declarePublic('escape') + # We want to allow all methods on string type except 'format'. + # That one needs special handling to avoid access to attributes. + from Products.CMFPlone.utils import safe_format + rules = dict([(m, True) for m in dir(str) if not m.startswith('_')]) + rules['format'] = safe_format + allow_type(str, rules) + + # Same for unicode instead of str. + rules = dict([(m, True) for m in dir(unicode) if not m.startswith('_')]) + rules['format'] = safe_format + allow_type(unicode, rules) + # Apply monkey patches from Products.CMFPlone import patches # noqa diff --git a/Products/CMFPlone/resources/browser/mixins.py b/Products/CMFPlone/resources/browser/mixins.py index 40fb148324..47f1753186 100644 --- a/Products/CMFPlone/resources/browser/mixins.py +++ b/Products/CMFPlone/resources/browser/mixins.py @@ -1,4 +1,5 @@ from Products.CMFPlone.interfaces import IResourceRegistry +from Products.CMFPlone.utils import safe_format from Products.Five.browser import BrowserView from plone.registry.interfaces import IRegistry from urlparse import urlparse @@ -67,9 +68,10 @@ def __call__(self): less_vars_params[name] = value for name, value in registry.items(): - t = value.format(**less_vars_params) + t = SafeFormatter(value).safe_format(**less_vars_params) result += "'%s': \"%s\",\n" % (name, t) + # Adding all plone.resource entries css values as less vars for name, value in self.resource_registry().items(): for css in value.css: @@ -115,7 +117,7 @@ def __call__(self): less_vars_params[name] = value for name, value in registry.items(): - t = value.format(**less_vars_params) + t = SafeFormatter(value).safe_format(**less_vars_params) result2 += "'@%s': \"%s\",\n" % (name, t) self.request.response.setHeader("Content-Type", diff --git a/Products/CMFPlone/skins/plone_templates/standard_error_message.py b/Products/CMFPlone/skins/plone_templates/standard_error_message.py index 541dab9e79..fe6e3e178c 100644 --- a/Products/CMFPlone/skins/plone_templates/standard_error_message.py +++ b/Products/CMFPlone/skins/plone_templates/standard_error_message.py @@ -29,7 +29,9 @@ if "text/html" not in context.REQUEST.getHeader('Accept', ''): context.REQUEST.RESPONSE.setHeader("Content-Type", "application/json") - return '{{"error_type": "{0:s}"}}'.format(error_type) + # Note: using %s instead of .format to avoid possibly expensive guarded + # attribute check. + return '{{"error_type": "%s"}}' % error_type if error_log_url: error_log_id = error_log_url.split('?id=')[1] diff --git a/Products/CMFPlone/tests/normal_zope3_page_template.pt b/Products/CMFPlone/tests/normal_zope3_page_template.pt new file mode 100644 index 0000000000..f3822734d1 --- /dev/null +++ b/Products/CMFPlone/tests/normal_zope3_page_template.pt @@ -0,0 +1,2 @@ +

+

diff --git a/Products/CMFPlone/tests/test_safe_formatter.py b/Products/CMFPlone/tests/test_safe_formatter.py new file mode 100644 index 0000000000..07753718e1 --- /dev/null +++ b/Products/CMFPlone/tests/test_safe_formatter.py @@ -0,0 +1,216 @@ +from plone.app.testing import login +from plone.app.testing import logout +from plone.app.testing import setRoles +from plone.app.testing import TEST_USER_ID +from plone.app.testing import TEST_USER_NAME +from Products.CMFPlone.tests.PloneTestCase import PloneTestCase +from zExceptions import Unauthorized + + +BAD_STR = """ +

+""" +BAD_UNICODE = """ +

+""" +GOOD_STR = '

' +GOOD_UNICODE = '

' +AQ_TEST = """ +

+""" + + +def noop(context=None): + return lambda: context + + +def hack_pt(pt, context=None): + # hacks to avoid getting error in pt_render. + pt.getPhysicalRoot = noop() + pt._getContext = noop(context) + pt._getContainer = noop(context) + pt.context = context + + +class TestSafeFormatter(PloneTestCase): + """The the safe formatter. + + This is from PloneHotfix20170117. + """ + + def test_cook_zope2_page_templates_bad_str(self): + from Products.PageTemplates.ZopePageTemplate import ZopePageTemplate + pt = ZopePageTemplate('mytemplate', BAD_STR) + hack_pt(pt) + self.assertRaises(Unauthorized, pt.pt_render) + + def test_cook_zope2_page_templates_bad_unicode(self): + from Products.PageTemplates.ZopePageTemplate import ZopePageTemplate + pt = ZopePageTemplate('mytemplate', BAD_UNICODE) + hack_pt(pt) + self.assertRaises(Unauthorized, pt.pt_render) + + def test_cook_zope2_page_templates_good_str(self): + from Products.PageTemplates.ZopePageTemplate import ZopePageTemplate + pt = ZopePageTemplate('mytemplate', GOOD_STR) + hack_pt(pt) + self.assertEqual(pt.pt_render().strip(), '

none

') + + def test_cook_zope2_page_templates_good_unicode(self): + from Products.PageTemplates.ZopePageTemplate import ZopePageTemplate + pt = ZopePageTemplate('mytemplate', unicode(GOOD_UNICODE)) + hack_pt(pt) + self.assertEqual(pt.pt_render().strip(), '

none

') + + def test_cook_zope2_page_templates_aq_parent(self): + # Accessing aq_parent should be allowed normally. + from Products.PageTemplates.ZopePageTemplate import ZopePageTemplate + pt = ZopePageTemplate('mytemplate', AQ_TEST) + hack_pt(pt, context=self.portal) + self.assertEqual( + pt.pt_render().strip(), + u'

parent of <PloneSite at plone> is ' + u'<Application at >

') + + def test_access_to_private_content_not_allowed_via_rich_text(self): + try: + # This is only available for tests if we have plone.app.dexterity, + # which in tests is by default only the case for Plone 5. + from plone.app.textfield.value import RichTextValue + except ImportError: + return + from Products.PageTemplates.ZopePageTemplate import ZopePageTemplate + setRoles(self.portal, TEST_USER_ID, ['Manager']) + login(self.portal, TEST_USER_NAME) + wf_tool = self.portal.portal_workflow + wf_tool.setChainForPortalTypes( + ['Document'], 'simple_publication_workflow') + self.portal.invokeFactory('Document', 'foobar') + foobar = self.portal.foobar + foobar.text = RichTextValue(u'Secret.', 'text/plain', 'text/html') + self.assertEqual( + self.portal.portal_workflow.getInfoFor(foobar, 'review_state'), + 'private') + logout() + pt = ZopePageTemplate('mytemplate', ''' +

+''') # noqa + hack_pt(pt, context=self.portal) + self.assertRaises(Unauthorized, pt.pt_render) + + def test_access_to_private_content_not_allowed_via_any_attribute(self): + # This is a more general version of the rich text one. + from Products.PageTemplates.ZopePageTemplate import ZopePageTemplate + setRoles(self.portal, TEST_USER_ID, ['Manager']) + login(self.portal, TEST_USER_NAME) + wf_tool = self.portal.portal_workflow + wf_tool.setChainForPortalTypes( + ['Document'], 'simple_publication_workflow') + self.portal.invokeFactory('Document', 'foobar') + foobar = self.portal.foobar + self.assertEqual( + self.portal.portal_workflow.getInfoFor(foobar, 'review_state'), + 'private') + logout() + # If access to context.foobar.Title was allowed, it would still only + # say 'bound method ATDocument.Title', without giving the actual title, + # but there may be other attributes that give worse info. + pt = ZopePageTemplate('mytemplate', ''' +

+''') + hack_pt(pt, context=self.portal) + self.assertRaises(Unauthorized, pt.pt_render) + + # Zope 3 templates are always file system templates. So we actually have + # no problems allowing str.format there. + + def test_cook_zope3_page_templates_normal(self): + from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile + # Note: on Plone 3.3 this is actually a ZopeTwoPageTemplateFile. + pt = ViewPageTemplateFile('normal_zope3_page_template.pt') + hack_pt(pt) + # Need to pass a namespace. + namespace = {'context': self.portal} + self.assertEqual( + pt.pt_render(namespace).strip(), + u'

<plonesite at plone>

\n' + u'

<PLONESITE AT PLONE>

') + + def test_cook_zope3_page_templates_using_format(self): + from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile + # Note: on Plone 3.3 this is actually a ZopeTwoPageTemplateFile. + pt = ViewPageTemplateFile('using_format_zope3_page_template.pt') + hack_pt(pt) + # Need to pass a namespace. + namespace = {'context': self.portal} + self.assertEqual( + pt.pt_render(namespace).strip(), + u"

class of <plonesite at plone> is " + u"<class 'products.cmfplone.portal.plonesite'>

\n" + u"

CLASS OF <PLONESITE AT PLONE> IS " + u"<CLASS 'PRODUCTS.CMFPLONE.PORTAL.PLONESITE'>

") + + def test_standard_error_message(self): + # In Plone 5.0 standard_error_message.py has: + # if "text/html" not in context.REQUEST.getHeader('Accept', ''): + # return '{{"error_type": "{0:s}"}}'.format(error_type) + # + # So if there is an error and the request does not accept html, then + # str.format is used. We don't want this to fail with an Unauthorized. + + response = self.publish( + '/plone/standard_error_message', + env={'HTTP_ACCEPT': 'application/json'}) + + # This should *not* return a 302 Unauthorized. We expect a 404. Or + # really a 200, because we explicitly call the standard_error_message + # page and this is correctly rendered. + self.assertTrue(response.status in (200, 404)) + # We expect a json string back. + self.assertTrue(response.body, '{"error_type": "None"}') + + def test_resource_registry_vector(self): + for vector in ('less-variables.js', 'less-modify.js'): + src = ''' +class ctx: + def format(self, *args, **kwargs): + self.foo=context + return "foo" + +context.portal_registry['plone.lessvariables']['foo'] = ctx() +context.portal_registry['plone.lessvariables']['bar'] = "{foo.foo.__class__}" +js = context.restrictedTraverse("%s") +return js() +''' % vector + from Products.PythonScripts.PythonScript import PythonScript + script = PythonScript('evil') + script._filepath = 'evil' + script.write(src) + self.portal.evil = script + output = self.publish('/plone/evil') + self.assertFalse( + 'Products.CMFPlone.Portal.PloneSite' in output.body) + + def test_positional_argument_regression(self): + """ + to test http://bugs.python.org/issue13598 issue + """ + from Products.CMFPlone.utils import SafeFormatter + try: + self.assertEquals( + SafeFormatter('{} {}').safe_format('foo', 'bar'), + 'foo bar' + ) + except ValueError: + # On Python 2.6 you get: + # ValueError: zero length field name in format + pass + + self.assertEquals( + SafeFormatter('{0} {1}').safe_format('foo', 'bar'), + 'foo bar' + ) + self.assertEquals( + SafeFormatter('{1} {0}').safe_format('foo', 'bar'), + 'bar foo' + ) diff --git a/Products/CMFPlone/tests/using_format_zope3_page_template.pt b/Products/CMFPlone/tests/using_format_zope3_page_template.pt new file mode 100644 index 0000000000..1e6508c251 --- /dev/null +++ b/Products/CMFPlone/tests/using_format_zope3_page_template.pt @@ -0,0 +1,2 @@ +

+

diff --git a/Products/CMFPlone/utils.py b/Products/CMFPlone/utils.py index 29b3fc7af3..abcd1a6804 100644 --- a/Products/CMFPlone/utils.py +++ b/Products/CMFPlone/utils.py @@ -3,6 +3,7 @@ from AccessControl import getSecurityManager from AccessControl import ModuleSecurityInfo from AccessControl import Unauthorized +from AccessControl.ZopeGuards import guarded_getattr from Acquisition import aq_base from Acquisition import aq_get from Acquisition import aq_inner @@ -11,6 +12,7 @@ from App.Dialogs import MessageDialog from App.ImageFile import ImageFile from cgi import escape +from collections import Mapping from DateTime import DateTime from DateTime.interfaces import DateTimeError from log import log @@ -46,6 +48,7 @@ import OFS import pkg_resources import re +import string import sys import transaction import zope.interface @@ -720,3 +723,74 @@ def get_top_site_from_url(context, request): # Also, TestRequest doesn't have physicalPathFromURL pass return site + + +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): + + def __init__(self, value): + self.value = value + super(SafeFormatter, self).__init__() + + def get_field(self, field_name, args, kwargs): + """ + Here we're overridding so we can use guarded_getattr instead of + regular getattr + """ + first, rest = field_name._formatter_field_name_split() + + 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 = obj[i] + + return obj, first + + def safe_format(self, *args, **kwargs): + 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