Skip to content

Commit eac5422

Browse files
committed
fixed security issue in the sandbox
1 parent 7e30569 commit eac5422

File tree

6 files changed

+160
-75
lines changed

6 files changed

+160
-75
lines changed

Diff for: CHANGELOG

+3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
* 1.38.0 (2019-XX-XX)
22

3+
* fixed sandbox security issue (under some circumstances, calling the
4+
__toString() method on an object was possible even if not allowed by the
5+
security policy)
36
* fixed batch filter clobbers array keys when fill parameter is used
47
* added preserveKeys support for the batch filter
58
* fixed "embed" support when used from "template_from_string"

Diff for: src/Node/CheckToStringNode.php

+42
Original file line numberDiff line numberDiff line change
@@ -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+
}

Diff for: src/Node/SandboxedPrintNode.php

+2
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
* and if the sandbox is enabled, we need to check that the __toString()
2323
* method is allowed if 'article' is an object.
2424
*
25+
* Not used anymore, to be deprecated in 2.x and removed in 3.0
26+
*
2527
* @author Fabien Potencier <fabien@symfony.com>
2628
*/
2729
class SandboxedPrintNode extends PrintNode

Diff for: src/NodeVisitor/SandboxNodeVisitor.php

+48-3
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,17 @@
1313

1414
use Twig\Environment;
1515
use Twig\Node\CheckSecurityNode;
16+
use Twig\Node\CheckToStringNode;
17+
use Twig\Node\Expression\Binary\ConcatBinary;
1618
use Twig\Node\Expression\Binary\RangeBinary;
1719
use Twig\Node\Expression\FilterExpression;
1820
use Twig\Node\Expression\FunctionExpression;
21+
use Twig\Node\Expression\GetAttrExpression;
22+
use Twig\Node\Expression\NameExpression;
1923
use Twig\Node\ModuleNode;
2024
use Twig\Node\Node;
2125
use Twig\Node\PrintNode;
22-
use Twig\Node\SandboxedPrintNode;
26+
use Twig\Node\SetNode;
2327

