@@ -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"

new file mode 100644

@@ -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(')')

+        ;

+    }

+}

@@ -22,6 +22,8 @@ use Twig\Node\Expression\FilterExpression;

  * 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

@@ -13,13 +13,17 @@ namespace Twig\NodeVisitor;

 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 @@ class SandboxNodeVisitor extends AbstractNodeVisitor

                 $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 @@ class SandboxNodeVisitor extends AbstractNodeVisitor

             $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;

@@ -34,7 +34,6 @@ class Twig_Tests_Extension_SandboxTest extends \PHPUnit\Framework\TestCase

             '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 @@ 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->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 @@ class Twig_Tests_Extension_SandboxTest extends \PHPUnit\Framework\TestCase

         }

     }

-    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 @@ 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->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 @@ class FooObject

         return 'foobar';

     }

+

+    public function getAnotherFooObject()

+    {

+        return new self();

+    }

 }

deleted file mode 100644

@@ -1,42 +0,0 @@

-<?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.

- */

-

-use Twig\Node\Expression\ConstantExpression;

-use Twig\Node\Expression\NameExpression;

-use Twig\Node\SandboxedPrintNode;

-use Twig\Test\NodeTestCase;

-

-class Twig_Tests_Node_SandboxedPrintTest extends NodeTestCase

-{

-    public function testConstructor()

-    {

-        $node = new SandboxedPrintNode($expr = new ConstantExpression('foo', 1), 1);

-

-        $this->assertEquals($expr, $node->getNode('expr'));

-    }

-

-    public function getTests()

-    {

-        $tests[] = [new SandboxedPrintNode(new ConstantExpression('foo', 1), 1), <<<EOF

-// line 1

-echo "foo";

-EOF

-        ];

-

-        $tests[] = [new SandboxedPrintNode(new NameExpression('foo', 1), 1), <<<EOF

-// line 1

-echo \$this->env->getExtension('\Twig\Extension\SandboxExtension')->ensureToStringAllowed({$this->getVariableGetter('foo', false)});

-EOF

-        ];

-

-        return $tests;

-    }

-}