Skip to content

Commit de09ba1

Browse files
eliashaeusslergarvinhicking
eliashaeussler
authored and
garvinhicking
committed
[TASK] Implement and apply custom PHPStan rule to harden unserialize
This patch integrates a custom PHPStan rule to detect unsecure or invalid calls to the `unserialize` function. The rule was copied over from [1] and may get removed again once it is merged into PHPStan core. As a consequence, remaining potentially insecure `unserialize` calls are covered by `@phpstan-ignore` annotations. This affects the following code parts: * `AuthenticatedMessageDeserializer`: Doing `unserialize` with `['allowed_classes' => true]` at this point is intended, as the relevant integrity check already happens via HMAC validation. * `ActionController`: Same here (HMAC validation) * `FormRuntime`: Same here (HMAC validation) * Tests: Doing insecure `unserialize` in tests is currently ignored, because the effect of having potential security leaks is extreme low. To allow PHPStan to properly detect the types used for `unserialize` in `PackageCacheEntry`, this class now receives higher type coverage. [1]: phpstan/phpstan-src#4754 Resolves: #110120 Releases: main, 14.3, 13.4 Change-Id: I04255241d697703c27ad6c557b8f917f0a70e329 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/94648 Reviewed-by: Garvin Hicking <garvin@hick.ing> Tested-by: core-ci <typo3@b13.com> Tested-by: Garvin Hicking <garvin@hick.ing>
1 parent a879c47 commit de09ba1

4 files changed

Lines changed: 4 additions & 1 deletion

File tree

‎Classes/Mvc/Controller/ActionController.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,7 @@ protected function forwardToReferringRequest(): ?ResponseInterface
650650
);
651651
$arguments = [];
652652
if (is_string($referringRequestArguments['arguments'] ?? null)) {
653+
/* @phpstan-ignore unserialize.allowedClasses.insecure (Integrity check already happens via HMAC validation) */
653654
$arguments = unserialize(
654655
base64_decode($this->hashService->validateAndStripHmac(
655656
$referringRequestArguments['arguments'],

‎Tests/Functional/Persistence/LazyLoadingProxyTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ public function serializeAndUnserialize(): void
5050

5151
self::assertFalse(str_contains($serialized, 'dataMapper'), 'Assert that serialized object string does not contain dataMapper');
5252

53+
/* @phpstan-ignore unserialize.allowedClasses.insecure (Serialization within testing context does no harm) */
5354
$administratorProxy = unserialize($serialized, ['allowed_classes' => true]);
5455
self::assertInstanceOf(LazyLoadingProxy::class, $administratorProxy, 'Assert that $administratorProxy is an instance of LazyLoadingProxy');
5556

‎Tests/Functional/Persistence/LazyObjectStorageTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ public function serializeAndUnserialize(): void
5353

5454
self::assertFalse(str_contains($serialized, 'dataMapper'), 'Assert that serialized object string does not contain dataMapper');
5555

56+
/* @phpstan-ignore unserialize.allowedClasses.insecure (Serialization within testing context does no harm) */
5657
$postsProxy = unserialize($serialized, ['allowed_classes' => true]);
5758
self::assertInstanceOf(LazyObjectStorage::class, $postsProxy, 'Assert that $postsProxy is an instance of LazyObjectStorage');
5859

‎Tests/Unit/Reflection/ReflectionServiceTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ public function reflectionServiceIsResetDuringWakeUp(): void
8888
$insecureString = file_get_contents(__DIR__ . '/Fixture/InsecureSerializedReflectionService.txt');
8989
// Note: We need to use the silence operator here for `unserialize()`, otherwise PHP8.3 would emit a warning
9090
// because of unneeded bytes in the content which needs to be unserialized.
91-
$reflectionService = @unserialize($insecureString);
91+
$reflectionService = @unserialize($insecureString, ['allowed_classes' => [ReflectionService::class]]);
9292

9393
$reflectionClass = new \ReflectionClass($reflectionService);
9494
$classSchemaProperty = $reflectionClass->getProperty('classSchemata');

0 commit comments

Comments
 (0)