2428
/**
2529
* @final
@@ -33,6 +37,8 @@ class SandboxNodeVisitor extends AbstractNodeVisitor
3337
protected $filters;
3438
protected $functions;
3539

40+
private $needsToStringWrap = false;
41+
3642
protected function doEnterNode(Node $node, Environment $env)
3743
{
3844
if ($node instanceof ModuleNode) {
@@ -63,9 +69,28 @@ protected function doEnterNode(Node $node, Environment $env)
6369
$this->functions['range'] = $node;
6470
}
6571

66-
// wrap print to check __toString() calls
6772
if ($node instanceof PrintNode) {
68-
return new SandboxedPrintNode($node->getNode('expr'), $node->getTemplateLine(), $node->getNodeTag());
73+
$this->needsToStringWrap = true;
74+
$this->wrapNode($node, 'expr');
75+
}
76+
77+
if ($node instanceof SetNode && !$node->getAttribute('capture')) {
78+
$this->needsToStringWrap = true;
79+
}
80+
81+
// wrap outer nodes that can implicitly call __toString()
82+
if ($this->needsToStringWrap) {
83+
if ($node instanceof ConcatBinary) {
84+
$this->wrapNode($node, 'left');
85+
$this->wrapNode($node, 'right');
86+
}
87+
if ($node instanceof FilterExpression) {
88+
$this->wrapNode($node, 'node');
89+
$this->wrapArrayNode($node, 'arguments');
90+
}
91+
if ($node instanceof FunctionExpression) {
92+
$this->wrapArrayNode($node, 'arguments');
93+
}
6994
}
7095
}
7196

@@ -78,11 +103,31 @@ protected function doLeaveNode(Node $node, Environment $env)
78103
$this->inAModule = false;
79104

80105
$node->setNode('constructor_end', new Node([new CheckSecurityNode($this->filters, $this->tags, $this->functions), $node->getNode('display_start')]));
106+
} elseif ($this->inAModule) {
107+
if ($node instanceof PrintNode || $node instanceof SetNode) {
108+
$this->needsToStringWrap = false;
109+
}
81110
}
82111

83112
return $node;
84113
}
85114

115+
private function wrapNode(Node $node, $name)
116+
{
117+
$expr = $node->getNode($name);
118+
if ($expr instanceof NameExpression || $expr instanceof GetAttrExpression) {
119+
$node->setNode($name, new CheckToStringNode($expr));
120+
}
121+
}
122+
123+
private function wrapArrayNode(Node $node, $name)
124+
{
125+
$args = $node->getNode($name);
126+
foreach ($args as $name => $_) {
127+
$this->wrapNode($args, $name);
128+
}
129+
}
130+
86131
public function getPriority()
87132
{
88133
return 0;

Diff for: test/Twig/Tests/Extension/SandboxTest.php

+65-30
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ protected function setUp()
3434
'1_basic3' => '{% if name %}foo{% endif %}',
3535
'1_basic4' => '{{ obj.bar }}',
3636
'1_basic5' => '{{ obj }}',
37-
'1_basic6' => '{{ arr.obj }}',
3837
'1_basic7' => '{{ cycle(["foo","bar"], 1) }}',
3938
'1_basic8' => '{{ obj.getfoobar }}{{ obj.getFooBar }}',
4039
'1_basic9' => '{{ obj.foobar }}{{ obj.fooBar }}',
@@ -112,11 +111,14 @@ public function testSandboxUnallowedProperty()
112111
}
113112
}
114113

115-
public function testSandboxUnallowedToString()
114+
/**
115+
* @dataProvider getSandboxUnallowedToStringTests
116+
*/
117+
public function testSandboxUnallowedToString($template)
116118
{
117-
$twig = $this->getEnvironment(true, [], self::$templates);
119+
$twig = $this->getEnvironment(true, [], ['index' => $template], [], ['upper'], ['FooObject' => 'getAnotherFooObject'], [], ['random']);
118120
try {
119-
$twig->load('1_basic5')->render(self::$params);
121+
$twig->loadTemplate('index')->render(self::$params);
120122
$this->fail('Sandbox throws a SecurityError exception if an unallowed method (__toString()) is called in the template');
121123
} catch (SecurityError $e) {
122124
$this->assertInstanceOf('\Twig\Sandbox\SecurityNotAllowedMethodError', $e, 'Exception should be an instance of Twig_Sandbox_SecurityNotAllowedMethodError');
@@ -125,17 +127,61 @@ public function testSandboxUnallowedToString()
125127
}
126128
}
127129

