Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-rpcg-f9q6-2mq6
* - Prevent traversal through authorized Python modules in TAL expressions

* - try a more generic solution that doesn't use special casing

* - add suggestions from Maurits

* - integrate remaining suggestions from Maurits
  • Loading branch information
dataflake committed Jun 8, 2021
1 parent 15e521b commit 1d89791
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 23 deletions.
5 changes: 4 additions & 1 deletion CHANGES.rst
Expand Up @@ -11,11 +11,14 @@ https://github.com/zopefoundation/Zope/blob/4.x/CHANGES.rst
5.2.1 (unreleased)
------------------

- Update to newest compatible versions of dependencies.
- Prevent unauthorized traversal through authorized Python modules in
TAL expressions

- Facelift the Zope logo.
(`#973 <https://github.com/zopefoundation/Zope/issues/973>`_)

- Update to newest compatible versions of dependencies.


5.2 (2021-05-21)
----------------
Expand Down
2 changes: 1 addition & 1 deletion src/OFS/zpt/main.zpt
Expand Up @@ -5,7 +5,7 @@
<main class="container-fluid">
<form id="objectItems" name="objectItems" method="post"
tal:define="has_order_support python:getattr(here.aq_explicit, 'has_order_support', 0);
sm modules/AccessControl/SecurityManagement/getSecurityManager;
sm modules/AccessControl/getSecurityManager;
default_sort python: 'position' if has_order_support else 'id';
skey python:request.get('skey',default_sort);
rkey python:request.get('rkey','asc');
Expand Down
38 changes: 29 additions & 9 deletions src/Products/PageTemplates/Expressions.py
Expand Up @@ -21,6 +21,7 @@

