Sandbox ArrayAccess and do sandbox checks before isset() checks

nicolas-grekas · fabpot · commit ec39a9dccc5f · 2024-11-06T19:48:59.000+01:00
diff --git a/doc/api.rst b/doc/api.rst
@@ -486,6 +486,15 @@ able to call the ``getTitle()`` and ``getBody()`` methods on ``Article``
 objects, and the ``title`` and ``body`` public properties. Everything else
 won't be allowed and will generate a ``\Twig\Sandbox\SecurityError`` exception.
 
+.. note::
+
+    As of Twig 1.14.1 (and on Twig 3.11.2), if the ``Article`` class implements
+    the ``ArrayAccess`` interface, the templates will only be able to access
+    the ``title`` and ``body`` attributes.
+
+    Note that native array-like classes (like ``ArrayObject``) are always
+    allowed, you don't need to configure them.
+
 The policy object is the first argument of the sandbox constructor::
 
     $sandbox = new \Twig\Extension\SandboxExtension($policy);
diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php
@@ -57,6 +57,8 @@
 use Twig\Node\Expression\Unary\NotUnary;
 use Twig\Node\Expression\Unary\PosUnary;
 use Twig\NodeVisitor\MacroAutoImportNodeVisitor;
+use Twig\Sandbox\SecurityNotAllowedMethodError;
+use Twig\Sandbox\SecurityNotAllowedPropertyError;
 use Twig\Source;
 use Twig\Template;
 use Twig\TemplateWrapper;