128-
public function testSandboxUnallowedToStringArray()
130+
public function getSandboxUnallowedToStringTests()
129131
{
130-
$twig = $this->getEnvironment(true, [], self::$templates);
131-
try {
132-
$twig->load('1_basic6')->render(self::$params);
133-
$this->fail('Sandbox throws a SecurityError exception if an unallowed method (__toString()) is called in the template');
134-
} catch (SecurityError $e) {
135-
$this->assertInstanceOf('\Twig\Sandbox\SecurityNotAllowedMethodError', $e, 'Exception should be an instance of Twig_Sandbox_SecurityNotAllowedMethodError');
136-
$this->assertEquals('FooObject', $e->getClassName(), 'Exception should be raised on the "FooObject" class');
137-
$this->assertEquals('__tostring', $e->getMethodName(), 'Exception should be raised on the "__toString" method');
138-
}
132+
return [
133+
'simple' => ['{{ obj }}'],
134+
'object_from_array' => ['{{ arr.obj }}'],
135+
'object_chain' => ['{{ obj.anotherFooObject }}'],
136+
'filter' => ['{{ obj|upper }}'],
137+
'filter_from_array' => ['{{ arr.obj|upper }}'],
138+
'function' => ['{{ random(obj) }}'],
139+
'function_from_array' => ['{{ random(arr.obj) }}'],
140+
'function_and_filter' => ['{{ random(obj|upper) }}'],
141+
'function_and_filter_from_array' => ['{{ random(arr.obj|upper) }}'],
142+
'object_chain_and_filter' => ['{{ obj.anotherFooObject|upper }}'],
143+
'object_chain_and_function' => ['{{ random(obj.anotherFooObject) }}'],
144+
'concat' => ['{{ obj ~ "" }}'],
145+
'concat_again' => ['{{ "" ~ obj }}'],
146+
];
147+
}
148+
149+
/**
150+
* @dataProvider getSandboxAllowedToStringTests
151+
*/
152+
public function testSandboxAllowedToString($template, $output)
153+
{
154+
$twig = $this->getEnvironment(true, [], ['index' => $template], ['set'], [], ['FooObject' => ['foo', 'getAnotherFooObject']]);
155+
$this->assertEquals($output, $twig->load('index')->render(self::$params));
156+
}
157+
158+
public function getSandboxAllowedToStringTests()
159+
{
160+
return [
161+
'constant_test' => ['{{ obj is constant("PHP_INT_MAX") }}', ''],
162+
'set_object' => ['{% set a = obj.anotherFooObject %}{{ a.foo }}', 'foo'],
163+
'is_defined' => ['{{ obj.anotherFooObject is defined }}', '1'],
164+
'is_null' => ['{{ obj is null }}', ''],
165+
'is_sameas' => ['{{ obj is same as(obj) }}', '1'],
166+
'is_sameas_from_array' => ['{{ arr.obj is same as(arr.obj) }}', '1'],
167+
'is_sameas_from_another_method' => ['{{ obj.anotherFooObject is same as(obj.anotherFooObject) }}', ''],
168+
];
169+
}
170+
171+
public function testSandboxAllowMethodToString()
172+
{
173+
$twig = $this->getEnvironment(true, [], self::$templates, [], [], ['FooObject' => '__toString']);
174+
FooObject::reset();
175+
$this->assertEquals('foo', $twig->load('1_basic5')->render(self::$params), 'Sandbox allow some methods');
176+
$this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once');
177+
}
178+
179+
public function testSandboxAllowMethodToStringDisabled()
180+
{
181+
$twig = $this->getEnvironment(false, [], self::$templates);
182+
FooObject::reset();
183+
$this->assertEquals('foo', $twig->load('1_basic5')->render(self::$params), 'Sandbox allows __toString when sandbox disabled');
184+
$this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once');
139185
}
140186

141187
public function testSandboxUnallowedFunction()
@@ -170,22 +216,6 @@ public function testSandboxAllowMethodFoo()
170216
$this->assertEquals(1, FooObject::$called['foo'], 'Sandbox only calls method once');
171217
}
172218

173-
public function testSandboxAllowMethodToString()
174-
{
175-
$twig = $this->getEnvironment(true, [], self::$templates, [], [], ['FooObject' => '__toString']);
176-
FooObject::reset();
177-
$this->assertEquals('foo', $twig->load('1_basic5')->render(self::$params), 'Sandbox allow some methods');
178-
$this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once');
179-
}
180-
181-
public function testSandboxAllowMethodToStringDisabled()
182-
{
183-
$twig = $this->getEnvironment(false, [], self::$templates);
184-
FooObject::reset();
185-
$this->assertEquals('foo', $twig->load('1_basic5')->render(self::$params), 'Sandbox allows __toString when sandbox disabled');
186-
$this->assertEquals(1, FooObject::$called['__toString'], 'Sandbox only calls method once');
187-
}
188-
189219
public function testSandboxAllowFilter()
190220
{
191221
$twig = $this->getEnvironment(true, [], self::$templates, [], ['upper']);
@@ -326,4 +356,9 @@ public function getFooBar()
326356

327357
return 'foobar';
328358
}
359+
360+
public function getAnotherFooObject()
361+
{
362+
return new self();
363+
}
329364
}

Diff for: test/Twig/Tests/Node/SandboxedPrintTest.php

-42
This file was deleted.

0 commit comments

Comments
 (0)