import OFS.interfaces
from AccessControl import safe_builtins
from AccessControl.SecurityManagement import getSecurityManager
from Acquisition import aq_base
from MultiMapping import MultiMapping
from zExceptions import NotFound
Expand Down Expand Up @@ -70,24 +71,43 @@ def boboAwareZopeTraverse(object, path_items, econtext):
necessary (bobo-awareness).
"""
request = getattr(econtext, 'request', None)
validate = getSecurityManager().validate
path_items = list(path_items)
path_items.reverse()

while path_items:
name = path_items.pop()

if name == '_':
warnings.warn('Traversing to the name `_` is deprecated '
'and will be removed in Zope 6.',
DeprecationWarning)
elif name.startswith('_'):
raise NotFound(name)

if OFS.interfaces.ITraversable.providedBy(object):
object = object.restrictedTraverse(name)
else:
object = traversePathElement(object, name, path_items,
request=request)
found = traversePathElement(object, name, path_items,
request=request)

# Special backwards compatibility exception for the name ``_``,
# which was often used for translation message factories.
# Allow and continue traversal.
if name == '_':
warnings.warn('Traversing to the name `_` is deprecated '
'and will be removed in Zope 6.',
DeprecationWarning)
object = found
continue

# All other names starting with ``_`` are disallowed.
# This emulates what restrictedTraverse does.
if name.startswith('_'):
raise NotFound(name)

# traversePathElement doesn't apply any Zope security policy,
# so we validate access explicitly here.
try:
validate(object, object, name, found)
object = found
except Unauthorized:
# Convert Unauthorized to prevent information disclosures
raise NotFound(name)

return object


Expand Down
44 changes: 35 additions & 9 deletions src/Products/PageTemplates/expression.py
Expand Up @@ -10,6 +10,7 @@
from chameleon.tales import NotExpr
from chameleon.tales import StringExpr

from AccessControl.SecurityManagement import getSecurityManager
from AccessControl.ZopeGuards import guarded_apply
from AccessControl.ZopeGuards import guarded_getattr
from AccessControl.ZopeGuards import guarded_getitem
Expand Down Expand Up @@ -57,24 +58,49 @@ class BoboAwareZopeTraverse:
def traverse(cls, base, request, path_items):
"""See ``zope.app.pagetemplate.engine``."""

validate = getSecurityManager().validate
path_items = list(path_items)
path_items.reverse()

while path_items:
name = path_items.pop()

if name == '_':
warnings.warn('Traversing to the name `_` is deprecated '
'and will be removed in Zope 6.',
DeprecationWarning)
elif name.startswith('_'):
raise NotFound(name)

if ITraversable.providedBy(base):
base = getattr(base, cls.traverse_method)(name)
else:
base = traversePathElement(base, name, path_items,
request=request)
found = traversePathElement(base, name, path_items,
request=request)

# If traverse_method is something other than
# ``restrictedTraverse`` then traversal is assumed to be
# unrestricted. This emulates ``unrestrictedTraverse``
if cls.traverse_method != 'restrictedTraverse':
base = found
continue

# Special backwards compatibility exception for the name ``_``,
# which was often used for translation message factories.
# Allow and continue traversal.
if name == '_':
warnings.warn('Traversing to the name `_` is deprecated '
'and will be removed in Zope 6.',
DeprecationWarning)
base = found
continue

# All other names starting with ``_`` are disallowed.
# This emulates what restrictedTraverse does.
if name.startswith('_'):
raise NotFound(name)

# traversePathElement doesn't apply any Zope security policy,
# so we validate access explicitly here.
try:
validate(base, base, name, found)
base = found
except Unauthorized:
# Convert Unauthorized to prevent information disclosures
raise NotFound(name)

return base

Expand Down
5 changes: 3 additions & 2 deletions src/Products/PageTemplates/tests/testExpressions.py
Expand Up @@ -4,6 +4,7 @@
from AccessControl import safe_builtins
from zExceptions import NotFound
from zope.component.testing import PlacelessSetup
from zope.location.interfaces import LocationError


class EngineTestsBase(PlacelessSetup):
Expand Down Expand Up @@ -233,10 +234,10 @@ def test_underscore_traversal(self):
with self.assertRaises(NotFound):
ec.evaluate("context/__class__")

with self.assertRaises(NotFound):
with self.assertRaises((NotFound, LocationError)):
ec.evaluate("nocall: random/_itertools/repeat")

with self.assertRaises(NotFound):
with self.assertRaises((NotFound, LocationError)):
ec.evaluate("random/_itertools/repeat/foobar")


Expand Down
45 changes: 44 additions & 1 deletion src/Products/PageTemplates/tests/testHTMLTests.py
Expand Up @@ -26,8 +26,10 @@
DefaultUnicodeEncodingConflictResolver
from Products.PageTemplates.unicodeconflictresolver import \
PreferredCharsetResolver
from Products.PageTemplates.ZopePageTemplate import ZopePageTemplate
from zExceptions import NotFound
from zope.component import provideUtility
from zope.location.interfaces import LocationError
from zope.traversing.adapters import DefaultTraversable

from .util import useChameleonEngine
Expand All @@ -37,6 +39,10 @@ class AqPageTemplate(Implicit, PageTemplate):
pass


class AqZopePageTemplate(Implicit, ZopePageTemplate):
pass


class Folder(util.Base):
pass

Expand Down Expand Up @@ -74,6 +80,7 @@ def setUp(self):
self.folder = f = Folder()
f.laf = AqPageTemplate()
f.t = AqPageTemplate()
f.z = AqZopePageTemplate('testing')
self.policy = UnitTestSecurityPolicy()
self.oldPolicy = SecurityManager.setSecurityPolicy(self.policy)
noSecurityManager() # Use the new policy.
Expand Down Expand Up @@ -226,9 +233,45 @@ def test_underscore_traversal(self):
t()

t.write('<p tal:define="p nocall: random/_itertools/repeat"/>')
with self.assertRaises(NotFound):
with self.assertRaises((NotFound, LocationError)):
t()

t.write('<p tal:content="random/_itertools/repeat/foobar"/>')
with self.assertRaises((NotFound, LocationError)):
t()

def test_module_traversal(self):
t = self.folder.z

# Need to reset to the standard security policy so AccessControl
# checks are actually performed. The test setup initializes
# a policy that circumvents those checks.
SecurityManager.setSecurityPolicy(self.oldPolicy)
noSecurityManager()

# The getSecurityManager function is explicitly allowed
content = ('<p tal:define="a nocall:%s"'
' tal:content="python: a().getUser().getUserName()"/>')
t.write(content % 'modules/AccessControl/getSecurityManager')
self.assertEqual(t(), '<p>Anonymous User</p>')

# Anything else should be unreachable and raise NotFound:
# Direct access through AccessControl
t.write('<p tal:define="a nocall:modules/AccessControl/users"/>')
with self.assertRaises(NotFound):
t()

# Indirect access through an intermediary variable
content = ('<p tal:define="mod nocall:modules/AccessControl;'
' must_fail nocall:mod/users"/>')
t.write(content)
with self.assertRaises(NotFound):
t()

# Indirect access through an intermediary variable and a dictionary
content = ('<p tal:define="mod nocall:modules/AccessControl;'
' a_dict python: {\'unsafe\': mod};'
' must_fail nocall: a_dict/unsafe/users"/>')
t.write(content)
with self.assertRaises(NotFound):
t()

0 comments on commit 1d89791

Please sign in to comment.