Skip to content

Commit b957e5a

Browse files
nicolas-grekasfabpot
nicolas-grekas
authored and
fabpot
committed
Sandbox ArrayAccess and do sandbox checks before isset() checks
1 parent d4a3026 commit b957e5a

File tree

4 files changed

+153
-15
lines changed

4 files changed

+153
-15
lines changed

‎doc/sandbox.rst‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,15 @@ able to call the ``getTitle()`` and ``getBody()`` methods on ``Article``
3737
objects, and the ``title`` and ``body`` public properties. Everything else
3838
won't be allowed and will generate a ``\Twig\Sandbox\SecurityError`` exception.
3939

40+
.. note::
41+
42+
As of Twig 1.14.1 (and on Twig 3.11.2), if the ``Article`` class implements
43+
the ``ArrayAccess`` interface, the templates will only be able to access
44+
the ``title`` and ``body`` attributes.
45+
46+
Note that native array-like classes (like ``ArrayObject``) are always
47+
allowed, you don't need to configure them.
48+
4049
.. caution::
4150

4251
The ``extends`` and ``use`` tags are always allowed in a sandboxed

‎src/Extension/CoreExtension.php‎

Lines changed: 56 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,8 @@
6868
use Twig\Node\Node;
6969
use Twig\OperatorPrecedenceChange;
7070
use Twig\Parser;
71+
use Twig\Sandbox\SecurityNotAllowedMethodError;
72+
use Twig\Sandbox\SecurityNotAllowedPropertyError;
7173
use Twig\Source;
7274
use Twig\Template;
7375
use Twig\TemplateWrapper;
@@ -98,6 +100,20 @@ final class CoreExtension extends AbstractExtension
98100
{
99101
private const DEFAULT_TRIM_CHARS = " \t\n\r\0\x0B";
100102

103+
public const ARRAY_LIKE_CLASSES = [
104+
'ArrayIterator',
105+
'ArrayObject',
106+
'CachingIterator',
107+
'RecursiveArrayIterator',
108+
'RecursiveCachingIterator',
109+
'SplDoublyLinkedList',
110+
'SplFixedArray',
111+
'SplObjectStorage',
112+
'SplQueue',
113+
'SplStack',
114+
'WeakMap',
115+
];
116+
101117
private $dateFormats = ['F j, Y H:i', '%d days'];
102118
private $numberFormat = [0, '.', ','];
103119
private $timezone = null;
@@ -1622,10 +1638,20 @@ public static function batch($items, $size, $fill = null, $preserveKeys = true):
16221638
*/
16231639
public static function getAttribute(Environment $env, Source $source, $object, $item, array $arguments = [], $type = Template::ANY_CALL, $isDefinedTest = false, $ignoreStrictCheck = false, $sandboxed = false, int $lineno = -1)
16241640
{
1641+
$propertyNotAllowedError = null;
1642+
16251643
// array
16261644
if (Template::METHOD_CALL !== $type) {
16271645
$arrayItem = \is_bool($item) || \is_float($item) ? (int) $item : $item;
16281646

1647+
if ($sandboxed && $object instanceof \ArrayAccess && !\in_array($object::class, self::ARRAY_LIKE_CLASSES, true)) {
1648+
try {
1649+
$env->getExtension(SandboxExtension::class)->checkPropertyAllowed($object, $arrayItem, $lineno, $source);
1650+
} catch (SecurityNotAllowedPropertyError $propertyNotAllowedError) {
1651+
goto methodCheck;
1652+
}
1653+
}
1654+
16291655
if (((\is_array($object) || $object instanceof \ArrayObject) && (isset($object[$arrayItem]) || \array_key_exists($arrayItem, (array) $object)))
16301656
|| ($object instanceof \ArrayAccess && isset($object[$arrayItem]))
16311657
) {
@@ -1697,6 +1723,14 @@ public static function getAttribute(Environment $env, Source $source, $object, $
16971723

16981724
// object property
16991725
if (Template::METHOD_CALL !== $type) {
1726+
if ($sandboxed) {
1727+
try {
1728+
$env->getExtension(SandboxExtension::class)->checkPropertyAllowed($object, $item, $lineno, $source);
1729+
} catch (SecurityNotAllowedPropertyError $propertyNotAllowedError) {
1730+
goto methodCheck;
1731+
}
1732+
}
1733+
17001734
if (isset($object->$item) || \array_key_exists((string) $item, (array) $object)) {
17011735
if ($isDefinedTest) {
17021736
return true;
@@ -1722,6 +1756,8 @@ public static function getAttribute(Environment $env, Source $source, $object, $
17221756
}
17231757
}
17241758

1759+
methodCheck:
1760+
17251761
static $cache = [];
17261762

17271763
$class = \get_class($object);
@@ -1780,19 +1816,35 @@ public static function getAttribute(Environment $env, Source $source, $object, $
17801816
return false;
17811817
}
17821818

1819+
if ($propertyNotAllowedError) {
1820+
throw $propertyNotAllowedError;
1821+
}
1822+
17831823
if ($ignoreStrictCheck || !$env->isStrictVariables()) {
17841824
return;
17851825
}
17861826

17871827
throw new RuntimeError(\sprintf('Neither the property "%1$s" nor one of the methods "%1$s()", "get%1$s()"/"is%1$s()"/"has%1$s()" or "__call()" exist and have public access in class "%2$s".', $item, $class), $lineno, $source);
17881828
}
17891829

1790-
if ($isDefinedTest) {
1791-
return true;
1830+
if ($sandboxed) {
1831+
try {
1832+
$env->getExtension(SandboxExtension::class)->checkMethodAllowed($object, $method, $lineno, $source);
1833+
} catch (SecurityNotAllowedMethodError $e) {
1834+
if ($isDefinedTest) {
1835+
return false;
1836+
}
1837+
1838+
if ($propertyNotAllowedError) {
1839+
throw $propertyNotAllowedError;
1840+
}
1841+
1842+
throw $e;
1843+
}
17921844
}
17931845

1794-
if ($sandboxed) {
1795-
$env->getExtension(SandboxExtension::class)->checkMethodAllowed($object, $method, $lineno, $source);
1846+
if ($isDefinedTest) {
1847+
return true;
17961848
}
17971849

17981850
// Some objects throw exceptions when they have __call, and the method we try

‎src/Node/Expression/GetAttrExpression.php‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ public function __construct(AbstractExpression $node, AbstractExpression $attrib
3939
public function compile(Compiler $compiler): void
4040
{
4141
$env = $compiler->getEnvironment();
42+
$arrayAccessSandbox = false;
4243

4344
// optimize array calls
4445
if (
@@ -52,17 +53,35 @@ public function compile(Compiler $compiler): void
5253
->raw('(('.$var.' = ')
5354
->subcompile($this->getNode('node'))
5455
->raw(') && is_array(')
55-
->raw($var)
56+
->raw($var);
57+
58+
if (!$env->hasExtension(SandboxExtension::class)) {
59+
$compiler
60+
->raw(') || ')
61+
->raw($var)
62+
->raw(' instanceof ArrayAccess ? (')
63+
->raw($var)
64+
->raw('[')
65+
->subcompile($this->getNode('attribute'))
66+
->raw('] ?? null) : null)')
67+
;
68+
69+
return;
70+
}
71+
72+
$arrayAccessSandbox = true;
73+
74+
$compiler
5675
->raw(') || ')
5776
->raw($var)
58-
->raw(' instanceof ArrayAccess ? (')
77+
->raw(' instanceof ArrayAccess && in_array(')
78+
->raw($var.'::class')
79+
->raw(', CoreExtension::ARRAY_LIKE_CLASSES, true) ? (')
5980
->raw($var)
6081
->raw('[')
6182
->subcompile($this->getNode('attribute'))
62-
->raw('] ?? null) : null)')
83+
->raw('] ?? null) : ')
6384
;
64-
65-
return;
6685
}
6786

6887
$compiler->raw('CoreExtension::getAttribute($this->env, $this->source, ');
@@ -91,5 +110,9 @@ public function compile(Compiler $compiler): void
91110
->raw(', ')->repr($this->getNode('node')->getTemplateLine())
92111
->raw(')')
93112
;
113+
114+
if ($arrayAccessSandbox) {
115+
$compiler->raw(')');
116+
}
94117
}
95118
}

‎tests/Extension/SandboxTest.php‎

Lines changed: 60 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ protected function setUp(): void
4343
'arr' => ['obj' => new FooObject()],
4444
'child_obj' => new ChildClass(),
4545
'some_array' => [5, 6, 7, new FooObject()],
46+
'array_like' => new ArrayLikeObject(),
47+
'magic' => new MagicObject(),
4648
];
4749

4850
self::$templates = [
@@ -67,6 +69,7 @@ protected function setUp(): void
6769
'1_childobj_parentmethod' => '{{ child_obj.ParentMethod() }}',
6870
'1_childobj_childmethod' => '{{ child_obj.ChildMethod() }}',
6971
'1_empty' => '',
72+
'1_array_like' => '{{ array_like["foo"] }}',
7073
];
7174
}
7275

@@ -141,15 +144,31 @@ public function testSandboxGloballySet()
141144
$this->assertEquals('FOO', $twig->load('1_basic')->render(self::$params), 'Sandbox does nothing if it is disabled globally');
142145
}
143146

144-
public function testSandboxUnallowedMethodAccessor()
147+
public function testSandboxUnallowedPropertyAccessor()
145148
{
146149
$twig = $this->getEnvironment(true, [], self::$templates);
147150
try {
148-
$twig->load('1_basic1')->render(self::$params);
151+
$twig->load('1_basic1')->render(['obj' => new MagicObject()]);
149152
$this->fail('Sandbox throws a SecurityError exception if an unallowed method is called');
150-
} catch (SecurityNotAllowedMethodError $e) {
151-
$this->assertEquals('Twig\Tests\Extension\FooObject', $e->getClassName(), 'Exception should be raised on the "Twig\Tests\Extension\FooObject" class');
152-
$this->assertEquals('foo', $e->getMethodName(), 'Exception should be raised on the "foo" method');
153+
} catch (SecurityNotAllowedPropertyError $e) {
154+
$this->assertEquals('Twig\Tests\Extension\MagicObject', $e->getClassName(), 'Exception should be raised on the "Twig\Tests\Extension\MagicObject" class');
155+
$this->assertEquals('foo', $e->getPropertyName(), 'Exception should be raised on the "foo" property');
156+
}
157+
}
158+
159+
public function testSandboxUnallowedArrayIndexAccessor()
160+
{
161+
$twig = $this->getEnvironment(true, [], self::$templates);
162+
163+
// ArrayObject and other internal array-like classes are exempted from sandbox restrictions
164+
$this->assertSame('bar', $twig->load('1_array_like')->render(['array_like' => new \ArrayObject(['foo' => 'bar'])]));
165+
166+
try {
167+
$twig->load('1_array_like')->render(self::$params);
168+
$this->fail('Sandbox throws a SecurityError exception if an unallowed method is called');
169+
} catch (SecurityNotAllowedPropertyError $e) {
170+
$this->assertEquals('Twig\Tests\Extension\ArrayLikeObject', $e->getClassName(), 'Exception should be raised on the "Twig\Tests\Extension\ArrayLikeObject" class');
171+
$this->assertEquals('foo', $e->getPropertyName(), 'Exception should be raised on the "foo" property');
153172
}
154173
}
155174

@@ -315,7 +334,8 @@ public static function getSandboxAllowedToStringTests()
315334
return [
316335
'constant_test' => ['{{ obj is constant("PHP_INT_MAX") }}', ''],
317336
'set_object' => ['{% set a = obj.anotherFooObject %}{{ a.foo }}', 'foo'],
318-
'is_defined' => ['{{ obj.anotherFooObject is defined }}', '1'],
337+
'is_defined1' => ['{{ obj.anotherFooObject is defined }}', '1'],
338+
'is_defined2' => ['{{ magic.foo is defined }}', ''],
319339
'is_null' => ['{{ obj is null }}', ''],
320340
'is_sameas' => ['{{ obj is same as(obj) }}', '1'],
321341
'is_sameas_no_brackets' => ['{{ obj is same as obj }}', '1'],
@@ -625,3 +645,37 @@ public function getAnotherFooObject()
625645
return new self();
626646
}
627647
}
648+
649+
class ArrayLikeObject extends \ArrayObject
650+
{
651+
public function offsetExists($offset): bool
652+
{
653+
throw new \BadMethodCallException('Should not be called');
654+
}
655+
656+
public function offsetGet($offset): mixed
657+
{
658+
throw new \BadMethodCallException('Should not be called');
659+
}
660+
661+
public function offsetSet($offset, $value): void
662+
{
663+
}
664+
665+
public function offsetUnset($offset): void
666+
{
667+
}
668+
}
669+
670+
class MagicObject
671+
{
672+
public function __get($name): mixed
673+
{
674+
throw new \BadMethodCallException('Should not be called');
675+
}
676+
677+
public function __isset($name): bool
678+
{
679+
throw new \BadMethodCallException('Should not be called');
680+
}
681+
}

0 commit comments

Comments
 (0)