From 41103dcdc2daab4c83cdd05b5b4fde5b7e41e635 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Mon, 9 Sep 2024 10:51:06 +0200 Subject: [PATCH 1/9] Fix a security issue when an included sandboxed template has been loaded before without the sandbox context --- src/Extension/CoreExtension.php | 11 ++++------ tests/Extension/CoreTest.php | 39 +++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php index 5ac80884a58..4b014b8df93 100644 --- a/src/Extension/CoreExtension.php +++ b/src/Extension/CoreExtension.php @@ -1400,13 +1400,6 @@ public static function include(Environment $env, $context, $template, $variables if (!$alreadySandboxed = $sandbox->isSandboxed()) { $sandbox->enableSandbox(); } - - foreach ((\is_array($template) ? $template : [$template]) as $name) { - // if a Template instance is passed, it might have been instantiated outside of a sandbox, check security - if ($name instanceof TemplateWrapper || $name instanceof Template) { - $name->unwrap()->checkSecurity(); - } - } } try { @@ -1419,6 +1412,10 @@ public static function include(Environment $env, $context, $template, $variables } } + if ($isSandboxed && $loaded) { + $loaded->unwrap()->checkSecurity(); + } + return $loaded ? $loaded->render($variables) : ''; } finally { if ($isSandboxed && !$alreadySandboxed) { diff --git a/tests/Extension/CoreTest.php b/tests/Extension/CoreTest.php index 31458628115..5b8268492a2 100644 --- a/tests/Extension/CoreTest.php +++ b/tests/Extension/CoreTest.php @@ -12,8 +12,13 @@ */ use PHPUnit\Framework\TestCase; +use Twig\Environment; use Twig\Error\RuntimeError; use Twig\Extension\CoreExtension; +use Twig\Extension\SandboxExtension; +use Twig\Loader\ArrayLoader; +use Twig\Sandbox\SecurityError; +use Twig\Sandbox\SecurityPolicy; class CoreTest extends TestCase { @@ -313,6 +318,40 @@ public function provideCompareCases() [1, 42, "\x00\x34\x32"], ]; } + + public function testSandboxedInclude() + { + $twig = new Environment(new ArrayLoader([ + 'index' => '{{ include("included", sandboxed=true) }}', + 'included' => '{{ "included"|e }}', + ])); + $policy = new SecurityPolicy(allowedFunctions: ['include']); + $sandbox = new SandboxExtension($policy, false); + $twig->addExtension($sandbox); + + // We expect a compile error + $this->expectException(SecurityError::class); + $twig->render('index'); + } + + public function testSandboxedIncludeWithPreloadedTemplate() + { + $twig = new Environment(new ArrayLoader([ + 'index' => '{{ include("included", sandboxed=true) }}', + 'included' => '{{ "included"|e }}', + ])); + $policy = new SecurityPolicy(allowedFunctions: ['include']); + $sandbox = new SandboxExtension($policy, false); + $twig->addExtension($sandbox); + + // The template is loaded without the sandbox enabled + // so, no compile error + $twig->load('included'); + + // We expect a runtime error + $this->expectException(SecurityError::class); + $twig->render('index'); + } } final class CoreTestIteratorAggregate implements \IteratorAggregate From ff063afc691e1cfda6714f1915ed766cb108d188 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Tue, 10 Sep 2024 12:38:17 +0200 Subject: [PATCH 2/9] Prepare the 3.11.1 release --- CHANGELOG | 4 ++++ src/Environment.php | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index dd776e083b8..55285d681cc 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,7 @@ +# 3.11.1 (2024-09-10) + + * Fix a security issue when an included sandboxed template has been loaded before without the sandbox context + # 3.11.0 (2024-08-08) * Add `Twig\Cache\ChainCache` and `Twig\Cache\ReadOnlyFilesystemCache` diff --git a/src/Environment.php b/src/Environment.php index 32b13135c69..e928e63955e 100644 --- a/src/Environment.php +++ b/src/Environment.php @@ -43,11 +43,11 @@ */ class Environment { - public const VERSION = '3.11.0'; - public const VERSION_ID = 301100; + public const VERSION = '3.11.1'; + public const VERSION_ID = 301101; public const MAJOR_VERSION = 4; public const MINOR_VERSION = 11; - public const RELEASE_VERSION = 0; + public const RELEASE_VERSION = 1; public const EXTRA_VERSION = ''; private $charset; From cafc608ece310e62a35a76f17e25c04ab9ed05cc Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Fri, 25 Oct 2024 11:04:18 +0200 Subject: [PATCH 3/9] Fix sandbox handling for __toString() --- src/Extension/SandboxExtension.php | 8 ++++++++ src/NodeVisitor/SandboxNodeVisitor.php | 15 ++++++++++++++- tests/Extension/SandboxTest.php | 15 +++++++++++++-- 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/Extension/SandboxExtension.php b/src/Extension/SandboxExtension.php index 921df287a44..95b6295aa21 100644 --- a/src/Extension/SandboxExtension.php +++ b/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) && \is_object($obj) && method_exists($obj, '__toString')) { try { $this->policy->checkMethodAllowed($obj, '__toString'); diff --git a/src/NodeVisitor/SandboxNodeVisitor.php b/src/NodeVisitor/SandboxNodeVisitor.php index 68020885e40..7cc1c2e9f66 100644 --- a/src/NodeVisitor/SandboxNodeVisitor.php +++ b/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); + } } } diff --git a/tests/Extension/SandboxTest.php b/tests/Extension/SandboxTest.php index cbe6175787b..fe1d68a919e 100644 --- a/tests/Extension/SandboxTest.php +++ b/tests/Extension/SandboxTest.php @@ -38,6 +38,7 @@ protected function setUp(): void 'obj' => new FooObject(), 'arr' => ['obj' => new FooObject()], 'child_obj' => new ChildClass(), + 'some_array' => [5, 6, 7, new FooObject()], ]; self::$templates = [ @@ -184,10 +185,10 @@ public function testSandboxUnallowedProperty() */ public function testSandboxUnallowedToString($template) { - $twig = $this->getEnvironment(true, [], ['index' => $template], [], ['upper'], ['Twig\Tests\Extension\FooObject' => 'getAnotherFooObject'], [], ['random']); + $twig = $this->getEnvironment(true, [], ['index' => $template], [], ['upper', 'join', 'replace'], ['Twig\Tests\Extension\FooObject' => 'getAnotherFooObject'], [], ['random']); try { $twig->load('index')->render(self::$params); - $this->fail('Sandbox throws a SecurityError exception if an unallowed method (__toString()) is called in the template'); + $this->fail('Sandbox throws a SecurityError exception if an unallowed method "__toString()" method is called in the template'); } catch (SecurityNotAllowedMethodError $e) { $this->assertEquals('Twig\Tests\Extension\FooObject', $e->getClassName(), 'Exception should be raised on the "Twig\Tests\Extension\FooObject" class'); $this->assertEquals('__tostring', $e->getMethodName(), 'Exception should be raised on the "__toString" method'); @@ -210,6 +211,16 @@ public function getSandboxUnallowedToStringTests() 'object_chain_and_function' => ['{{ random(obj.anotherFooObject) }}'], 'concat' => ['{{ obj ~ "" }}'], 'concat_again' => ['{{ "" ~ obj }}'], + 'object_in_arguments' => ['{{ "__toString"|replace({"__toString": obj}) }}'], + 'object_in_array' => ['{{ [12, "foo", obj]|join(", ") }}'], + 'object_in_array_var' => ['{{ some_array|join(", ") }}'], + 'object_in_array_nested' => ['{{ [12, "foo", [12, "foo", obj]]|join(", ") }}'], + 'object_in_array_var_nested' => ['{{ [12, "foo", some_array]|join(", ") }}'], + 'object_in_array_dynamic_key' => ['{{ {(obj): "foo"}|join(", ") }}'], + 'object_in_array_dynamic_key_nested' => ['{{ {"foo": { (obj): "foo" }}|join(", ") }}'], + 'context' => ['{{ _context|join(", ") }}'], + 'spread_array_operator' => ['{{ [1, 2, ...[5, 6, 7, obj]]|join(",") }}'], + 'spread_array_operator_var' => ['{{ [1, 2, ...some_array]|join(",") }}'], ]; } From ec39a9dccc5fb4eaaba55e5d79a6f84a8dd8b69d Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 29 Oct 2024 15:00:56 +0100 Subject: [PATCH 4/9] Sandbox ArrayAccess and do sandbox checks before isset() checks --- doc/api.rst | 9 +++ src/Extension/CoreExtension.php | 64 ++++++++++++++++++--- src/Node/Expression/GetAttrExpression.php | 33 +++++++++-- tests/Extension/SandboxTest.php | 68 +++++++++++++++++++++-- 4 files changed, 155 insertions(+), 19 deletions(-) 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'); + } +} From 8b5278227b18755e83533991d8647069d7b06438 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 6 Nov 2024 19:29:31 +0100 Subject: [PATCH 5/9] Update CHANGELOG --- CHANGELOG | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 55285d681cc..6e62956410b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,10 @@ +# 3.11.2 (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.11.1 (2024-09-10) * Fix a security issue when an included sandboxed template has been loaded before without the sandbox context From 94612e76021558023219f8c272d4e77a0b3c0e20 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 6 Nov 2024 19:30:53 +0100 Subject: [PATCH 6/9] Prepare the 3.11.2 release --- src/Environment.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Environment.php b/src/Environment.php index e928e63955e..84dd1d30323 100644 --- a/src/Environment.php +++ b/src/Environment.php @@ -43,11 +43,11 @@ */ class Environment { - public const VERSION = '3.11.1'; - public const VERSION_ID = 301101; + public const VERSION = '3.11.2'; + public const VERSION_ID = 301102; public const MAJOR_VERSION = 4; public const MINOR_VERSION = 11; - public const RELEASE_VERSION = 1; + public const RELEASE_VERSION = 2; public const EXTRA_VERSION = ''; private $charset; From 5b580ec1882b54c98cbd8c0f8a3ca5d1904db6b1 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 6 Nov 2024 19:39:38 +0100 Subject: [PATCH 7/9] Fix code --- src/Extension/CoreExtension.php | 2 +- src/Node/Expression/GetAttrExpression.php | 2 +- tests/Extension/CoreTest.php | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php index 1e769c6a2b6..b077e796304 100644 --- a/src/Extension/CoreExtension.php +++ b/src/Extension/CoreExtension.php @@ -1571,7 +1571,7 @@ public static function getAttribute(Environment $env, Source $source, $object, $ 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)) { + if ($sandboxed && $object instanceof \ArrayAccess && !\in_array(get_class($object), self::ARRAY_LIKE_CLASSES, true)) { try { $env->getExtension(SandboxExtension::class)->checkPropertyAllowed($object, $arrayItem, $lineno, $source); } catch (SecurityNotAllowedPropertyError $propertyNotAllowedError) { diff --git a/src/Node/Expression/GetAttrExpression.php b/src/Node/Expression/GetAttrExpression.php index 2181b0f7862..f54f2f09d54 100644 --- a/src/Node/Expression/GetAttrExpression.php +++ b/src/Node/Expression/GetAttrExpression.php @@ -67,7 +67,7 @@ public function compile(Compiler $compiler): void ->raw(') || ') ->raw($var) ->raw(' instanceof ArrayAccess && in_array(') - ->raw($var.'::class') + ->raw('get_class('.$var.')') ->raw(', CoreExtension::ARRAY_LIKE_CLASSES, true) ? (') ->raw($var) ->raw('[') diff --git a/tests/Extension/CoreTest.php b/tests/Extension/CoreTest.php index 5b8268492a2..0c397b6ec88 100644 --- a/tests/Extension/CoreTest.php +++ b/tests/Extension/CoreTest.php @@ -325,7 +325,7 @@ public function testSandboxedInclude() 'index' => '{{ include("included", sandboxed=true) }}', 'included' => '{{ "included"|e }}', ])); - $policy = new SecurityPolicy(allowedFunctions: ['include']); + $policy = new SecurityPolicy([], [], [], [], ['include']); $sandbox = new SandboxExtension($policy, false); $twig->addExtension($sandbox); @@ -340,7 +340,7 @@ public function testSandboxedIncludeWithPreloadedTemplate() 'index' => '{{ include("included", sandboxed=true) }}', 'included' => '{{ "included"|e }}', ])); - $policy = new SecurityPolicy(allowedFunctions: ['include']); + $policy = new SecurityPolicy([], [], [], [], ['include']); $sandbox = new SandboxExtension($policy, false); $twig->addExtension($sandbox); From a0f775683d289dd517781d70c5962916bc4f2113 Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Thu, 7 Nov 2024 11:49:11 +1000 Subject: [PATCH 8/9] Fix recursion when arrays contain self-references in sandboxed mode --- src/Extension/SandboxExtension.php | 11 ++++++++++- tests/Extension/SandboxTest.php | 12 ++++++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/Extension/SandboxExtension.php b/src/Extension/SandboxExtension.php index 95b6295aa21..9603f8e9751 100644 --- a/src/Extension/SandboxExtension.php +++ b/src/Extension/SandboxExtension.php @@ -26,11 +26,14 @@ final class SandboxExtension extends AbstractExtension private $policy; private $sourcePolicy; + static array $recursionProjection = []; + public function __construct(SecurityPolicyInterface $policy, $sandboxed = false, ?SourcePolicyInterface $sourcePolicy = null) { $this->policy = $policy; $this->sandboxedGlobally = $sandboxed; $this->sourcePolicy = $sourcePolicy; + static::$recursionProjection = []; } public function getTokenParsers(): array @@ -120,10 +123,16 @@ public function checkPropertyAllowed($obj, $property, int $lineno = -1, ?Source public function ensureToStringAllowed($obj, int $lineno = -1, ?Source $source = null) { if (\is_array($obj)) { + $hash = \hash('sha256', \serialize($obj)); + if (\array_key_exists($hash, static::$recursionProjection)) { + unset(static::$recursionProjection[$hash]); + return $obj; + } + static::$recursionProjection[$hash] = TRUE; foreach ($obj as $v) { $this->ensureToStringAllowed($v, $lineno, $source); } - + unset(static::$recursionProjection[$hash]); return $obj; } diff --git a/tests/Extension/SandboxTest.php b/tests/Extension/SandboxTest.php index cc74e8cd56e..647ea29534e 100644 --- a/tests/Extension/SandboxTest.php +++ b/tests/Extension/SandboxTest.php @@ -41,7 +41,10 @@ protected function setUp(): void 'some_array' => [5, 6, 7, new FooObject()], 'array_like' => new ArrayLikeObject(), 'magic' => new MagicObject(), + 'recursion' => [4], ]; + self::$params['recursion'][] = &self::$params['recursion']; + self::$params['recursion'][] = new FooObject(); self::$templates = [ '1_basic1' => '{{ obj.foo }}', @@ -240,6 +243,7 @@ public function getSandboxUnallowedToStringTests() 'context' => ['{{ _context|join(", ") }}'], 'spread_array_operator' => ['{{ [1, 2, ...[5, 6, 7, obj]]|join(",") }}'], 'spread_array_operator_var' => ['{{ [1, 2, ...some_array]|join(",") }}'], + 'recursion' => ['{{ recursion|join(", ") }}'], ]; } @@ -573,13 +577,13 @@ class ArrayLikeObject extends \ArrayObject { public function offsetExists($offset): bool { - throw new \BadMethodCallException('Should not be called'); + throw new \BadMethodCallException('Should not be called.'); } #[\ReturnTypeWillChange] public function offsetGet($offset) { - throw new \BadMethodCallException('Should not be called'); + throw new \BadMethodCallException('Should not be called.'); } public function offsetSet($offset, $value): void @@ -596,11 +600,11 @@ class MagicObject #[\ReturnTypeWillChange] public function __get($name) { - throw new \BadMethodCallException('Should not be called'); + throw new \BadMethodCallException('Should not be called.'); } public function __isset($name): bool { - throw new \BadMethodCallException('Should not be called'); + throw new \BadMethodCallException('Should not be called.'); } } From d3fc0741713b5610782ba9b36e840257d7041887 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Thu, 7 Nov 2024 09:40:47 +0100 Subject: [PATCH 9/9] Improve detection of recursion --- doc/api.rst | 2 +- src/Extension/SandboxExtension.php | 56 +++++++++++++++++++++++------- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/doc/api.rst b/doc/api.rst index d45af36278e..8da695037e7 100644 --- a/doc/api.rst +++ b/doc/api.rst @@ -488,7 +488,7 @@ 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 + As of Twig 3.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. diff --git a/src/Extension/SandboxExtension.php b/src/Extension/SandboxExtension.php index 9603f8e9751..255f2f28dea 100644 --- a/src/Extension/SandboxExtension.php +++ b/src/Extension/SandboxExtension.php @@ -26,14 +26,11 @@ final class SandboxExtension extends AbstractExtension private $policy; private $sourcePolicy; - static array $recursionProjection = []; - public function __construct(SecurityPolicyInterface $policy, $sandboxed = false, ?SourcePolicyInterface $sourcePolicy = null) { $this->policy = $policy; $this->sandboxedGlobally = $sandboxed; $this->sourcePolicy = $sourcePolicy; - static::$recursionProjection = []; } public function getTokenParsers(): array @@ -123,16 +120,8 @@ public function checkPropertyAllowed($obj, $property, int $lineno = -1, ?Source public function ensureToStringAllowed($obj, int $lineno = -1, ?Source $source = null) { if (\is_array($obj)) { - $hash = \hash('sha256', \serialize($obj)); - if (\array_key_exists($hash, static::$recursionProjection)) { - unset(static::$recursionProjection[$hash]); - return $obj; - } - static::$recursionProjection[$hash] = TRUE; - foreach ($obj as $v) { - $this->ensureToStringAllowed($v, $lineno, $source); - } - unset(static::$recursionProjection[$hash]); + $this->ensureToStringAllowedForArray($obj, $lineno, $source); + return $obj; } @@ -149,4 +138,45 @@ public function ensureToStringAllowed($obj, int $lineno = -1, ?Source $source = return $obj; } + + private function ensureToStringAllowedForArray(array $obj, int $lineno, ?Source $source, array &$stack = []): void + { + foreach ($obj as $k => $v) { + if (!$v) { + continue; + } + + if (!\is_array($v)) { + $this->ensureToStringAllowed($v, $lineno, $source); + continue; + } + + if (\PHP_VERSION_ID < 70400) { + static $cookie; + + if ($v === $cookie ?? $cookie = new \stdClass()) { + continue; + } + + $obj[$k] = $cookie; + try { + $this->ensureToStringAllowedForArray($v, $lineno, $source, $stack); + } finally { + $obj[$k] = $v; + } + + continue; + } + + if ($r = \ReflectionReference::fromArrayElement($obj, $k)) { + if (isset($stack[$r->getId()])) { + continue; + } + + $stack[$r->getId()] = true; + } + + $this->ensureToStringAllowedForArray($v, $lineno, $source, $stack); + } + } }