Skip to content

Commit 8aa8ea0

Browse files
Merge commit from fork
* fix: prevent path traversal in file cache deletion * apply review suggestions Co-authored-by: John Paul E Balandan <paulbalandan@gmail.com> --------- Co-authored-by: John Paul E Balandan <paulbalandan@gmail.com>
1 parent 609d458 commit 8aa8ea0

5 files changed

Lines changed: 184 additions & 2 deletions

File tree

‎system/Cache/Handlers/FileHandler.php‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
use CodeIgniter\Cache\Exceptions\CacheException;
1717
use CodeIgniter\I18n\Time;
1818
use Config\Cache;
19+
use FilesystemIterator;
1920
use Throwable;
21+
use UnexpectedValueException;
2022

2123
/**
2224
* File system cache handler
@@ -117,10 +119,25 @@ public function delete(string $key): bool
117119

118120
public function deleteMatching(string $pattern): int
119121
{
122+
// The pattern is matched against file names only and never becomes part
123+
// of a path, so it cannot reach files outside the cache directory.
124+
// Passing the cache path to glob() would also interpret special
125+
// characters in the path (e.g. "[") as part of the pattern.
126+
try {
127+
$iterator = new FilesystemIterator($this->path, FilesystemIterator::SKIP_DOTS);
128+
} catch (UnexpectedValueException) {
129+
return 0;
130+
}
131+
120132
$deleted = 0;
121133

122-
foreach (glob($this->path . $pattern, GLOB_NOSORT) as $filename) {
123-
if (is_file($filename) && @unlink($filename)) {
134+
foreach ($iterator as $file) {
135+
// FNM_PERIOD keeps glob() semantics: wildcards don't match a leading dot.
136+
if (! fnmatch($pattern, $file->getFilename(), FNM_PERIOD)) {
137+
continue;
138+
}
139+
140+
if ($file->isFile() && @unlink($file->getPathname())) {
124141
$deleted++;
125142
}
126143
}

‎tests/system/Cache/Handlers/FileHandlerTest.php‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,132 @@ public function testDeleteMatchingSuffix(): void
210210
$this->handler->clean();
211211
}
212212

213+
/**
214+
* @param string $pattern "{victim}" is replaced with the name of a file in the parent of the cache directory
215+
*/
216+
#[DataProvider('provideDeleteMatchingDoesNotDeleteFilesOutsideCacheDirectory')]
217+
public function testDeleteMatchingDoesNotDeleteFilesOutsideCacheDirectory(string $pattern): void
218+
{
219+
$parent = realpath(dirname($this->config->file['storePath']));
220+
$this->assertNotFalse($parent);
221+
222+
$victim = tempnam($parent, 'deleteMatchingVictim');
223+
$this->assertNotFalse($victim);
224+
225+
try {
226+
// tempnam() falls back to the system temp directory on failure.
227+
$this->assertSame($parent, realpath(dirname($victim)));
228+
229+
$this->assertTrue($this->handler->save(self::$key1, 'value'));
230+
231+
$pattern = str_replace('{victim}', basename($victim), $pattern);
232+
233+
$this->assertSame(0, $this->handler->deleteMatching($pattern));
234+
$this->assertFileExists($victim);
235+
$this->assertSame('value', $this->handler->get(self::$key1));
236+
} finally {
237+
if (is_file($victim)) {
238+
unlink($victim);
239+
}
240+
}
241+
}
242+
243+
/**
244+
* @return iterable<string, array{string}>
245+
*/
246+
public static function provideDeleteMatchingDoesNotDeleteFilesOutsideCacheDirectory(): iterable
247+
{
248+
yield 'parent directory' => ['../{victim}'];
249+
250+
yield 'parent directory with wildcard' => ['../{victim}*'];
251+
252+
yield 'parent directory with backslash' => ['..\\{victim}'];
253+
254+
yield 'parent directory only' => ['..'];
255+
}
256+
257+
public function testDeleteMatchingDoesNotDeleteFilesInSubdirectories(): void
258+
{
259+
$directoryName = 'dir_' . bin2hex(random_bytes(8));
260+
$subdirectory = $this->config->file['storePath'] . '/' . $directoryName . '/';
261+
262+
$this->assertTrue(mkdir($subdirectory));
263+
264+
try {
265+
$this->assertNotFalse(file_put_contents($subdirectory . 'key_1', 'data'));
266+
267+
$this->assertSame(0, $this->handler->deleteMatching($directoryName . '/key_*'));
268+
$this->assertFileExists($subdirectory . 'key_1');
269+
} finally {
270+
if (is_file($subdirectory . 'key_1')) {
271+
unlink($subdirectory . 'key_1');
272+
}
273+
274+
rmdir($subdirectory);
275+
}
276+
}
277+
278+
#[RequiresOperatingSystem('Linux|Darwin')]
279+
public function testDeleteMatchingSupportsEscapedWildcards(): void
280+
{
281+
$this->assertTrue($this->handler->save('key*1', 'value'));
282+
$this->assertTrue($this->handler->save('key_1', 'value'));
283+
284+
$this->assertSame(1, $this->handler->deleteMatching('key\*1'));
285+
$this->assertNull($this->handler->get('key*1'));
286+
$this->assertSame('value', $this->handler->get('key_1'));
287+
}
288+
289+
public function testDeleteMatchingTreatsCachePathLiterally(): void
290+
{
291+
// As a glob pattern, "cache[1]" would match the sibling "cache1" instead of itself.
292+
$cachePath = $this->config->file['storePath'] . '/cache[1]/';
293+
$siblingPath = $this->config->file['storePath'] . '/cache1/';
294+
295+
$this->assertTrue(mkdir($cachePath));
296+
$this->assertTrue(mkdir($siblingPath));
297+
298+
try {
299+
$this->assertNotFalse(file_put_contents($siblingPath . 'key_1', 'data'));
300+
301+
$config = new Cache();
302+
$config->file['storePath'] = $cachePath;
303+
$handler = CacheFactory::getHandler($config, 'file');
304+
305+
$this->assertTrue($handler->save('key_1', 'value'));
306+
307+
$this->assertSame(1, $handler->deleteMatching('*'));
308+
$this->assertNull($handler->get('key_1'));
309+
$this->assertFileExists($siblingPath . 'key_1');
310+
} finally {
311+
foreach ([$cachePath, $siblingPath] as $path) {
312+
if (is_file($path . 'key_1')) {
313+
unlink($path . 'key_1');
314+
}
315+
316+
rmdir($path);
317+
}
318+
}
319+
}
320+
321+
public function testDeleteMatchingWildcardDoesNotMatchHiddenFiles(): void
322+
{
323+
$hidden = $this->config->file['storePath'] . '/.hidden';
324+
325+
$this->assertNotFalse(file_put_contents($hidden, 'data'));
326+
327+
try {
328+
$this->assertTrue($this->handler->save(self::$key1, 'value'));
329+
330+
$this->assertSame(1, $this->handler->deleteMatching('*'));
331+
$this->assertFileExists($hidden);
332+
} finally {
333+
if (is_file($hidden)) {
334+
unlink($hidden);
335+
}
336+
}
337+
}
338+
213339
public function testIncrement(): void
214340
{
215341
$this->handler->save(self::$key1, 1);

‎user_guide_src/source/changelogs/v4.7.6.rst‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,19 @@ Release Date: Unreleased
1010
:local:
1111
:depth: 3
1212

13+
********
14+
SECURITY
15+
********
16+
17+
- **Cache:** ``FileHandler::deleteMatching()`` now matches the pattern against cache
18+
file names only. Previously, a pattern such as ``../*`` could delete files outside
19+
the cache directory. Patterns can no longer match files in subdirectories of the
20+
cache directory, so those files are no longer deleted. Special characters in the
21+
cache path, such as ``[``, are no longer interpreted as part of the pattern.
22+
See the `Security advisory GHSA-9hf2-g3w9-pfmp <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-9hf2-g3w9-pfmp>`_
23+
for more information. See :ref:`upgrade-476-cache-delete-matching` for migration
24+
instructions.
25+
1326
********
1427
BREAKING
1528
********

‎user_guide_src/source/installation/upgrade_476.rst‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,28 @@ Mandatory File Changes
2020
Breaking Changes
2121
****************
2222

23+
.. _upgrade-476-cache-delete-matching:
24+
25+
File Cache deleteMatching() No Longer Matches Subdirectories
26+
============================================================
27+
28+
For security reasons, ``FileHandler::deleteMatching()`` now matches the pattern against
29+
the names of files in the cache directory only. See the
30+
`Security advisory GHSA-9hf2-g3w9-pfmp <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-9hf2-g3w9-pfmp>`_
31+
for more information.
32+
33+
Previously, the pattern was appended to the cache path and passed to ``glob()``, so a
34+
pattern containing a directory separator, such as ``dir/key_*``, also deleted matching
35+
files in a subdirectory of the cache directory. Such a pattern now never matches
36+
anything and the method returns ``0``.
37+
38+
Most applications are not affected. The File handler stores every cache item directly
39+
in the cache directory, and the default ``Config\Cache::$reservedCharacters`` forbid
40+
``/`` and ``\`` in cache keys, so cache items could not be stored in subdirectories.
41+
If your application places its own files in subdirectories of the cache directory and
42+
removed them with ``deleteMatching()``, delete them directly instead, for example with
43+
the :php:func:`delete_files()` helper function.
44+
2345
*********************
2446
Breaking Enhancements
2547
*********************

‎user_guide_src/source/libraries/caching.rst‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,10 @@ Class Reference
179179
.. important:: This method is only implemented for File, Redis and Predis handlers.
180180
Due to limitations, it couldn't be implemented for Memcached and Wincache handlers.
181181

182+
.. note:: With the File handler, the pattern is matched against cache
183+
file names only and cannot match files in subdirectories. A pattern
184+
containing a forward slash (``/``) therefore never matches.
185+
182186
Example:
183187

184188
.. literalinclude:: caching/006.php

0 commit comments

Comments
 (0)