Skip to content

Commit 831c148

Browse files
nicolas-grekasfabpot
nicolas-grekas
authored and
fabpot
committed
Sandbox ArrayAccess and do sandbox checks before isset() checks
1 parent 2bb8c24 commit 831c148

File tree

4 files changed

+153
-19
lines changed

4 files changed

+153
-19
lines changed

‎doc/api.rst‎

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

489+
.. note::
490+
491+
As of Twig 1.14.1 (and on Twig 3.11.2), if the ``Article`` class implements
492+
the ``ArrayAccess`` interface, the templates will only be able to access
493+
the ``title`` and ``body`` attributes.
494+
495+
Note that native array-like classes (like ``ArrayObject``) are always
496+
allowed, you don't need to configure them.
497+
489498
.. caution::
490499

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

‎src/Extension/CoreExtension.php‎

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@
6565
use Twig\Node\Node;
6666
use Twig\NodeVisitor\MacroAutoImportNodeVisitor;
6767
use Twig\Parser;
68+
use Twig\Sandbox\SecurityNotAllowedMethodError;
69+
use Twig\Sandbox\SecurityNotAllowedPropertyError;
6870
use Twig\Source;
6971
use Twig\Template;
7072
use Twig\TemplateWrapper;
@@ -92,6 +94,20 @@
9294

