From: =?utf-8?q?David_Pr=C3=A9vot?= Date: Tue, 12 Mar 2019 10:13:15 -1000 Subject: Fix security issue in the sandbox Fix sandbox security issue (under some circumstances, calling the __toString() method on an object was possible even if not allowed by the security policy). Origin: backport, https://github.com/twigphp/Twig/commit/eac5422956e1dcca89a3669a03a3ff32f0502077 --- lib/Twig/Node/CheckToString.php | 39 +++++++++++++ lib/Twig/Node/SandboxedPrint.php | 2 + lib/Twig/NodeVisitor/Sandbox.php | 45 +++++++++++++- src/Node/CheckToStringNode.php | 11 ++++ test/Twig/Tests/Extension/SandboxTest.php | 91 +++++++++++++++++++++++------ test/Twig/Tests/Node/SandboxedPrintTest.php | 33 ----------- 6 files changed, 167 insertions(+), 54 deletions(-) create mode 100644 lib/Twig/Node/CheckToString.php create mode 100644 src/Node/CheckToStringNode.php delete mode 100644 test/Twig/Tests/Node/SandboxedPrintTest.php diff --git a/lib/Twig/Node/CheckToString.php b/lib/Twig/Node/CheckToString.php new file mode 100644 index 0000000..a1125af --- /dev/null +++ b/lib/Twig/Node/CheckToString.php @@ -0,0 +1,39 @@ + + */ +class Twig_Node_CheckToString extends Twig_Node +{ + public function __construct(Twig_Node_Expression $expr) + { + parent::__construct(array('expr' => $expr), array(), $expr->getLine(), $expr->getNodeTag()); + } + + public function compile(Twig_Compiler $compiler) + { + $compiler + ->write('$this->env->getExtension(\'sandbox\')->ensureToStringAllowed(') + ->subcompile($this->getNode('expr')) + ->raw(')') + ; + } +} + +class_alias('Twig_Node_CheckToString', 'Twig\Node\CheckToStringNode', false); diff --git a/lib/Twig/Node/SandboxedPrint.php b/lib/Twig/Node/SandboxedPrint.php index 148dd2b..761d622 100644 --- a/lib/Twig/Node/SandboxedPrint.php +++ b/lib/Twig/Node/SandboxedPrint.php @@ -17,6 +17,8 @@ * and if the sandbox is enabled, we need to check that the __toString() * method is allowed if 'article' is an object. * + * Not used anymore, to be deprecated in 2.x and removed in 3.0 + * * @author Fabien Potencier */ class Twig_Node_SandboxedPrint extends Twig_Node_Print diff --git a/lib/Twig/NodeVisitor/Sandbox.php b/lib/Twig/NodeVisitor/Sandbox.php index 7f1b913..08b26c9 100644 --- a/lib/Twig/NodeVisitor/Sandbox.php +++ b/lib/Twig/NodeVisitor/Sandbox.php @@ -21,6 +21,8 @@ class Twig_NodeVisitor_Sandbox extends Twig_BaseNodeVisitor protected $filters; protected $functions; + private $needsToStringWrap = false; + /** * {@inheritdoc} */ @@ -49,9 +51,28 @@ class Twig_NodeVisitor_Sandbox extends Twig_BaseNodeVisitor $this->functions[$node->getAttribute('name')] = $node; } - // wrap print to check __toString() calls if ($node instanceof Twig_Node_Print) { - return new Twig_Node_SandboxedPrint($node->getNode('expr'), $node->getLine(), $node->getNodeTag()); + $this->needsToStringWrap = true; + $this->wrapNode($node, 'expr'); + } + + if ($node instanceof Twig_Node_Set && !$node->getAttribute('capture')) { + $this->needsToStringWrap = true; + } + + // wrap outer nodes that can implicitly call __toString() + if ($this->needsToStringWrap) { + if ($node instanceof Twig_Node_Expression_Binary_Concat) { + $this->wrapNode($node, 'left'); + $this->wrapNode($node, 'right'); + } + if ($node instanceof Twig_Node_Expression_Filter) { + $this->wrapNode($node, 'node'); + $this->wrapArrayNode($node, 'arguments'); + } + if ($node instanceof Twig_Node_Expression_Function) { + $this->wrapArrayNode($node, 'arguments'); + } } } @@ -67,11 +88,31 @@ class Twig_NodeVisitor_Sandbox extends Twig_BaseNodeVisitor $this->inAModule = false; $node->setNode('display_start', new Twig_Node(array(new Twig_Node_CheckSecurity($this->filters, $this->tags, $this->functions), $node->getNode('display_start')))); + } elseif ($this->inAModule) { + if ($node instanceof Twig_Node_Print || $node instanceof Twig_Node_Set) { + $this->needsToStringWrap = false; + } } return $node; } + private function wrapNode(Twig_Node $node, $name) + { + $expr = $node->getNode($name); + if ($expr instanceof Twig_Node_Expression_Name || $expr instanceof Twig_Node_Expression_GetAttr) { + $node->setNode($name, new Twig_Node_CheckToString($expr)); + } + } + + private function wrapArrayNode(Twig_Node $node, $name) + { + $args = $node->getNode($name); + foreach ($args as $name => $_) { + $this->wrapNode($args, $name); + } + } + /** * {@inheritdoc} */ diff --git a/src/Node/CheckToStringNode.php b/src/Node/CheckToStringNode.php new file mode 100644 index 0000000..d8df055 --- /dev/null +++ b/src/Node/CheckToStringNode.php @@ -0,0 +1,11 @@ + '{% if name %}foo{% endif %}', '1_basic4' => '{{ obj.bar }}', '1_basic5' => '{{ obj }}', - '1_basic6' => '{{ arr.obj }}', '1_basic7' => '{{ cycle(["foo","bar"], 1) }}', '1_basic8' => '{{ obj.getfoobar }}{{ obj.getFooBar }}', '1_basic9' => '{{ obj.foobar }}{{ obj.fooBar }}', @@ -79,42 +78,91 @@ class Twig_Tests_Extension_SandboxTest extends PHPUnit_Framework_TestCase $this->fail('Sandbox throws a SecurityError exception if an unallowed property is called in the template'); } catch (Twig_Sandbox_SecurityError $e) { } + } - $twig = $this->getEnvironment(true, array(), self::$templates); + /** + * @dataProvider getSandboxUnallowedToStringTests + */ + public function testSandboxUnallowedToString($template) + { + $twig = $this->getEnvironment(true, array(), array('index' => $template), array(), array('upper'), array('FooObject' => 'getAnotherFooObject'), array(), array('random')); try { - $twig->loadTemplate('1_basic5')->render(self::$params); + $twig->loadTemplate('index')->render(self::$params); $this->fail('Sandbox throws a SecurityError exception if an unallowed method (__toString()) is called in the template'); } catch (Twig_Sandbox_SecurityError $e) { } + } - $twig = $this->getEnvironment(true, array(), self::$templates); - try { - $twig->loadTemplate('1_basic6')->render(self::$params); - $this->fail('Sandbox throws a SecurityError exception if an unallowed method (__toString()) is called in the template'); - } catch (Twig_Sandbox_SecurityError $e) { - } + public function getSandboxUnallowedToStringTests() + { + return array( + 'simple' => array('{{ obj }}'), + 'object_from_array' => array('{{ arr.obj }}'), + 'object_chain' => array('{{ obj.anotherFooObject }}'), + 'filter' => array('{{ obj|upper }}'), + 'filter_from_array' => array('{{ arr.obj|upper }}'), + 'function' => array('{{ random(obj) }}'), + 'function_from_array' => array('{{ random(arr.obj) }}'), + 'function_and_filter' => array('{{ random(obj|upper) }}'), + 'function_and_filter_from_array' => array('{{ random(arr.obj|upper) }}'), + 'object_chain_and_filter' => array('{{ obj.anotherFooObject|upper }}'), + 'object_chain_and_function' => array('{{ random(obj.anotherFooObject) }}'), + 'concat' => array('{{ obj ~ "" }}'), + 'concat_again' => array('{{ "" ~ obj }}'), + ); + } - $twig = $this->getEnvironment(true, array(), self::$templates); - try { - $twig->loadTemplate('1_basic7')->render(self::$params); - $this->fail('Sandbox throws a SecurityError exception if an unallowed function is called in the template'); - } catch (Twig_Sandbox_SecurityError $e) { - } + /** + * @dataProvider getSandboxAllowedToStringTests + */ + public function testSandboxAllowedToString($template, $output) + { + $twig = $this->getEnvironment(true, array(), array('index' => $template), array('set'), array(), array('FooObject' => array('foo', 'getAnotherFooObject'))); + $this->assertEquals($output, $twig->loadTemplate('index')->render(self::$params)); + } - $twig = $this->getEnvironment(true, array(), self::$templates, array(), array(), array('FooObject' => 'foo')); - FooObject::reset(); - $this->assertEquals('foo', $twig->loadTemplate('1_basic1')->render(self::$params), 'Sandbox allow some methods'); - $this->assertEquals(1, FooObject::$called['foo'], 'Sandbox only calls method once'); + public function getSandboxAllowedToStringTests() + { + return array( + 'constant_test' => array('{{ obj is constant("PHP_INT_MAX") }}', ''), + 'set_object' => array('{% set a = obj.anotherFooObject %}{{ a.foo }}', 'foo'), + 'is_defined' => array('{{ obj.anotherFooObject is defined }}', '1'), + 'is_null' => array('{{ obj is null }}', ''), + 'is_sameas' => array('{{ obj is same as(obj) }}', '1'), + 'is_sameas_from_array' => array('{{ arr.obj is same as(arr.obj) }}', '1'), + 'is_sameas_from_another_method' => array('{{ obj.anotherFooObject is same as(obj.anotherFooObject) }}', ''), + ); + } + public function testSandboxAllowMethodToString() + { $twig = $this->getEnvironment(true, array(), self::$templates, array(), array(), array('FooObject' => '__toString')); FooObject::reset(); $this->assertEquals('foo', $twig->loadTemplate('1_basic5')->render(self::$params), 'Sandbox allow some methods'); $this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once'); + } + public function testSandboxAllowMethodToStringDisabled() + { $twig = $this->getEnvironment(false, array(), self::$templates); FooObject::reset(); $this->assertEquals('foo', $twig->loadTemplate('1_basic5')->render(self::$params), 'Sandbox allows __toString when sandbox disabled'); $this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once'); + } + + public function testSandboxUnallowedFunction() + { + $twig = $this->getEnvironment(true, array(), self::$templates); + try { + $twig->loadTemplate('1_basic7')->render(self::$params); + $this->fail('Sandbox throws a SecurityError exception if an unallowed function is called in the template'); + } catch (Twig_Sandbox_SecurityError $e) { + } + + $twig = $this->getEnvironment(true, array(), self::$templates, array(), array(), array('FooObject' => 'foo')); + FooObject::reset(); + $this->assertEquals('foo', $twig->loadTemplate('1_basic1')->render(self::$params), 'Sandbox allow some methods'); + $this->assertEquals(1, FooObject::$called['foo'], 'Sandbox only calls method once'); $twig = $this->getEnvironment(true, array(), self::$templates, array(), array('upper')); $this->assertEquals('FABIEN', $twig->loadTemplate('1_basic2')->render(self::$params), 'Sandbox allow some filters'); @@ -217,4 +265,9 @@ class FooObject return 'foobar'; } + + public function getAnotherFooObject() + { + return new self(); + } } diff --git a/test/Twig/Tests/Node/SandboxedPrintTest.php b/test/Twig/Tests/Node/SandboxedPrintTest.php deleted file mode 100644 index 05e1854..0000000 --- a/test/Twig/Tests/Node/SandboxedPrintTest.php +++ /dev/null @@ -1,33 +0,0 @@ -assertEquals($expr, $node->getNode('expr')); - } - - public function getTests() - { - $tests = array(); - - $tests[] = array(new Twig_Node_SandboxedPrint(new Twig_Node_Expression_Constant('foo', 1), 1), <<env->getExtension('sandbox')->ensureToStringAllowed("foo"); -EOF - ); - - return $tests; - } -}