Skip to content

Commit

Permalink
Avoid Server Side Request Forgery via lxml parser.
Browse files Browse the repository at this point in the history
Taken over from [`PloneHotfix20210518`](https://plone.org/security/hotfix/20210518/server-side-request-forgery-via-lxml-parser).

Strangely, several tests in `test_transforms` fail, seemingly because we try to load a theme html from the tile system.
This may depend on the `absolutePrefix` setting.
Commenting out the new FailingFileSystemResolver, helps.  But that is one of the parts that brings extra security.

In practice with the hotfix, I have not seen it fail.  Also, I would expect a big cry out if this part was broken.
But these failures are definitely  suspect.
  • Loading branch information
mauritsvanrees committed Jul 6, 2021
1 parent ffe8249 commit dcbc5dc
Show file tree
Hide file tree
Showing 4 changed files with 222 additions and 2 deletions.
3 changes: 3 additions & 0 deletions news/3274.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Avoid Server Side Request Forgery via lxml parser.
Taken over from `PloneHotfix20210518 <https://plone.org/security/hotfix/20210518/server-side-request-forgery-via-lxml-parser>`_.
[maurits]
1 change: 1 addition & 0 deletions src/plone/app/theming/tests/package_theme.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This can be loaded with the python package resolver.
170 changes: 170 additions & 0 deletions src/plone/app/theming/tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,39 @@
# -*- coding: utf-8 -*-
from plone.app.theming.testing import THEMING_FUNCTIONAL_TESTING
from plone.app.theming.testing import THEMING_INTEGRATION_TESTING
from plone.app.theming.utils import applyTheme
from plone.app.theming.utils import extractThemeInfo
from plone.app.theming.utils import getTheme
from plone.app.testing import SITE_OWNER_NAME
from plone.app.testing import SITE_OWNER_PASSWORD
from plone.testing.zope import Browser

import os.path
import tempfile
import transaction
import unittest
import zipfile


# We will try to let the rules file point to a theme on the file system.
# For security reasons, this should not work.
# This is one of the fixes from PloneHotFix20210518.
RULES = """<?xml version="1.0" encoding="UTF-8"?>
<rules
xmlns="http://namespaces.plone.org/diazo"
xmlns:css="http://namespaces.plone.org/diazo/css"
xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
<theme href="{0}" />
</rules>
"""
# The theme will contain a message:
MESSAGE = u"Hello from a temporary directory."
# We have a sample theme file here:
HERE = os.path.dirname(__file__)
PACKAGE_THEME_FILENAME = "package_theme.txt"
PACKAGE_THEME = os.path.join(HERE, PACKAGE_THEME_FILENAME)


class TestIntegration(unittest.TestCase):

layer = THEMING_INTEGRATION_TESTING
Expand Down Expand Up @@ -420,3 +447,146 @@ def test_extractThemeInfo_with_subdirectories(self):
u'/++theme++subdirectories/rules.xml'
)
self.assertEqual(theme.absolutePrefix, '/++theme++subdirectories')


class TestAttackVector(unittest.TestCase):
layer = THEMING_FUNCTIONAL_TESTING

def setUp(self):
self.portal = self.layer["portal"]
rules_fd, self.rules_file = tempfile.mkstemp(
suffix=".xml", prefix="rules", text=True
)
with open(self.rules_file, "w") as myfile:
myfile.write(MESSAGE)

def tearDown(self):
os.remove(self.rules_file)

def get_admin_browser(self):
browser = Browser(self.layer["app"])
browser.handleErrors = False
browser.addHeader(
"Authorization",
"Basic {0}:{1}".format(SITE_OWNER_NAME, SITE_OWNER_PASSWORD),
)
return browser

def get_anon_browser(self):
browser = Browser(self.layer["app"])
browser.handleErrors = False
return browser

def test_failing_file_protocol_resolver(self):
from plone.app.theming.utils import FailingFileProtocolResolver

resolver = FailingFileProtocolResolver()
with self.assertRaises(ValueError):
resolver.resolve("file:///etc/passwd", "public_id", "context")
with self.assertRaises(ValueError):
resolver.resolve(
"file:" + os.path.relpath("/etc/passwd"), "public_id", "context"
)
with self.assertRaises(ValueError):
resolver.resolve("file://" + self.rules_file, "public_id", "context")
with self.assertRaises(ValueError):
resolver.resolve(
"file:" + os.path.relpath(self.rules_file), "public_id", "context"
)

def test_failing_file_system_resolver(self):
from plone.app.theming.utils import FailingFileSystemResolver

resolver = FailingFileSystemResolver()
with self.assertRaises(ValueError):
resolver.resolve("/etc/passwd", "public_id", "context")
with self.assertRaises(ValueError):
resolver.resolve(os.path.relpath("/etc/passwd"), "public_id", "context")
with self.assertRaises(ValueError):
resolver.resolve(self.rules_file, "public_id", "context")
with self.assertRaises(ValueError):
resolver.resolve(os.path.relpath(self.rules_file), "public_id", "context")

def new_theme(self, theme_path):
from plone.app.theming.utils import createThemeFromTemplate
from plone.resource.directory import PersistentResourceDirectory

# Start with an empty theme.
# Pass title and description
theme_name = createThemeFromTemplate("Security", "")
theme = getTheme(theme_name)
directory = PersistentResourceDirectory()
directory.writeFile(
"/".join(["theme", theme_name, "rules.xml"]), RULES.format(theme_path)
)
applyTheme(theme)
transaction.commit()