@@ -82,6 +84,20 @@
 
 final class CoreExtension extends AbstractExtension
 {
+    public const ARRAY_LIKE_CLASSES = [
+        'ArrayIterator',
+        'ArrayObject',
+        'CachingIterator',
+        'RecursiveArrayIterator',
+        'RecursiveCachingIterator',
+        'SplDoublyLinkedList',
+        'SplFixedArray',
+        'SplObjectStorage',
+        'SplQueue',
+        'SplStack',
+        'WeakMap',
+    ];
+
     private $dateFormats = ['F j, Y H:i', '%d days'];
     private $numberFormat = [0, '.', ','];
     private $timezone = null;
@@ -1549,10 +1565,20 @@ public static function batch($items, $size, $fill = null, $preserveKeys = true):
      */
     public static function getAttribute(Environment $env, Source $source, $object, $item, array $arguments = [], $type = /* Template::ANY_CALL */ 'any', $isDefinedTest = false, $ignoreStrictCheck = false, $sandboxed = false, int $lineno = -1)
     {
+        $propertyNotAllowedError = null;
+
         // array
         if (/* Template::METHOD_CALL */ 'method' !== $type) {
             $arrayItem = \is_bool($item) || \is_float($item) ? (int) $item : $item;
 
+            if ($sandboxed && $object instanceof \ArrayAccess && !\in_array($object::class, self::ARRAY_LIKE_CLASSES, true)) {
+                try {
+                    $env->getExtension(SandboxExtension::class)->checkPropertyAllowed($object, $arrayItem, $lineno, $source);
+                } catch (SecurityNotAllowedPropertyError $propertyNotAllowedError) {
+                    goto methodCheck;
+                }
+            }
+
             if (((\is_array($object) || $object instanceof \ArrayObject) && (isset($object[$arrayItem]) || \array_key_exists($arrayItem, (array) $object)))
                 || ($object instanceof \ArrayAccess && isset($object[$arrayItem]))
             ) {
@@ -1624,19 +1650,25 @@ public static function getAttribute(Environment $env, Source $source, $object, $
 
         // object property
         if (/* Template::METHOD_CALL */ 'method' !== $type) {
+            if ($sandboxed) {
+                try {
+                    $env->getExtension(SandboxExtension::class)->checkPropertyAllowed($object, $item, $lineno, $source);
+                } catch (SecurityNotAllowedPropertyError $propertyNotAllowedError) {
+                    goto methodCheck;
+                }
+            }
+
             if (isset($object->$item) || \array_key_exists((string) $item, (array) $object)) {
                 if ($isDefinedTest) {
                     return true;
                 }
 
-                if ($sandboxed) {
-                    $env->getExtension(SandboxExtension::class)->checkPropertyAllowed($object, $item, $lineno, $source);
-                }
-
                 return $object->$item;
             }
         }
 
+        methodCheck:
+
         static $cache = [];
 
         $class = \get_class($object);
@@ -1695,19 +1727,35 @@ public static function getAttribute(Environment $env, Source $source, $object, $
                 return false;
             }
 
+            if ($propertyNotAllowedError) {
+                throw $propertyNotAllowedError;
+            }
+
             if ($ignoreStrictCheck || !$env->isStrictVariables()) {
                 return;
             }
 
             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);
         }
 
-        if ($isDefinedTest) {
-            return true;
+        if ($sandboxed) {
+            try {
+                $env->getExtension(SandboxExtension::class)->checkMethodAllowed($object, $method, $lineno, $source);
+            } catch (SecurityNotAllowedMethodError $e) {
+                if ($isDefinedTest) {
+                    return false;
+                }
+
+                if ($propertyNotAllowedError) {
+                    throw $propertyNotAllowedError;
+                }
+
+                throw $e;
+            }
         }
 
-        if ($sandboxed) {
-            $env->getExtension(SandboxExtension::class)->checkMethodAllowed($object, $method, $lineno, $source);
+        if ($isDefinedTest) {
+            return true;
         }
 
         // Some objects throw exceptions when they have __call, and the method we try
diff --git a/src/Node/Expression/GetAttrExpression.php b/src/Node/Expression/GetAttrExpression.php
@@ -31,6 +31,7 @@ public function __construct(AbstractExpression $node, AbstractExpression $attrib
     public function compile(Compiler $compiler): void
     {
         $env = $compiler->getEnvironment();
+        $arrayAccessSandbox = false;
 
         // optimize array calls
         if (
@@ -44,17 +45,35 @@ public function compile(Compiler $compiler): void
                 ->raw('(('.$var.' = ')
                 ->subcompile($this->getNode('node'))
                 ->raw(') && is_array(')
-                ->raw($var)
+                ->raw($var);
+
+            if (!$env->hasExtension(SandboxExtension::class)) {
+                $compiler
+                    ->raw(') || ')
+                    ->raw($var)
+                    ->raw(' instanceof ArrayAccess ? (')
+                    ->raw($var)
+                    ->raw('[')
+                    ->subcompile($this->getNode('attribute'))
+                    ->raw('] ?? null) : null)')
+                ;
+
+                return;
+            }
+
+            $arrayAccessSandbox = true;
+
+            $compiler
                 ->raw(') || ')
                 ->raw($var)
-                ->raw(' instanceof ArrayAccess ? (')
+                ->raw(' instanceof ArrayAccess && in_array(')
+                ->raw($var.'::class')
+                ->raw(', CoreExtension::ARRAY_LIKE_CLASSES, true) ? (')
                 ->raw($var)
                 ->raw('[')
                 ->subcompile($this->getNode('attribute'))
-                ->raw('] ?? null) : null)')
+                ->raw('] ?? null) : ')
             ;
-
-            return;
         }
 
         $compiler->raw('CoreExtension::getAttribute($this->env, $this->source, ');
@@ -83,5 +102,9 @@ public function compile(Compiler $compiler): void
             ->raw(', ')->repr($this->getNode('node')->getTemplateLine())
             ->raw(')')
         ;
+
+        if ($arrayAccessSandbox) {
+            $compiler->raw(')');
+        }
     }
 }
diff --git a/tests/Extension/SandboxTest.php b/tests/Extension/SandboxTest.php
@@ -39,6 +39,8 @@ protected function setUp(): void
             'arr' => ['obj' => new FooObject()],
             'child_obj' => new ChildClass(),
             'some_array' => [5, 6, 7, new FooObject()],
+            'array_like' => new ArrayLikeObject(),
+            'magic' => new MagicObject(),
         ];
 
         self::$templates = [
@@ -61,6 +63,7 @@ protected function setUp(): void
             '1_syntax_error' => '{% syntax error }}',
             '1_childobj_parentmethod' => '{{ child_obj.ParentMethod() }}',
             '1_childobj_childmethod' => '{{ child_obj.ChildMethod() }}',
+            '1_array_like' => '{{ array_like["foo"] }}',
         ];
     }
 
@@ -79,15 +82,31 @@ public function testSandboxGloballySet()
         $this->assertEquals('FOO', $twig->load('1_basic')->render(self::$params), 'Sandbox does nothing if it is disabled globally');
     }
 
-    public function testSandboxUnallowedMethodAccessor()
+    public function testSandboxUnallowedPropertyAccessor()
     {
         $twig = $this->getEnvironment(true, [], self::$templates);
         try {
-            $twig->load('1_basic1')->render(self::$params);
+            $twig->load('1_basic1')->render(['obj' => new MagicObject()]);
             $this->fail('Sandbox throws a SecurityError exception if an unallowed method is called');
-        } catch (SecurityNotAllowedMethodError $e) {
-            $this->assertEquals('Twig\Tests\Extension\FooObject', $e->getClassName(), 'Exception should be raised on the "Twig\Tests\Extension\FooObject" class');
-            $this->assertEquals('foo', $e->getMethodName(), 'Exception should be raised on the "foo" method');
+        } catch (SecurityNotAllowedPropertyError $e) {
+            $this->assertEquals('Twig\Tests\Extension\MagicObject', $e->getClassName(), 'Exception should be raised on the "Twig\Tests\Extension\MagicObject" class');
+            $this->assertEquals('foo', $e->getPropertyName(), 'Exception should be raised on the "foo" property');
+        }
+    }
+
+    public function testSandboxUnallowedArrayIndexAccessor()
+    {
+        $twig = $this->getEnvironment(true, [], self::$templates);
+
+        // ArrayObject and other internal array-like classes are exempted from sandbox restrictions
+        $this->assertSame('bar', $twig->load('1_array_like')->render(['array_like' => new \ArrayObject(['foo' => 'bar'])]));
+
+        try {
+            $twig->load('1_array_like')->render(self::$params);
+            $this->fail('Sandbox throws a SecurityError exception if an unallowed method is called');
+        } catch (SecurityNotAllowedPropertyError $e) {
+            $this->assertEquals('Twig\Tests\Extension\ArrayLikeObject', $e->getClassName(), 'Exception should be raised on the "Twig\Tests\Extension\ArrayLikeObject" class');
+            $this->assertEquals('foo', $e->getPropertyName(), 'Exception should be raised on the "foo" property');
         }
     }
 
@@ -238,7 +257,8 @@ 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_defined1' => ['{{ obj.anotherFooObject is defined }}', '1'],
+            'is_defined2' => ['{{ magic.foo is defined }}', ''],
             'is_null' => ['{{ obj is null }}', ''],
             'is_sameas' => ['{{ obj is same as(obj) }}', '1'],
             'is_sameas_no_brackets' => ['{{ obj is same as obj }}', '1'],
@@ -548,3 +568,39 @@ public function getAnotherFooObject()
         return new self();
     }
 }
+
+class ArrayLikeObject extends \ArrayObject
+{
+    public function offsetExists($offset): bool
+    {
+        throw new \BadMethodCallException('Should not be called');
+    }
+
+    #[\ReturnTypeWillChange]
+    public function offsetGet($offset)
+    {
+        throw new \BadMethodCallException('Should not be called');
+    }
+
+    public function offsetSet($offset, $value): void
+    {
+    }
+
+    public function offsetUnset($offset): void
+    {
+    }
+}
+
+class MagicObject
+{
+    #[\ReturnTypeWillChange]
+    public function __get($name)
+    {
+        throw new \BadMethodCallException('Should not be called');
+    }
+
+    public function __isset($name): bool
+    {
+        throw new \BadMethodCallException('Should not be called');
+    }
+}
