security #2885 Fix security issue in the sandbox (fabpot)

This PR was merged into the 1.x branch. Discussion ---------- Fix security issue in the sandbox Commits ------- eac5422 fixed security issue in the sandbox
twigphp · Mar 12, 2019 · 0f3af98 · 0f3af98
2 parents 5e1a361 + eac5422
commit 0f3af98
Show file tree

Hide file tree

Showing 6 changed files with 160 additions and 75 deletions.
diff --git 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
@@ -0,0 +1,42 @@
+<?php
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Twig\Node;
+
+use Twig\Compiler;
+use Twig\Node\Expression\AbstractExpression;
+
+/**
+ * Checks if casting an expression to __toString() is allowed by the sandbox.
+ *
+ * For instance, when there is a simple Print statement, like {{ article }},
+ * and if the sandbox is enabled, we need to check that the __toString()
+ * method is allowed if 'article' is an object. The same goes for {{ article|upper }}
+ * or {{ random(article) }}
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+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
@@ -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 <fabien@symfony.com>
  */
 class SandboxedPrintNode extends PrintNode

diff --git 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
@@ -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