From eac5422956e1dcca89a3669a03a3ff32f0502077 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sun, 10 Mar 2019 18:31:18 +0100 Subject: [PATCH] fixed security issue in the sandbox --- CHANGELOG | 3 + src/Node/CheckToStringNode.php | 42 +++++++++ src/Node/SandboxedPrintNode.php | 2 + src/NodeVisitor/SandboxNodeVisitor.php | 51 ++++++++++- test/Twig/Tests/Extension/SandboxTest.php | 95 ++++++++++++++------- test/Twig/Tests/Node/SandboxedPrintTest.php | 42 --------- 6 files changed, 160 insertions(+), 75 deletions(-) create mode 100644 src/Node/CheckToStringNode.php delete mode 100644 test/Twig/Tests/Node/SandboxedPrintTest.php diff --git a/CHANGELOG b/CHANGELOG index b5e117f4b2..778fc6a1f6 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,8 @@ * 1.38.0 (2019-XX-XX) + * fixed sandbox security issue (under some circumstances, calling the + __toString() method on an object was possible even if not allowed by the + security policy) * fixed batch filter clobbers array keys when fill parameter is used * added preserveKeys support for the batch filter * fixed "embed" support when used from "template_from_string" diff --git a/src/Node/CheckToStringNode.php b/src/Node/CheckToStringNode.php new file mode 100644 index 0000000000..464c96fbae --- /dev/null +++ b/src/Node/CheckToStringNode.php @@ -0,0 +1,42 @@ + + */ +class CheckToStringNode extends Node +{ + public function __construct(AbstractExpression $expr) + { + parent::__construct(['expr' => $expr], [], $expr->getTemplateLine(), $expr->getNodeTag()); + } + + public function compile(Compiler $compiler) + { + $compiler + ->raw('$this->sandbox->ensureToStringAllowed(') + ->subcompile($this->getNode('expr')) + ->raw(')') + ; + } +} diff --git a/src/Node/SandboxedPrintNode.php b/src/Node/SandboxedPrintNode.php index efb7b35777..2359af9110 100644 --- a/src/Node/SandboxedPrintNode.php +++ b/src/Node/SandboxedPrintNode.php @@ -22,6 +22,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 SandboxedPrintNode extends PrintNode diff --git a/src/NodeVisitor/SandboxNodeVisitor.php b/src/NodeVisitor/SandboxNodeVisitor.php index 726d875019..c1d302a89b 100644 --- a/src/NodeVisitor/SandboxNodeVisitor.php +++ b/src/NodeVisitor/SandboxNodeVisitor.php @@ -13,13 +13,17 @@ use Twig\Environment; use Twig\Node\CheckSecurityNode; +use Twig\Node\CheckToStringNode; +use Twig\Node\Expression\Binary\ConcatBinary; use Twig\Node\Expression\Binary\RangeBinary; use Twig\Node\Expression\FilterExpression; use Twig\Node\Expression\FunctionExpression; +use Twig\Node\Expression\GetAttrExpression; +use Twig\Node\Expression\NameExpression; use Twig\Node\ModuleNode; use Twig\Node\Node; use Twig\Node\PrintNode; -use Twig\Node\SandboxedPrintNode; +use Twig\Node\SetNode; /** * @final @@ -33,6 +37,8 @@ class SandboxNodeVisitor extends AbstractNodeVisitor protected $filters; protected $functions; + private $needsToStringWrap = false; + protected function doEnterNode(Node $node, Environment $env) { if ($node instanceof ModuleNode) { @@ -63,9 +69,28 @@ protected function doEnterNode(Node $node, Environment $env) $this->functions['range'] = $node; } - // wrap print to check __toString() calls if ($node instanceof PrintNode) { - return new SandboxedPrintNode($node->getNode('expr'), $node->getTemplateLine(), $node->getNodeTag()); + $this->needsToStringWrap = true; + $this->wrapNode($node, 'expr'); + } + + if ($node instanceof SetNode && !$node->getAttribute('capture')) { + $this->needsToStringWrap = true; + } + + // wrap outer nodes that can implicitly call __toString() + if ($this->needsToStringWrap) { + if ($node instanceof ConcatBinary) { + $this->wrapNode($node, 'left'); + $this->wrapNode($node, 'right'); + } + if ($node instanceof FilterExpression) { + $this->wrapNode($node, 'node'); + $this->wrapArrayNode($node, 'arguments'); + } + if ($node instanceof FunctionExpression) { + $this->wrapArrayNode($node, 'arguments'); + } } } @@ -78,11 +103,31 @@ protected function doLeaveNode(Node $node, Environment $env) $this->inAModule = false; $node->setNode('constructor_end', new Node([new CheckSecurityNode($this->filters, $this->tags, $this->functions), $node->getNode('display_start')])); + } elseif ($this->inAModule) { + if ($node instanceof PrintNode || $node instanceof SetNode) { + $this->needsToStringWrap = false; + } } return $node; } + private function wrapNode(Node $node, $name) + { + $expr = $node->getNode($name); + if ($expr instanceof NameExpression || $expr instanceof GetAttrExpression) { + $node->setNode($name, new CheckToStringNode($expr)); + } + } + + private function wrapArrayNode(Node $node, $name) + { + $args = $node->getNode($name); + foreach ($args as $name => $_) { + $this->wrapNode($args, $name); + } + } + public function getPriority() { return 0; diff --git a/test/Twig/Tests/Extension/SandboxTest.php b/test/Twig/Tests/Extension/SandboxTest.php index e554bfbc32..de9ced0234 100644 --- a/test/Twig/Tests/Extension/SandboxTest.php +++ b/test/Twig/Tests/Extension/SandboxTest.php @@ -34,7 +34,6 @@ protected function setUp() '1_basic3' => '{% 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 }}', @@ -112,11 +111,14 @@ public function testSandboxUnallowedProperty() } } - 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->load('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 (SecurityError $e) { $this->assertInstanceOf('\Twig\Sandbox\SecurityNotAllowedMethodError', $e, 'Exception should be an instance of Twig_Sandbox_SecurityNotAllowedMethodError'); @@ -125,17 +127,61 @@ public function testSandboxUnallowedToString() } } - public function testSandboxUnallowedToStringArray() + public function getSandboxUnallowedToStringTests() { - $twig = $this->getEnvironment(true, [], self::$templates); - try { - $twig->load('1_basic6')->render(self::$params); - $this->fail('Sandbox throws a SecurityError exception if an unallowed method (__toString()) is called in the template'); - } catch (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() @@ -170,22 +216,6 @@ public function testSandboxAllowMethodFoo() $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->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 testSandboxAllowFilter() { $twig = $this->getEnvironment(true, [], self::$templates, [], ['upper']); @@ -326,4 +356,9 @@ public function getFooBar() 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 14c119135f..0000000000 --- a/test/Twig/Tests/Node/SandboxedPrintTest.php +++ /dev/null @@ -1,42 +0,0 @@ -assertEquals($expr, $node->getNode('expr')); - } - - public function getTests() - { - $tests[] = [new SandboxedPrintNode(new ConstantExpression('foo', 1), 1), <<env->getExtension('\Twig\Extension\SandboxExtension')->ensureToStringAllowed({$this->getVariableGetter('foo', false)}); -EOF - ]; - - return $tests; - } -}