-

CHANGELOG

@@ -1,3 +1,10 @@
+# 3.14.1 (2024-11-06)
+
+ * [BC BREAK] Fix a security issue in the sandbox mode allowing an attacker to call attributes on Array-like objects
+   They are now checked via the property policy
+ * Fix a security issue in the sandbox mode allowing an attacker to be able to call `toString()`
+   under some circumstances on an object even if the `__toString()` method is not allowed by the security policy
+
 # 3.14.0 (2024-09-09)
 
  * Fix a security issue when an included sandboxed template has been loaded before without the sandbox context

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.
+
 .. caution::
 
     The ``extends`` and ``use`` tags are always allowed in a sandboxed

src/Environment.php

@@ -43,11 +43,11 @@
  */
 class Environment
 {
-    public const VERSION = '3.14.0';
-    public const VERSION_ID = 31400;
+    public const VERSION = '3.14.1';
+    public const VERSION_ID = 31401;
     public const MAJOR_VERSION = 3;
     public const MINOR_VERSION = 14;
-    public const RELEASE_VERSION = 0;
+    public const RELEASE_VERSION = 1;
     public const EXTRA_VERSION = '';
 
     private $charset;

src/Extension/CoreExtension.php

@@ -65,6 +65,8 @@
 use Twig\Node\Node;
 use Twig\NodeVisitor\MacroAutoImportNodeVisitor;
 use Twig\Parser;
+use Twig\Sandbox\SecurityNotAllowedMethodError;
+use Twig\Sandbox\SecurityNotAllowedPropertyError;
 use Twig\Source;
 use Twig\Template;
 use Twig\TemplateWrapper;
@@ -92,6 +94,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;
@@ -1587,10 +1603,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, $isDefinedTest = false, $ignoreStrictCheck = false, $sandboxed = false, int $lineno = -1)
     {
+        $propertyNotAllowedError = null;
+
         // array
         if (Template::METHOD_CALL !== $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]))
             ) {
@@ -1662,19 +1688,25 @@ public static function getAttribute(Environment $env, Source $source, $object, $
 
         // object property
         if (Template::METHOD_CALL !== $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);
@@ -1733,19 +1765,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

src/Extension/SandboxExtension.php

@@ -119,6 +119,14 @@ public function checkPropertyAllowed($obj, $property, int $lineno = -1, ?Source
 
     public function ensureToStringAllowed($obj, int $lineno = -1, ?Source $source = null)
     {
+        if (\is_array($obj)) {
+            foreach ($obj as $v) {
+                $this->ensureToStringAllowed($v, $lineno, $source);
+            }
+
+            return $obj;
+        }
+
         if ($this->isSandboxed($source) && $obj instanceof \Stringable) {
             try {
                 $this->policy->checkMethodAllowed($obj, '__toString');

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(')');
+        }
     }
 }

src/NodeVisitor/SandboxNodeVisitor.php

@@ -15,12 +15,14 @@
 use Twig\Node\CheckSecurityCallNode;
 use Twig\Node\CheckSecurityNode;
 use Twig\Node\CheckToStringNode;
+use Twig\Node\Expression\ArrayExpression;
 use Twig\Node\Expression\Binary\ConcatBinary;
 use Twig\Node\Expression\Binary\RangeBinary;
 use Twig\Node\Expression\FilterExpression;
 use Twig\Node\Expression\FunctionExpression;
 use Twig\Node\Expression\GetAttrExpression;
 use Twig\Node\Expression\NameExpression;
+use Twig\Node\Expression\Unary\SpreadUnary;
 use Twig\Node\ModuleNode;
 use Twig\Node\Node;
 use Twig\Node\PrintNode;
@@ -120,7 +122,18 @@ private function wrapNode(Node $node, string $name): void
     {
         $expr = $node->getNode($name);
         if (($expr instanceof NameExpression || $expr instanceof GetAttrExpression) && !$expr->isGenerator()) {
-            $node->setNode($name, new CheckToStringNode($expr));
+            // Simplify in 4.0 as the spread attribute has been removed there
+            $new = new CheckToStringNode($expr);
+            if ($expr->hasAttribute('spread')) {
+                $new->setAttribute('spread', $expr->getAttribute('spread'));
+            }
+            $node->setNode($name, $new);
+        } elseif ($expr instanceof SpreadUnary) {
+            $this->wrapNode($expr, 'node');
+        } elseif ($expr instanceof ArrayExpression) {
+            foreach ($expr as $name => $_) {
+                $this->wrapNode($expr, $name);
+            }
         }
     }