diff --git a/doc/api.rst b/doc/api.rst index 219bdec3395..d45af36278e 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. + 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 index 4b014b8df93..1e769c6a2b6 100644 --- 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,6 +1727,10 @@ public static function getAttribute(Environment $env, Source $source, $object, $ return false; } + if ($propertyNotAllowedError) { + throw $propertyNotAllowedError; + } + if ($ignoreStrictCheck || !$env->isStrictVariables()) { return; } @@ -1702,12 +1738,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 fe1d68a919e..cc74e8cd56e 100644 --- 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'); + } +}