Browse code

Merge branch '1.x' into 2.x

* 1.x:
fixed security issue in the sandbox

Fabien Potencier authored on 12/03/2019 12:36:16
Showing 6 changed files
... ...
@@ -166,6 +166,9 @@
166 166
 
167 167
 * 1.38.0 (2019-XX-XX)
168 168
 
169
+ * fixed sandbox security issue (under some circumstances, calling the
170
+   __toString() method on an object was possible even if not allowed by the
171
+   security policy)
169 172
  * fixed batch filter clobbers array keys when fill parameter is used
170 173
  * added preserveKeys support for the batch filter
171 174
  * fixed "embed" support when used from "template_from_string"
172 175
new file mode 100644
... ...
@@ -0,0 +1,42 @@
1
+<?php
2
+
3
+/*
4
+ * This file is part of Twig.
5
+ *
6
+ * (c) Fabien Potencier
7
+ *
8
+ * For the full copyright and license information, please view the LICENSE
9
+ * file that was distributed with this source code.
10
+ */
11
+
12
+namespace Twig\Node;
13
+
14
+use Twig\Compiler;
15
+use Twig\Node\Expression\AbstractExpression;
16
+
17
+/**
18
+ * Checks if casting an expression to __toString() is allowed by the sandbox.
19
+ *
20
+ * For instance, when there is a simple Print statement, like {{ article }},
21
+ * and if the sandbox is enabled, we need to check that the __toString()
22
+ * method is allowed if 'article' is an object. The same goes for {{ article|upper }}
23
+ * or {{ random(article) }}
24
+ *
25
+ * @author Fabien Potencier <fabien@symfony.com>
26
+ */
27
+class CheckToStringNode extends Node
28
+{
29
+    public function __construct(AbstractExpression $expr)
30
+    {
31
+        parent::__construct(['expr' => $expr], [], $expr->getTemplateLine(), $expr->getNodeTag());
32
+    }
33
+
34
+    public function compile(Compiler $compiler)
35
+    {
36
+        $compiler
37
+            ->raw('$this->sandbox->ensureToStringAllowed(')
38
+            ->subcompile($this->getNode('expr'))
39
+            ->raw(')')
40
+        ;
41
+    }
42
+}
... ...
@@ -21,6 +21,8 @@ use Twig\Node\Expression\ConstantExpression;
21 21
  * and if the sandbox is enabled, we need to check that the __toString()
22 22
  * method is allowed if 'article' is an object.
23 23
  *
24
+ * Not used anymore, to be deprecated in 2.x and removed in 3.0
25
+ *
24 26
  * @author Fabien Potencier <fabien@symfony.com>
25 27
  */
26 28
 class SandboxedPrintNode extends PrintNode
... ...
@@ -13,13 +13,17 @@ namespace Twig\NodeVisitor;
13 13
 
14 14
 use Twig\Environment;
15 15
 use Twig\Node\CheckSecurityNode;
16
+use Twig\Node\CheckToStringNode;
17
+use Twig\Node\Expression\Binary\ConcatBinary;
16 18
 use Twig\Node\Expression\Binary\RangeBinary;
17 19
 use Twig\Node\Expression\FilterExpression;
18 20
 use Twig\Node\Expression\FunctionExpression;
21
+use Twig\Node\Expression\GetAttrExpression;
22
+use Twig\Node\Expression\NameExpression;
19 23
 use Twig\Node\ModuleNode;
20 24
 use Twig\Node\Node;
21 25
 use Twig\Node\PrintNode;
22
-use Twig\Node\SandboxedPrintNode;
26
+use Twig\Node\SetNode;
23 27
 
