Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion system/Cache/Handlers/PredisHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ public function get(string $key): mixed
}

return match ($data['__ci_type']) {
'array', 'object' => unserialize($data['__ci_value']),
'array' => unserialize($data['__ci_value'], ['allowed_classes' => false]),
'object' => unserialize($data['__ci_value']),
'boolean', 'integer', 'double', 'string', 'NULL' => settype($data['__ci_value'], $data['__ci_type']) ? $data['__ci_value'] : null,
default => null,
};
Expand Down
3 changes: 2 additions & 1 deletion system/Cache/Handlers/RedisHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ public function get(string $key): mixed
}

return match ($data['__ci_type']) {
'array', 'object' => unserialize($data['__ci_value']),
'array' => unserialize($data['__ci_value'], ['allowed_classes' => false]),
'object' => unserialize($data['__ci_value']),
Comment on lines +111 to +112
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cached arrays may legitimately contain nested objects. This changes existing behavior by restoring them as __PHP_Incomplete_Class. It also does not prevent cache poisoning because an attacker with direct Redis write access can set __ci_type = object.

'boolean', 'integer', 'double', 'string', 'NULL' => settype($data['__ci_value'], $data['__ci_type']) ? $data['__ci_value'] : null,
default => null,
};
Expand Down
2 changes: 1 addition & 1 deletion system/Cache/ResponseCache.php
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public function get(CLIRequest|IncomingRequest $request, ResponseInterface $resp
$cachedResponse = $this->cache->get($this->generateCacheKey($request));

if (is_string($cachedResponse) && $cachedResponse !== '') {
$cachedResponse = unserialize($cachedResponse);
$cachedResponse = unserialize($cachedResponse, ['allowed_classes' => false]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResponseCache normally stores only scalar values and arrays of strings.


if (
! is_array($cachedResponse)
Expand Down
2 changes: 1 addition & 1 deletion system/Entity/Cast/ArrayCast.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class ArrayCast extends BaseCast
public static function get($value, array $params = []): array
{
if (is_string($value) && (str_starts_with($value, 'a:') || str_starts_with($value, 's:'))) {
$value = unserialize($value);
$value = unserialize($value, ['allowed_classes' => false]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may break existing applications that store arrays containing entities or other objects. This is why it wasn't changed.

}

return (array) $value;
Expand Down
33 changes: 33 additions & 0 deletions tests/system/Cache/ResponseCacheTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -257,4 +257,37 @@ public function testInvalidCacheError(): void
// Check cache with a request with the same URI path.
$pageCache->get($request, new Response(new App()));
}

public function testGetResponseCacheRejectsObjectsInSerializedData(): void
{
/** @var MockCache $mockCache */
$mockCache = mock(CacheFactory::class);
$pageCache = new ResponseCache(new Cache(), $mockCache);

$request = $this->createIncomingRequest('foo/bar');

$response = new Response(new App());
$response->setHeader('ETag', 'abcd1234');
$response->setBody('The response body.');

$pageCache->make($request, $response);

$cacheKey = $pageCache->generateCacheKey($request);

// Inject a serialized object (PHP Object Injection simulation).
// With allowed_classes => false, unserialize() returns an
// __PHP_Incomplete_Class and the subsequent is_array() check rejects it.
$malicious = serialize([
'output' => serialize(new \stdClass()),
'headers' => [],
'status' => 200,
'reason' => 'OK',
]);
$mockCache->save($cacheKey, $malicious);

// The cache entry contains a nested serialized object; get() should
// return false (cache miss) rather than instantiating the object.
$result = $pageCache->get($request, new Response(new App()));
$this->assertFalse($result);
}
}
20 changes: 20 additions & 0 deletions tests/system/Entity/EntityTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,26 @@ public function testCastArrayByConstructor(): void
$this->assertSame([1, 2, 3], $entity->seventh);
}

public function testCastArrayRejectsSerializedObjects(): void
{
// A serialized object injected as a database column value should not
// be instantiated. With allowed_classes => false the object becomes
// a __PHP_Incomplete_Class, which is then cast to an array — the
// important thing is no real class is instantiated.
$entity = $this->getCastEntity();

// Store a serialized stdClass directly into the raw attribute
// (simulating a poisoned DB column value).
$this->setPrivateProperty($entity, 'attributes', array_merge(
$this->getPrivateProperty($entity, 'attributes'),
['seventh' => serialize(new \stdClass())]
));

// Accessing the cast attribute must NOT instantiate stdClass.
$result = $entity->seventh;
$this->assertNotInstanceOf(\stdClass::class, $result);
}

public function testCastNullable(): void
{
$entity = $this->getCastNullableEntity();
Expand Down
3 changes: 3 additions & 0 deletions user_guide_src/source/changelogs/v4.7.4.rst
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ Deprecations
Bugs Fixed
**********

- **Security:** ``RedisHandler::get()`` and ``PredisHandler::get()`` now pass ``['allowed_classes' => false]`` to ``unserialize()`` when restoring cached arrays, preventing PHP Object Injection via a poisoned cache backend. See `Security Advisory <https://github.com/codeigniter4/CodeIgniter4/security>`_.
- **Security:** ``ResponseCache::get()`` now passes ``['allowed_classes' => false]`` to ``unserialize()``, preventing PHP Object Injection via the page cache.
- **Security:** ``Entity/Cast/ArrayCast::get()`` now passes ``['allowed_classes' => false]`` to ``unserialize()``, preventing PHP Object Injection via a poisoned database column value.
- **Database:** Fixed a bug where ``updateBatch()`` could be called after Query Builder ``where()`` conditions, even though it's not supported. In this situation, now the ``DatabaseException`` is thrown.

See the repo's
Expand Down
Loading