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 | 95 ++++++++++++++++++++--------- test/Twig/Tests/Node/SandboxedPrintTest.php | 33 ---------- 6 files changed, 160 insertions(+), 65 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..07a7837 --- /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(['expr' => $expr], [], $expr->getTemplateLine(), $expr->getNodeTag()); + } + + public function compile(Twig_Compiler $compiler) + { + $compiler + ->write('$this->extensions[\'Twig_Extension_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 eb45cb8..aee7d2f 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 4d41ff6..cdc7ff2 100644 --- a/lib/Twig/NodeVisitor/Sandbox.php +++ b/lib/Twig/NodeVisitor/Sandbox.php @@ -21,6 +21,8 @@ final class Twig_NodeVisitor_Sandbox extends Twig_BaseNodeVisitor private $filters; private $functions; + private $needsToStringWrap = false; + protected function doEnterNode(Twig_Node $node, Twig_Environment $env) { if ($node instanceof Twig_Node_Module) { @@ -51,9 +53,28 @@ final class Twig_NodeVisitor_Sandbox extends Twig_BaseNodeVisitor $this->functions['range'] = $node; } - // wrap print to check __toString() calls if ($node instanceof Twig_Node_Print) { - return new Twig_Node_SandboxedPrint($node->getNode('expr'), $node->getTemplateLine(), $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'); + } } } @@ -66,11 +87,31 @@ final class Twig_NodeVisitor_Sandbox extends Twig_BaseNodeVisitor $this->inAModule = false; $node->setNode('display_start', new Twig_Node([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); + } + } + public function getPriority() { return 0; 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 }}', @@ -106,11 +105,14 @@ class Twig_Tests_Extension_SandboxTest extends \PHPUnit\Framework\TestCase } } - public function testSandboxUnallowedToString() + /** + * @dataProvider getSandboxUnallowedToStringTests + */ + public function testSandboxUnallowedToString($template) { - $twig = $this->getEnvironment(true, [], self::$templates); + $twig = $this->getEnvironment(true, [], ['index' => $template], [], ['upper'], ['FooObject' => 'getAnotherFooObject'], [], ['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) { $this->assertInstanceOf('Twig_Sandbox_SecurityNotAllowedMethodError', $e, 'Exception should be an instance of Twig_Sandbox_SecurityNotAllowedMethodError'); @@ -119,17 +121,61 @@ class Twig_Tests_Extension_SandboxTest extends \PHPUnit\Framework\TestCase } } - public function testSandboxUnallowedToStringArray() + public function getSandboxUnallowedToStringTests() { - $twig = $this->getEnvironment(true, [], 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) { - $this->assertInstanceOf('Twig_Sandbox_SecurityNotAllowedMethodError', $e, 'Exception should be an instance of Twig_Sandbox_SecurityNotAllowedMethodError'); - $this->assertEquals('FooObject', $e->getClassName(), 'Exception should be raised on the "FooObject" class'); - $this->assertEquals('__tostring', $e->getMethodName(), 'Exception should be raised on the "__toString" method'); - } + return [ + 'simple' => ['{{ obj }}'], + 'object_from_array' => ['{{ arr.obj }}'], + 'object_chain' => ['{{ obj.anotherFooObject }}'], + 'filter' => ['{{ obj|upper }}'], + 'filter_from_array' => ['{{ arr.obj|upper }}'], + 'function' => ['{{ random(obj) }}'], + 'function_from_array' => ['{{ random(arr.obj) }}'], + 'function_and_filter' => ['{{ random(obj|upper) }}'], + 'function_and_filter_from_array' => ['{{ random(arr.obj|upper) }}'], + 'object_chain_and_filter' => ['{{ obj.anotherFooObject|upper }}'], + 'object_chain_and_function' => ['{{ random(obj.anotherFooObject) }}'], + 'concat' => ['{{ obj ~ "" }}'], + 'concat_again' => ['{{ "" ~ obj }}'], + ]; + } + + /** + * @dataProvider getSandboxAllowedToStringTests + */ + public function testSandboxAllowedToString($template, $output) + { + $twig = $this->getEnvironment(true, [], ['index' => $template], ['set'], [], ['FooObject' => ['foo', 'getAnotherFooObject']]); + $this->assertEquals($output, $twig->load('index')->render(self::$params)); + } + + public function getSandboxAllowedToStringTests() + { + return [ + 'constant_test' => ['{{ obj is constant("PHP_INT_MAX") }}', ''], + 'set_object' => ['{% set a = obj.anotherFooObject %}{{ a.foo }}', 'foo'], + 'is_defined' => ['{{ obj.anotherFooObject is defined }}', '1'], + 'is_null' => ['{{ obj is null }}', ''], + 'is_sameas' => ['{{ obj is same as(obj) }}', '1'], + 'is_sameas_from_array' => ['{{ arr.obj is same as(arr.obj) }}', '1'], + 'is_sameas_from_another_method' => ['{{ obj.anotherFooObject is same as(obj.anotherFooObject) }}', ''], + ]; + } + + public function testSandboxAllowMethodToString() + { + $twig = $this->getEnvironment(true, [], self::$templates, [], [], ['FooObject' => '__toString']); + FooObject::reset(); + $this->assertEquals('foo', $twig->load('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, [], self::$templates); + FooObject::reset(); + $this->assertEquals('foo', $twig->load('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() @@ -164,22 +210,6 @@ class Twig_Tests_Extension_SandboxTest extends \PHPUnit\Framework\TestCase $this->assertEquals(1, FooObject::$called['foo'], 'Sandbox only calls method once'); } - public function testSandboxAllowMethodToString() - { - $twig = $this->getEnvironment(true, [], self::$templates, [], [], ['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, [], 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 testSandboxAllowFilter() { $twig = $this->getEnvironment(true, [], self::$templates, [], ['upper']); @@ -319,4 +349,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 269a461..0000000 --- a/test/Twig/Tests/Node/SandboxedPrintTest.php +++ /dev/null @@ -1,33 +0,0 @@ -assertEquals($expr, $node->getNode('expr')); - } - - public function getTests() - { - $tests = []; - - $tests[] = [new Twig_Node_SandboxedPrint(new Twig_Node_Expression_Constant('foo', 1), 1), <<extensions['Twig_Extension_Sandbox']->ensureToStringAllowed("foo"); -EOF - ]; - - return $tests; - } -}