24 28
 /**
25 29
  * @author Fabien Potencier <fabien@symfony.com>
... ...
@@ -31,6 +35,8 @@ final class SandboxNodeVisitor extends AbstractNodeVisitor
31 35
     private $filters;
32 36
     private $functions;
33 37
 
38
+    private $needsToStringWrap = false;
39
+
34 40
     protected function doEnterNode(Node $node, Environment $env)
35 41
     {
36 42
         if ($node instanceof ModuleNode) {
... ...
@@ -61,9 +67,28 @@ final class SandboxNodeVisitor extends AbstractNodeVisitor
61 67
                 $this->functions['range'] = $node;
62 68
             }
63 69
 
64
-            // wrap print to check __toString() calls
65 70
             if ($node instanceof PrintNode) {
66
-                return new SandboxedPrintNode($node->getNode('expr'), $node->getTemplateLine(), $node->getNodeTag());
71
+                $this->needsToStringWrap = true;
72
+                $this->wrapNode($node, 'expr');
73
+            }
74
+
75
+            if ($node instanceof SetNode && !$node->getAttribute('capture')) {
76
+                $this->needsToStringWrap = true;
77
+            }
78
+
79
+            // wrap outer nodes that can implicitly call __toString()
80
+            if ($this->needsToStringWrap) {
81
+                if ($node instanceof ConcatBinary) {
82
+                    $this->wrapNode($node, 'left');
83
+                    $this->wrapNode($node, 'right');
84
+                }
85
+                if ($node instanceof FilterExpression) {
86
+                    $this->wrapNode($node, 'node');
87
+                    $this->wrapArrayNode($node, 'arguments');
88
+                }
89
+                if ($node instanceof FunctionExpression) {
90
+                    $this->wrapArrayNode($node, 'arguments');
91
+                }
67 92
             }
68 93
         }
69 94
 
... ...
@@ -76,11 +101,31 @@ final class SandboxNodeVisitor extends AbstractNodeVisitor
76 101
             $this->inAModule = false;
77 102
 
78 103
             $node->setNode('constructor_end', new Node([new CheckSecurityNode($this->filters, $this->tags, $this->functions), $node->getNode('display_start')]));
104
+        } elseif ($this->inAModule) {
105
+            if ($node instanceof PrintNode || $node instanceof SetNode) {
106
+                $this->needsToStringWrap = false;
107
+            }
79 108
         }
80 109
 
81 110
         return $node;
82 111
     }
83 112
 
113
+    private function wrapNode(Node $node, $name)
114
+    {
115
+        $expr = $node->getNode($name);
116
+        if ($expr instanceof NameExpression || $expr instanceof GetAttrExpression) {
117
+            $node->setNode($name, new CheckToStringNode($expr));
118
+        }
119
+    }
120
+
121
+    private function wrapArrayNode(Node $node, $name)
122
+    {
123
+        $args = $node->getNode($name);
124
+        foreach ($args as $name => $_) {
125
+            $this->wrapNode($args, $name);
126
+        }
127
+    }
128
+
84 129
     public function getPriority()
85 130
     {
86 131
         return 0;
... ...
@@ -39,7 +39,6 @@ class Twig_Tests_Extension_SandboxTest extends \PHPUnit\Framework\TestCase
39 39
             '1_basic3' => '{% if name %}foo{% endif %}',
40 40
             '1_basic4' => '{{ obj.bar }}',
41 41
             '1_basic5' => '{{ obj }}',
42
-            '1_basic6' => '{{ arr.obj }}',
43 42
             '1_basic7' => '{{ cycle(["foo","bar"], 1) }}',
44 43
             '1_basic8' => '{{ obj.getfoobar }}{{ obj.getFooBar }}',
45 44
             '1_basic9' => '{{ obj.foobar }}{{ obj.fooBar }}',
... ...
@@ -117,11 +116,14 @@ class Twig_Tests_Extension_SandboxTest extends \PHPUnit\Framework\TestCase
117 116
         }
118 117
     }
119 118
 
120
-    public function testSandboxUnallowedToString()
119
+    /**
120
+     * @dataProvider getSandboxUnallowedToStringTests
121
+     */
122
+    public function testSandboxUnallowedToString($template)
121 123
     {
122
-        $twig = $this->getEnvironment(true, [], self::$templates);
124
+        $twig = $this->getEnvironment(true, [], ['index' => $template], [], ['upper'], ['FooObject' => 'getAnotherFooObject'], [], ['random']);
123 125
         try {
124
-            $twig->load('1_basic5')->render(self::$params);
126
+            $twig->load('index')->render(self::$params);
125 127
             $this->fail('Sandbox throws a SecurityError exception if an unallowed method (__toString()) is called in the template');
126 128
         } catch (SecurityError $e) {
127 129
             $this->assertInstanceOf(SecurityNotAllowedMethodError::class, $e, 'Exception should be an instance of Twig_Sandbox_SecurityNotAllowedMethodError');
... ...
@@ -130,17 +132,61 @@ class Twig_Tests_Extension_SandboxTest extends \PHPUnit\Framework\TestCase
130 132
         }
131 133
     }