9395
final class CoreExtension extends AbstractExtension
9496
{
97+
public const ARRAY_LIKE_CLASSES = [
98+
'ArrayIterator',
99+
'ArrayObject',
100+
'CachingIterator',
101+
'RecursiveArrayIterator',
102+
'RecursiveCachingIterator',
103+
'SplDoublyLinkedList',
104+
'SplFixedArray',
105+
'SplObjectStorage',
106+
'SplQueue',
107+
'SplStack',
108+
'WeakMap',
109+
];
110+
95111
private $dateFormats = ['F j, Y H:i', '%d days'];
96112
private $numberFormat = [0, '.', ','];
97113
private $timezone = null;
@@ -1587,10 +1603,20 @@ public static function batch($items, $size, $fill = null, $preserveKeys = true):
15871603
*/
15881604
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)
15891605
{
1606+
$propertyNotAllowedError = null;
1607+
15901608
// array
15911609
if (Template::METHOD_CALL !== $type) {
15921610
$arrayItem = \is_bool($item) || \is_float($item) ? (int) $item : $item;
15931611

1612+
if ($sandboxed && $object instanceof \ArrayAccess && !\in_array($object::class, self::ARRAY_LIKE_CLASSES, true)) {
1613+
try {
1614+
$env->getExtension(SandboxExtension::class)->checkPropertyAllowed($object, $arrayItem, $lineno, $source);
1615+
} catch (SecurityNotAllowedPropertyError $propertyNotAllowedError) {
1616+
goto methodCheck;
1617+
}
1618+
}
1619+
15941620
if (((\is_array($object) || $object instanceof \ArrayObject) && (isset($object[$arrayItem]) || \array_key_exists($arrayItem, (array) $object)))
15951621
|| ($object instanceof \ArrayAccess && isset($object[$arrayItem]))
15961622
) {
@@ -1662,19 +1688,25 @@ public static function getAttribute(Environment $env, Source $source, $object, $
16621688

16631689
// object property
16641690
if (Template::METHOD_CALL !== $type) {
1691+
if ($sandboxed) {
1692+
try {
1693+
$env->getExtension(SandboxExtension::class)->checkPropertyAllowed($object, $item, $lineno, $source);
1694+
} catch (SecurityNotAllowedPropertyError $propertyNotAllowedError) {
1695+
goto methodCheck;
1696+
}
1697+
}
1698+
16651699
if (isset($object->$item) || \array_key_exists((string) $item, (array) $object)) {
16661700
if ($isDefinedTest) {
16671701
return true;
16681702
}
16691703

1670-
if ($sandboxed) {
1671-
$env->getExtension(SandboxExtension::class)->checkPropertyAllowed($object, $item, $lineno, $source);
1672-
}
1673-
16741704
return $object->$item;
16751705
}
16761706
}
16771707

1708+
methodCheck:
1709+
16781710
static $cache = [];
16791711

16801712
$class = \get_class($object);
@@ -1733,19 +1765,35 @@ public static function getAttribute(Environment $env, Source $source, $object, $
17331765
return false;
17341766
}
17351767

1768+
if ($propertyNotAllowedError) {
1769+
throw $propertyNotAllowedError;
1770+
}
1771+
17361772
if ($ignoreStrictCheck || !$env->isStrictVariables()) {
17371773
return;
17381774
}
17391775

17401776
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);
17411777
}
17421778

1743-
if ($isDefinedTest) {
1744-
return true;
1779+
if ($sandboxed) {
1780+
try {
1781+
$env->getExtension(SandboxExtension::class)->checkMethodAllowed($object, $method, $lineno, $source);
1782+
} catch (SecurityNotAllowedMethodError $e) {
1783+
if ($isDefinedTest) {
1784+
return false;
1785+
}
1786+
1787+
if ($propertyNotAllowedError) {
1788+
throw $propertyNotAllowedError;
1789+
}
1790+
1791+
throw $e;
1792+
}
17451793
}
17461794

1747-
if ($sandboxed) {
1748-
$env->getExtension(SandboxExtension::class)->checkMethodAllowed($object, $method, $lineno, $source);
1795+
if ($isDefinedTest) {
1796+
return true;
17491797
}
17501798

17511799
// 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
@@ -31,6 +31,7 @@ public function __construct(AbstractExpression $node, AbstractExpression $attrib
3131
public function compile(Compiler $compiler): void
3232
{
3333
$env = $compiler->getEnvironment();
34+
$arrayAccessSandbox = false;
3435

3536
// optimize array calls
3637
if (
@@ -44,17 +45,35 @@ public function compile(Compiler $compiler): void
4445
->raw('(('.$var.' = ')
4546
->subcompile($this->getNode('node'))
4647
->raw(') && is_array(')
47-
->raw($var)
48+
->raw($var);
49+
50+
if (!$env->hasExtension(SandboxExtension::class)) {
51+
$compiler
52+
->raw(') || ')
53+
->raw($var)
54+
->raw(' instanceof ArrayAccess ? (')
55+
->raw($var)
56+
->raw('[')
57+
->subcompile($this->getNode('attribute'))
58+
->raw('] ?? null) : null)')
59+
;
60+
61+
return;
62+
}
63+
64+
$arrayAccessSandbox = true;
65+
66+
$compiler
4867
->raw(') || ')
4968
->raw($var)
50-
->raw(' instanceof ArrayAccess ? (')
69+
->raw(' instanceof ArrayAccess && in_array(')
70+
->raw($var.'::class')
71+
->raw(', CoreExtension::ARRAY_LIKE_CLASSES, true) ? (')
5172
->raw($var)
5273
->raw('[')
5374
->subcompile($this->getNode('attribute'))
54-
->raw('] ?? null) : null)')
75+
->raw('] ?? null) : ')
5576
;
56-
57-
return;
5877
}
5978

6079
$compiler->raw('CoreExtension::getAttribute($this->env, $this->source, ');
@@ -83,5 +102,9 @@ public function compile(Compiler $compiler): void
83102
->raw(', ')->repr($this->getNode('node')->getTemplateLine())
84103
->raw(')')
85104
;
105+
106+
if ($arrayAccessSandbox) {
107+
$compiler->raw(')');
108+
}
86109
}
87110
}

‎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 = [
@@ -66,6 +68,7 @@ protected function setUp(): void
6668
'1_childobj_parentmethod' => '{{ child_obj.ParentMethod() }}',
6769
'1_childobj_childmethod' => '{{ child_obj.ChildMethod() }}',
6870
'1_empty' => '',
71+
'1_array_like' => '{{ array_like["foo"] }}',
6972
];
7073
}
7174

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

@@ -300,7 +319,8 @@ public static function getSandboxAllowedToStringTests()
300319
return [
301320
'constant_test' => ['{{ obj is constant("PHP_INT_MAX") }}', ''],
302321
'set_object' => ['{% set a = obj.anotherFooObject %}{{ a.foo }}', 'foo'],
303-
'is_defined' => ['{{ obj.anotherFooObject is defined }}', '1'],
322+
'is_defined1' => ['{{ obj.anotherFooObject is defined }}', '1'],
323+
'is_defined2' => ['{{ magic.foo is defined }}', ''],
304324
'is_null' => ['{{ obj is null }}', ''],
305325
'is_sameas' => ['{{ obj is same as(obj) }}', '1'],
306326
'is_sameas_no_brackets' => ['{{ obj is same as obj }}', '1'],
@@ -610,3 +630,37 @@ public function getAnotherFooObject()
610630
return new self();
611631
}
612632
}
633+
634+
class ArrayLikeObject extends \ArrayObject
635+
{
636+
public function offsetExists($offset): bool
637+
{
638+
throw new \BadMethodCallException('Should not be called');
639+
}
640+
641+
public function offsetGet($offset): mixed
642+
{
643+
throw new \BadMethodCallException('Should not be called');
644+
}
645+
646+
public function offsetSet($offset, $value): void
647+
{
648+
}
649+
650+
public function offsetUnset($offset): void
651+
{
652+
}
653+
}
654+
655+
class MagicObject
656+
{
657+
public function __get($name): mixed
658+
{
659+
throw new \BadMethodCallException('Should not be called');
660+
}
661+
662+
public function __isset($name): bool
663+
{
664+
throw new \BadMethodCallException('Should not be called');
665+
}
666+
}

0 commit comments

Comments
 (0)