From ccacf25ff225030c0fdf683511981fd36c9f0b63 Mon Sep 17 00:00:00 2001 From: Yaakov Saxon Date: Fri, 27 Oct 2023 12:01:57 -0400 Subject: [PATCH 1/8] initial commit --- src/Sandbox/MemberMatcher.php | 70 ++++++++++++++++++++++++++++++++++ src/Sandbox/SecurityPolicy.php | 31 +++------------ 2 files changed, 76 insertions(+), 25 deletions(-) create mode 100644 src/Sandbox/MemberMatcher.php diff --git a/src/Sandbox/MemberMatcher.php b/src/Sandbox/MemberMatcher.php new file mode 100644 index 00000000000..16b2218e533 --- /dev/null +++ b/src/Sandbox/MemberMatcher.php @@ -0,0 +1,70 @@ + + */ +final class MemberMatcher +{ + private $allowedMethods; + private $cache = []; + + public function __construct($allowedMethods) + { + foreach ($allowedMethods as $class => $methods) { + foreach ($methods as $index => $method) { + $allowedMethods[$class][$index] = strtolower($method); + } + } + $this->allowedMethods = $allowedMethods; + } + + + public function isAllowed($obj, $method) + { + $cacheKey = get_class($obj) . "::" . $method; + + // Check cache first + if (isset($this->cache[$cacheKey])) { + return $this->cache[$cacheKey]; + } + + $method = strtolower($method); // normalize method name + + foreach ($this->allowedMethods as $class => $methods) { + if ($class === '*' || $obj instanceof $class) { + foreach ($methods as $allowedMethod) { + if ($allowedMethod === '*') { + $this->cache[$cacheKey] = true; + return true; + } + if ($allowedMethod === $method) { + $this->cache[$cacheKey] = true; + return true; + } + //if allowedMethod ends with a *, check if the method starts with the allowedMethod + if (substr($allowedMethod, -1) === '*' && substr($method, 0, strlen($allowedMethod) - 1) === rtrim($allowedMethod, '*')) { + $this->cache[$cacheKey] = true; + return true; + } + } + } + } + + // If we reach here, the method is not allowed + $this->cache[$cacheKey] = false; + return false; + } +} diff --git a/src/Sandbox/SecurityPolicy.php b/src/Sandbox/SecurityPolicy.php index 3b79a870f83..b62ce47c4e0 100644 --- a/src/Sandbox/SecurityPolicy.php +++ b/src/Sandbox/SecurityPolicy.php @@ -32,7 +32,7 @@ public function __construct(array $allowedTags = [], array $allowedFilters = [], $this->allowedTags = $allowedTags; $this->allowedFilters = $allowedFilters; $this->setAllowedMethods($allowedMethods); - $this->allowedProperties = $allowedProperties; + $this->setAllowedProperties($allowedProperties); $this->allowedFunctions = $allowedFunctions; } @@ -48,15 +48,12 @@ public function setAllowedFilters(array $filters) public function setAllowedMethods(array $methods) { - $this->allowedMethods = []; - foreach ($methods as $class => $m) { - $this->allowedMethods[$class] = array_map(function ($value) { return strtr($value, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz'); }, \is_array($m) ? $m : [$m]); - } + $this->allowedMethods = new MemberMatcher($methods); } public function setAllowedProperties(array $properties) { - $this->allowedProperties = $properties; + $this->allowedProperties = new MemberMatcher($properties); } public function setAllowedFunctions(array $functions) @@ -91,32 +88,16 @@ public function checkMethodAllowed($obj, $method) return; } - $allowed = false; - $method = strtr($method, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz'); - foreach ($this->allowedMethods as $class => $methods) { - if ($obj instanceof $class && \in_array($method, $methods)) { - $allowed = true; - break; - } - } - - if (!$allowed) { + if (!$this->allowedMethods->isAllowed($obj, $method)) { $class = \get_class($obj); + $method = strtr($method, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz'); throw new SecurityNotAllowedMethodError(sprintf('Calling "%s" method on a "%s" object is not allowed.', $method, $class), $class, $method); } } public function checkPropertyAllowed($obj, $property) { - $allowed = false; - foreach ($this->allowedProperties as $class => $properties) { - if ($obj instanceof $class && \in_array($property, \is_array($properties) ? $properties : [$properties])) { - $allowed = true; - break; - } - } - - if (!$allowed) { + if (!$this->allowedProperties->isAllowed($obj, $property)) { $class = \get_class($obj); throw new SecurityNotAllowedPropertyError(sprintf('Calling "%s" property on a "%s" object is not allowed.', $property, $class), $class, $property); } From bc44bb21f4b4e06078c0d5574b1069e04bc9929a Mon Sep 17 00:00:00 2001 From: Yaakov Saxon Date: Fri, 27 Oct 2023 13:21:59 -0400 Subject: [PATCH 2/8] add type hints --- src/Sandbox/MemberMatcher.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Sandbox/MemberMatcher.php b/src/Sandbox/MemberMatcher.php index 16b2218e533..6d8d365fdd3 100644 --- a/src/Sandbox/MemberMatcher.php +++ b/src/Sandbox/MemberMatcher.php @@ -21,7 +21,7 @@ final class MemberMatcher private $allowedMethods; private $cache = []; - public function __construct($allowedMethods) + public function __construct(array $allowedMethods) { foreach ($allowedMethods as $class => $methods) { foreach ($methods as $index => $method) { @@ -32,7 +32,7 @@ public function __construct($allowedMethods) } - public function isAllowed($obj, $method) + public function isAllowed($obj, string $method): bool { $cacheKey = get_class($obj) . "::" . $method; From e84974fa51db6e5cc671bebbac31798745fb379b Mon Sep 17 00:00:00 2001 From: Yaakov Saxon Date: Fri, 27 Oct 2023 13:30:35 -0400 Subject: [PATCH 3/8] fix behavior when allowedMethods keypair value is a string instead of an array --- src/Sandbox/MemberMatcher.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Sandbox/MemberMatcher.php b/src/Sandbox/MemberMatcher.php index 6d8d365fdd3..432167b8294 100644 --- a/src/Sandbox/MemberMatcher.php +++ b/src/Sandbox/MemberMatcher.php @@ -23,12 +23,16 @@ final class MemberMatcher public function __construct(array $allowedMethods) { + $normalizedMethods = []; foreach ($allowedMethods as $class => $methods) { - foreach ($methods as $index => $method) { - $allowedMethods[$class][$index] = strtolower($method); + if (!is_array($methods)) { + $normalizedMethods[$class][] = strtolower($methods); + } + else foreach ($methods as $index => $method) { + $normalizedMethods[$class][$index] = strtolower($method); } } - $this->allowedMethods = $allowedMethods; + $this->allowedMethods = $normalizedMethods; } From 868fb9411d4ca499930e6649f5f7df432b1d3c9b Mon Sep 17 00:00:00 2001 From: Yaakov Saxon Date: Fri, 27 Oct 2023 13:32:48 -0400 Subject: [PATCH 4/8] rename method->member to denote either method or property --- src/Sandbox/MemberMatcher.php | 40 +++++++++++++++++------------------ 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/Sandbox/MemberMatcher.php b/src/Sandbox/MemberMatcher.php index 432167b8294..08be92b3b96 100644 --- a/src/Sandbox/MemberMatcher.php +++ b/src/Sandbox/MemberMatcher.php @@ -12,54 +12,54 @@ namespace Twig\Sandbox; /** - * Allows for flexible wildcard supported method and property matching in Security Policies. + * Allows for flexible wildcard supported member and property matching in Security Policies. * * @author Yaakov Saxon */ final class MemberMatcher { - private $allowedMethods; + private $allowedMembers; private $cache = []; - public function __construct(array $allowedMethods) + public function __construct(array $allowedMembers) { - $normalizedMethods = []; - foreach ($allowedMethods as $class => $methods) { - if (!is_array($methods)) { - $normalizedMethods[$class][] = strtolower($methods); + $normalizedMembers = []; + foreach ($allowedMembers as $class => $members) { + if (!is_array($members)) { + $normalizedMembers[$class][] = strtolower($members); } - else foreach ($methods as $index => $method) { - $normalizedMethods[$class][$index] = strtolower($method); + else foreach ($members as $index => $member) { + $normalizedMembers[$class][$index] = strtolower($member); } } - $this->allowedMethods = $normalizedMethods; + $this->allowedMembers = $normalizedMembers; } - public function isAllowed($obj, string $method): bool + public function isAllowed($obj, string $member): bool { - $cacheKey = get_class($obj) . "::" . $method; + $cacheKey = get_class($obj) . "::" . $member; // Check cache first if (isset($this->cache[$cacheKey])) { return $this->cache[$cacheKey]; } - $method = strtolower($method); // normalize method name + $member = strtolower($member); // normalize member name - foreach ($this->allowedMethods as $class => $methods) { + foreach ($this->allowedMembers as $class => $members) { if ($class === '*' || $obj instanceof $class) { - foreach ($methods as $allowedMethod) { - if ($allowedMethod === '*') { + foreach ($members as $allowedMember) { + if ($allowedMember === '*') { $this->cache[$cacheKey] = true; return true; } - if ($allowedMethod === $method) { + if ($allowedMember === $member) { $this->cache[$cacheKey] = true; return true; } - //if allowedMethod ends with a *, check if the method starts with the allowedMethod - if (substr($allowedMethod, -1) === '*' && substr($method, 0, strlen($allowedMethod) - 1) === rtrim($allowedMethod, '*')) { + //if allowedMember ends with a *, check if the member starts with the allowedMember + if (substr($allowedMember, -1) === '*' && substr($member, 0, strlen($allowedMember) - 1) === rtrim($allowedMember, '*')) { $this->cache[$cacheKey] = true; return true; } @@ -67,7 +67,7 @@ public function isAllowed($obj, string $method): bool } } - // If we reach here, the method is not allowed + // If we reach here, the member is not allowed $this->cache[$cacheKey] = false; return false; } From 97d3992219c605efacd9694dd192861553446cfd Mon Sep 17 00:00:00 2001 From: Yaakov Saxon Date: Fri, 27 Oct 2023 13:36:01 -0400 Subject: [PATCH 5/8] Not necessary to cache rejected members as false, as this should be rare and would throw an error immediately anyways --- src/Sandbox/MemberMatcher.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Sandbox/MemberMatcher.php b/src/Sandbox/MemberMatcher.php index 08be92b3b96..4e788b41da0 100644 --- a/src/Sandbox/MemberMatcher.php +++ b/src/Sandbox/MemberMatcher.php @@ -42,7 +42,7 @@ public function isAllowed($obj, string $member): bool // Check cache first if (isset($this->cache[$cacheKey])) { - return $this->cache[$cacheKey]; + return true; } $member = strtolower($member); // normalize member name @@ -68,7 +68,6 @@ public function isAllowed($obj, string $member): bool } // If we reach here, the member is not allowed - $this->cache[$cacheKey] = false; return false; } } From 34897655ed450a758b730be4d34275ed887c3a28 Mon Sep 17 00:00:00 2001 From: Yaakov Saxon Date: Fri, 27 Oct 2023 14:32:48 -0400 Subject: [PATCH 6/8] add tests for various permutations of wildcards --- tests/Extension/SandboxTest.php | 81 +++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/tests/Extension/SandboxTest.php b/tests/Extension/SandboxTest.php index dff2ddc8dd4..8fbf062844e 100644 --- a/tests/Extension/SandboxTest.php +++ b/tests/Extension/SandboxTest.php @@ -449,6 +449,87 @@ public function testMultipleClassMatchesViaInheritanceInAllowedMethods() } } + public function testSandboxAllowedWildcardMethod() + { + $allowedMethods = ['Twig\Tests\Extension\FooObject' => ['*']]; + $twig = $this->getEnvironment(true, [], self::$templates, [], [], $allowedMethods); + + // This should pass without throwing an exception + $result = $twig->load('1_basic1')->render(self::$params); + $this->assertEquals('foo', $result); + $result = $twig->load('1_basic8')->render(self::$params); + $this->assertEquals('foobarfoobar', $result); + + } + + public function testSandboxAllowedWildcardSuffixMethod() + { + $allowedMethods = ['Twig\Tests\Extension\FooObject' => ['get*']]; + $twig = $this->getEnvironment(true, [], self::$templates, [], [], $allowedMethods); + + // This should pass without throwing an exception + $result = $twig->load('1_basic8')->render(self::$params); + $this->assertEquals('foobarfoobar', $result); + } + + public function testSandboxUnallowedPrefixWildcardSuffixMethod() + { + $allowedMethods = ['Twig\Tests\Extension\FooObject' => ['get*']]; + $twig = $this->getEnvironment(true, [], self::$templates, [], [], $allowedMethods); + + try { + $twig->load('1_basic1')->render(self::$params); + $this->fail('Sandbox should block method not matching method prefix'); + } catch (SecurityError $e) { + $this->assertInstanceOf(SecurityNotAllowedMethodError::class, $e); + $this->assertEquals('foo', $e->getMethodName()); + } + } + + public function testSandboxAllowedWildcardClassMethod() + { + $allowedMethods = ['*' => ['foo']]; + $twig = $this->getEnvironment(true, [], self::$templates, [], [], $allowedMethods); + + // This should pass without throwing an exception + $result = $twig->load('1_basic1')->render(self::$params); + $this->assertEquals('foo', $result); + } + + public function testSandboxUnallowedWildcardClassMethod() + { + $allowedMethods = ['*' => ['foo']]; + $twig = $this->getEnvironment(true, [], self::$templates, [], [], $allowedMethods); + + try { + $twig->load('1_basic8')->render(self::$params); + $this->fail('Sandbox should block non matching method despite class wildcard'); + } catch (SecurityError $e) { + $this->assertInstanceOf(SecurityNotAllowedMethodError::class, $e); + $this->assertEquals('getfoobar', $e->getMethodName()); + } + } + + public function testSandboxAllowedWildcardClassWildcardSuffixMethod() + { + $allowedMethods = ['*' => ['get*']]; + $twig = $this->getEnvironment(true, [], self::$templates, [], [], $allowedMethods); + + // This should pass without throwing an exception + $result = $twig->load('1_basic8')->render(self::$params); + $this->assertEquals('foobarfoobar', $result); + } + + public function testSandboxAllowedWildcardClassWildcardSuffixProperty() + { + $allowedProperties = ['*' => ['b*']]; + $twig = $this->getEnvironment(true, [], self::$templates, [], [], [], $allowedProperties); + + // This should pass without throwing an exception + $result = $twig->load('1_basic4')->render(self::$params); + $this->assertEquals('bar', $result); + } + protected function getEnvironment($sandboxed, $options, $templates, $tags = [], $filters = [], $methods = [], $properties = [], $functions = []) { $loader = new ArrayLoader($templates); From c7b2c142f640f236e0693186495aa23d2994c1f8 Mon Sep 17 00:00:00 2001 From: Yaakov Saxon Date: Fri, 27 Oct 2023 14:42:34 -0400 Subject: [PATCH 7/8] improve class docstring --- src/Sandbox/MemberMatcher.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Sandbox/MemberMatcher.php b/src/Sandbox/MemberMatcher.php index 4e788b41da0..2b87851597d 100644 --- a/src/Sandbox/MemberMatcher.php +++ b/src/Sandbox/MemberMatcher.php @@ -12,8 +12,11 @@ namespace Twig\Sandbox; /** - * Allows for flexible wildcard supported member and property matching in Security Policies. - * + * Allows for flexible wildcard support in allowedMethods and allowedProperties in SecurityPolicy. + * - Class can be specified as wildcard `* => [...]` in order to allow those methods/properties for all classes. + * - Method/property can be specified as wildcard eg. `\DateTime => '*'` in order to allow all methods/properties for that class. + * - Method/property can also be specified with a trailing wildcard to allow all methods/properties with a certain prefix, eg. `\DateTime => ['get*', ...]` in order to allow all methods/properties that start with `get`. + * * @author Yaakov Saxon */ final class MemberMatcher From 2ee0fad6ef713c32baf959f94616afdcd6e55674 Mon Sep 17 00:00:00 2001 From: Yaakov Saxon Date: Mon, 30 Oct 2023 15:17:14 -0400 Subject: [PATCH 8/8] minor fabbot changes --- src/Sandbox/MemberMatcher.php | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/Sandbox/MemberMatcher.php b/src/Sandbox/MemberMatcher.php index 2b87851597d..ae9fc4c942e 100644 --- a/src/Sandbox/MemberMatcher.php +++ b/src/Sandbox/MemberMatcher.php @@ -16,7 +16,7 @@ * - Class can be specified as wildcard `* => [...]` in order to allow those methods/properties for all classes. * - Method/property can be specified as wildcard eg. `\DateTime => '*'` in order to allow all methods/properties for that class. * - Method/property can also be specified with a trailing wildcard to allow all methods/properties with a certain prefix, eg. `\DateTime => ['get*', ...]` in order to allow all methods/properties that start with `get`. - * + * * @author Yaakov Saxon */ final class MemberMatcher @@ -28,17 +28,17 @@ public function __construct(array $allowedMembers) { $normalizedMembers = []; foreach ($allowedMembers as $class => $members) { - if (!is_array($members)) { + if (!\is_array($members)) { $normalizedMembers[$class][] = strtolower($members); - } - else foreach ($members as $index => $member) { - $normalizedMembers[$class][$index] = strtolower($member); + } else { + foreach ($members as $index => $member) { + $normalizedMembers[$class][$index] = strtolower($member); + } } } $this->allowedMembers = $normalizedMembers; } - public function isAllowed($obj, string $member): bool { $cacheKey = get_class($obj) . "::" . $member; @@ -51,19 +51,22 @@ public function isAllowed($obj, string $member): bool $member = strtolower($member); // normalize member name foreach ($this->allowedMembers as $class => $members) { - if ($class === '*' || $obj instanceof $class) { + if ('*' === $class || $obj instanceof $class) { foreach ($members as $allowedMember) { - if ($allowedMember === '*') { + if ('*' === $allowedMember) { $this->cache[$cacheKey] = true; + return true; } if ($allowedMember === $member) { $this->cache[$cacheKey] = true; + return true; } - //if allowedMember ends with a *, check if the member starts with the allowedMember - if (substr($allowedMember, -1) === '*' && substr($member, 0, strlen($allowedMember) - 1) === rtrim($allowedMember, '*')) { + // if allowedMember ends with a *, check if the member starts with the allowedMember + if ('*' === substr($allowedMember, -1) && substr($member, 0, \strlen($allowedMember) - 1) === rtrim($allowedMember, '*')) { $this->cache[$cacheKey] = true; + return true; } }