132 134
 
133
-    public function testSandboxUnallowedToStringArray()
135
+    public function getSandboxUnallowedToStringTests()
134 136
     {
135
-        $twig = $this->getEnvironment(true, [], self::$templates);
136
-        try {
137
-            $twig->load('1_basic6')->render(self::$params);
138
-            $this->fail('Sandbox throws a SecurityError exception if an unallowed method (__toString()) is called in the template');
139
-        } catch (SecurityError $e) {
140
-            $this->assertInstanceOf(SecurityNotAllowedMethodError::class, $e, 'Exception should be an instance of Twig_Sandbox_SecurityNotAllowedMethodError');
141
-            $this->assertEquals('FooObject', $e->getClassName(), 'Exception should be raised on the "FooObject" class');
142
-            $this->assertEquals('__tostring', $e->getMethodName(), 'Exception should be raised on the "__toString" method');
143
-        }
137
+        return [
138
+            'simple' => ['{{ obj }}'],
139
+            'object_from_array' => ['{{ arr.obj }}'],
140
+            'object_chain' => ['{{ obj.anotherFooObject }}'],
141
+            'filter' => ['{{ obj|upper }}'],
142
+            'filter_from_array' => ['{{ arr.obj|upper }}'],
143
+            'function' => ['{{ random(obj) }}'],
144
+            'function_from_array' => ['{{ random(arr.obj) }}'],
145
+            'function_and_filter' => ['{{ random(obj|upper) }}'],
146
+            'function_and_filter_from_array' => ['{{ random(arr.obj|upper) }}'],
147
+            'object_chain_and_filter' => ['{{ obj.anotherFooObject|upper }}'],
148
+            'object_chain_and_function' => ['{{ random(obj.anotherFooObject) }}'],
149
+            'concat' => ['{{ obj ~ "" }}'],
150
+            'concat_again' => ['{{ "" ~ obj }}'],
151
+        ];
152
+    }
153
+
154
+    /**
155
+     * @dataProvider getSandboxAllowedToStringTests
156
+     */
157
+    public function testSandboxAllowedToString($template, $output)
158
+    {
159
+        $twig = $this->getEnvironment(true, [], ['index' => $template], ['set'], [], ['FooObject' => ['foo', 'getAnotherFooObject']]);
160
+        $this->assertEquals($output, $twig->load('index')->render(self::$params));
161
+    }
162
+
163
+    public function getSandboxAllowedToStringTests()
164
+    {
165
+        return [
166
+            'constant_test' => ['{{ obj is constant("PHP_INT_MAX") }}', ''],
167
+            'set_object' => ['{% set a = obj.anotherFooObject %}{{ a.foo }}', 'foo'],
168
+            'is_defined' => ['{{ obj.anotherFooObject is defined }}', '1'],
169
+            'is_null' => ['{{ obj is null }}', ''],
170
+            'is_sameas' => ['{{ obj is same as(obj) }}', '1'],
171
+            'is_sameas_from_array' => ['{{ arr.obj is same as(arr.obj) }}', '1'],
172
+            'is_sameas_from_another_method' => ['{{ obj.anotherFooObject is same as(obj.anotherFooObject) }}', ''],
173
+        ];
174
+    }
175
+
176
+    public function testSandboxAllowMethodToString()
177
+    {
178
+        $twig = $this->getEnvironment(true, [], self::$templates, [], [], ['FooObject' => '__toString']);
179
+        FooObject::reset();
180
+        $this->assertEquals('foo', $twig->load('1_basic5')->render(self::$params), 'Sandbox allow some methods');
181
+        $this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once');
182
+    }
183
+
184
+    public function testSandboxAllowMethodToStringDisabled()
185
+    {
186
+        $twig = $this->getEnvironment(false, [], self::$templates);
187
+        FooObject::reset();
188
+        $this->assertEquals('foo', $twig->load('1_basic5')->render(self::$params), 'Sandbox allows __toString when sandbox disabled');
189
+        $this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once');
144 190
     }