def test_theme_file_system_absolute(self):
self.new_theme(self.rules_file)
browser = self.get_anon_browser()
browser.open(self.portal.absolute_url())
self.assertNotIn(MESSAGE, browser.contents)

def test_theme_file_system_relative(self):
self.new_theme(os.path.relpath(self.rules_file))
browser = self.get_anon_browser()
browser.open(self.portal.absolute_url())
self.assertNotIn(MESSAGE, browser.contents)

def test_theme_file_protocol_absolute(self):
self.new_theme("file://" + self.rules_file)
browser = self.get_anon_browser()
browser.open(self.portal.absolute_url())
self.assertNotIn(MESSAGE, browser.contents)

def test_theme_file_protocol_relative(self):
# This is actually handled by the InternalResolver.
# Well, in fact it gives an error because it cannot resolve it in the Plone Site:
# AttributeError: 'PersistentResourceDirectory' object has no attribute 'getPhysicalPath'
# This can be seen when previewing the theme in the theme editor.
self.new_theme("file:" + os.path.relpath(self.rules_file))
browser = self.get_anon_browser()
browser.open(self.portal.absolute_url())
self.assertNotIn(MESSAGE, browser.contents)

def test_theme_python_protocol(self):
# Since our example rules file is in a Python package,
# we can use the python resolver to access it.
# I don't think we can avoid this.
self.new_theme(
"python://plone.app.theming/tests/" + PACKAGE_THEME_FILENAME
)
with open(PACKAGE_THEME) as myfile:
contents = myfile.read()
browser = self.get_anon_browser()
browser.open(self.portal.absolute_url())
self.assertIn(contents, browser.contents)

def test_available_themes(self):
"""Test that all available themes render properly.
Our security fixes should not break them.
"""
from plone.app.theming.utils import getAvailableThemes

for theme in getAvailableThemes():
applyTheme(theme)
transaction.commit()
# Can you view the portal anonymously?
browser = self.get_anon_browser()
browser.open(self.portal.absolute_url())
# Can you see the preview as admin?
# This can give errors that are otherwise swallowed by the
# diazo/theming transform, effectively disabling the theme.
if theme.__name__ in ("another-theme", "secondary-theme"):
# Some of the test themes give problems.
# We are only interested in the Sunburst and other official themes.
continue
browser = self.get_admin_browser()
browser.open(
self.portal.absolute_url()
+ theme.absolutePrefix
+ "/@@theming-controlpanel-mapper-getframe?path=/&theme=apply"
+ "&forms=disable&links=replace&title=Preview"
)
50 changes: 48 additions & 2 deletions src/plone/app/theming/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
from zope.interface import implementer

import logging
import os
import pkg_resources
import six

Expand Down Expand Up @@ -67,15 +68,57 @@ def theming_policy(request=None):


class FailingFileProtocolResolver(etree.Resolver):
"""Resolver that fails for security when file:/// urls are tried.
"""Resolver that fails for security when file: urls are tried.
Note: an earlier version only checked for "file://", not "file:",
and did not catch relative paths.
"""
def resolve(self, system_url, public_id, context):
if system_url.startswith('file://') and system_url != 'file:///__diazo__':
if system_url.startswith('file:') and system_url != 'file:///__diazo__':
# The error will be caught by lxml and we only see this in the traceback:
# XIncludeError: could not load <system_url>, and no fallback was found
raise ValueError("File protocol access not allowed: '%s'" % system_url)


class FailingFileSystemResolver(etree.Resolver):
"""Resolver that fails for security when accessing the file system.
Problem 1: none of the current plone.app.theming resolvers
resolve file system paths, and yet they get resolved.
So somewhere in etree there is a fallback.
Problem 2: the InternalResolver of plone.app.theming can resolve paths
internal in the Plone Site. If that happens, then our failing resolver
should not be called. But the order in which resolvers are called,
seems random, so we cannot rely on the InternalResolver being called first.
So what do we do?
Situation:
- The Plone Site has a theme.html in the site root.
- On the file system there is a file theme.html in the root.
Possibilities when resolving /theme.html:
A. The InternalResolver is called first, and resolves it correctly.
B. Our FailingFileSystemResolver is called first,
sees that the file exists, and raises an error.
In this situation, the resolving would randomly work and not work.
This seems unavoidable, but also seems a corner case
which will not happen very often.
In case the file does not exist on the file system,
our resolver should return nothing.
Then the InternalResolver or other resolvers can have a go.
"""
def resolve(self, system_url, public_id, context):
if system_url and os.path.exists(system_url):
# The error will be caught by lxml and we only see this in the traceback:
# XIncludeError: could not load <system_url>, and no fallback was found
raise ValueError("File system access not allowed: '%s'" % system_url)


class NetworkResolver(etree.Resolver):
"""Resolver for network urls
"""
Expand Down Expand Up @@ -633,11 +676,14 @@ def getParser(type, readNetwork):
parser = etree.HTMLParser()
elif type == 'compiler':
parser = etree.XMLParser(resolve_entities=False, remove_pis=True)
# Note: the order in which resolvers are called, seems random.
# They end up in a set.
parser.resolvers.add(InternalResolver())
parser.resolvers.add(PythonResolver())
if readNetwork:
parser.resolvers.add(NetworkResolver())
parser.resolvers.add(FailingFileProtocolResolver())
parser.resolvers.add(FailingFileSystemResolver())
return parser


Expand Down

0 comments on commit dcbc5dc

Please sign in to comment.