diff --git a/doc/api.rst b/doc/api.rst index 09c553175e1..d5e484aa87b 100644 --- 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. + .. caution:: The ``extends`` and ``use`` tags are always allowed in a sandboxed diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php index 3ed27a35cc3..e1fd3962e0e 100644 --- a/src/Extension/CoreExtension.php +++ b/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,6 +1765,10 @@ public static function getAttribute(Environment $env, Source $source, $object, $ return false; } + if ($propertyNotAllowedError) { + throw $propertyNotAllowedError; + } + if ($ignoreStrictCheck || !$env->isStrictVariables()) { return; } @@ -1740,12 +1776,24 @@ public static function getAttribute(Environment $env, Source $source, $object, $ 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 index 29a446b881b..2181b0f7862 100644 --- 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 index 59e68f67ec2..999483241ae 100644 --- a/tests/Extension/SandboxTest.php +++ b/tests/Extension/SandboxTest.php @@ -43,6 +43,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 = [ @@ -66,6 +68,7 @@ protected function setUp(): void '1_childobj_parentmethod' => '{{ child_obj.ParentMethod() }}', '1_childobj_childmethod' => '{{ child_obj.ChildMethod() }}', '1_empty' => '', + '1_array_like' => '{{ array_like["foo"] }}', ]; } @@ -141,15 +144,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'); } } @@ -300,7 +319,8 @@ public static 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'], @@ -610,3 +630,37 @@ public function getAnotherFooObject() return new self(); } } + +class ArrayLikeObject extends \ArrayObject +{ + public function offsetExists($offset): bool + { + throw new \BadMethodCallException('Should not be called'); + } + + public function offsetGet($offset): mixed + { + throw new \BadMethodCallException('Should not be called'); + } + + public function offsetSet($offset, $value): void + { + } + + public function offsetUnset($offset): void + { + } +} + +class MagicObject +{ + public function __get($name): mixed + { + throw new \BadMethodCallException('Should not be called'); + } + + public function __isset($name): bool + { + throw new \BadMethodCallException('Should not be called'); + } +}