145 191
 
146 192
     public function testSandboxUnallowedFunction()
... ...
@@ -175,22 +221,6 @@ class Twig_Tests_Extension_SandboxTest extends \PHPUnit\Framework\TestCase
175 221
         $this->assertEquals(1, FooObject::$called['foo'], 'Sandbox only calls method once');
176 222
     }
177 223
 
178
-    public function testSandboxAllowMethodToString()
179
-    {
180
-        $twig = $this->getEnvironment(true, [], self::$templates, [], [], ['FooObject' => '__toString']);
181
-        FooObject::reset();
182
-        $this->assertEquals('foo', $twig->load('1_basic5')->render(self::$params), 'Sandbox allow some methods');
183
-        $this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once');
184
-    }
185
-
186
-    public function testSandboxAllowMethodToStringDisabled()
187
-    {
188
-        $twig = $this->getEnvironment(false, [], self::$templates);
189
-        FooObject::reset();
190
-        $this->assertEquals('foo', $twig->load('1_basic5')->render(self::$params), 'Sandbox allows __toString when sandbox disabled');
191
-        $this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once');
192
-    }
193
-
194 224
     public function testSandboxAllowFilter()
195 225
     {
196 226
         $twig = $this->getEnvironment(true, [], self::$templates, [], ['upper']);
... ...
@@ -330,4 +360,9 @@ class FooObject
330 360
 
331 361
         return 'foobar';
332 362
     }
363
+
364
+    public function getAnotherFooObject()
365
+    {
366
+        return new self();
367
+    }
333 368
 }
334 369
deleted file mode 100644
... ...
@@ -1,42 +0,0 @@
1
-<?php
2
-
3
-/*
4
- * This file is part of Twig.
5
- *
6
- * (c) Fabien Potencier
7
- *
8
- * For the full copyright and license information, please view the LICENSE
9
- * file that was distributed with this source code.
10
- */
11
-
12
-use Twig\Node\Expression\ConstantExpression;
13
-use Twig\Node\Expression\NameExpression;
14
-use Twig\Node\SandboxedPrintNode;
15
-use Twig\Test\NodeTestCase;
16
-
17
-class Twig_Tests_Node_SandboxedPrintTest extends NodeTestCase
18
-{
19
-    public function testConstructor()
20
-    {
21
-        $node = new SandboxedPrintNode($expr = new ConstantExpression('foo', 1), 1);
22
-
23
-        $this->assertEquals($expr, $node->getNode('expr'));
24
-    }
25
-
26
-    public function getTests()
27
-    {
28
-        $tests[] = [new SandboxedPrintNode(new ConstantExpression('foo', 1), 1), <<<EOF
29
-// line 1
30
-echo "foo";
31
-EOF
32
-        ];
33
-
34
-        $tests[] = [new SandboxedPrintNode(new NameExpression('foo', 1), 1), <<<EOF
35
-// line 1
36
-echo \$this->extensions[SandboxExtension::class]->ensureToStringAllowed({$this->getVariableGetter('foo', false)});
37
-EOF
38
-        ];
39
-
40
-        return $tests;
41
-    }
42
-}