Skip to content

Commit 8cf6fad

Browse files
michalsnmichalsn
authored
Merge commit from fork
1 parent 6942f3a commit 8cf6fad

6 files changed

Lines changed: 205 additions & 1 deletion

File tree

‎system/Helpers/security_helper.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
* If it is acceptable for the user input to include relative paths,
2525
* e.g. file/in/some/approved/folder.txt, you can set the second optional
2626
* parameter, $relativePath to TRUE.
27+
* Only forward slashes are preserved, backslashes are always removed.
2728
*
2829
* @param string $filename Input file name
2930
* @param bool $relativePath Whether to preserve paths
@@ -32,6 +33,7 @@ function sanitize_filename(string $filename, bool $relativePath = false): string
3233
{
3334
// List of sanitized filename strings
3435
$bad = [
36+
'\\',
3537
'../',
3638
'<!--',
3739
'-->',
@@ -77,7 +79,7 @@ function sanitize_filename(string $filename, bool $relativePath = false): string
7779
$filename = str_replace($bad, '', $filename);
7880
} while ($old !== $filename);
7981

80-
return stripslashes($filename);
82+
return $filename;
8183
}
8284
}
8385

‎tests/system/Helpers/SecurityHelperTest.php‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use CodeIgniter\Test\CIUnitTestCase;
1717
use CodeIgniter\Test\Mock\MockSecurity;
1818
use Config\Security as SecurityConfig;
19+
use PHPUnit\Framework\Attributes\DataProvider;
1920
use PHPUnit\Framework\Attributes\Group;
2021
use Tests\Support\Config\Services;
2122

@@ -45,6 +46,161 @@ public function testSanitizeFilenameStripsExtras(): void
4546
$this->assertSame('foo ', sanitize_filename($filename));
4647
}
4748

49+
#[DataProvider('provideSanitizeFilenamePaths')]
50+
public function testSanitizeFilenamePaths(string $filename, bool $relativePath, string $expected): void
51+
{
52+
$this->assertSame($expected, sanitize_filename($filename, $relativePath));
53+
}
54+
55+
/**
56+
* @return iterable<string, array{string, bool, string}>
57+
*/
58+
public static function provideSanitizeFilenamePaths(): iterable
59+
{
60+
$backslash = chr(92);
61+
$doubleBackslash = str_repeat($backslash, 2);
62+
$tripleBackslash = str_repeat($backslash, 3);
63+
$fourBackslashes = str_repeat($backslash, 4);
64+
$eightBackslashes = str_repeat($backslash, 8);
65+
66+
return [
67+
'empty filename' => ['', false, ''],
68+
'escaped NUL in filename' => ['a' . $backslash . '0b', false, 'a0b'],
69+
'escaped NUL in relative path' => ['a' . $backslash . '0b', true, 'a0b'],
70+
'literal NUL in filename' => ['a' . chr(0) . 'b', false, 'ab'],
71+
'literal NUL in relative path' => ['a' . chr(0) . 'b', true, 'ab'],
72+
'strip forward path' => [
73+
'approved/folder/file.txt',
74+
false,
75+
'approvedfolderfile.txt',
76+
],
77+
'preserve forward relative path' => [
78+
'approved/folder/file.txt',
79+
true,
80+
'approved/folder/file.txt',
81+
],
82+
'strip forward traversal' => ['../../public/file.txt', false, 'publicfile.txt'],
83+
'relative forward traversal' => ['../../public/file.txt', true, 'public/file.txt'],
84+
'forward traversal exposed by filters' => ['.<.>/public/file.txt', true, 'public/file.txt'],
85+
'forward traversal split by backslash' => ['.' . $backslash . './file.txt', true, 'file.txt'],
86+
'backslash traversal split by backslash' => [
87+
'.' . $backslash . '.' . $doubleBackslash . 'file.txt',
88+
true,
89+
'..file.txt',
90+
],
91+
'mixed separators' => [
92+
'..' . $doubleBackslash . '../public' . $doubleBackslash . 'file.txt',
93+
true,
94+
'..publicfile.txt',
95+
],
96+
'backslashes exposed by filters' => [
97+
'folder' . $backslash . '<' . $backslash . 'file.txt',
98+
true,
99+
'folderfile.txt',
100+
],
101+
'filename traversal with single backslash' => [
102+
'..' . $backslash . '..' . $backslash . 'public' . $backslash . 'file.txt',
103+
false,
104+
'....publicfile.txt',
105+
],
106+
'filename path with single backslash' => [
107+
'folder' . $backslash . 'file.txt',
108+
false,
109+
'folderfile.txt',
110+
],
111+
'relative traversal with single backslash' => [
112+
'..' . $backslash . '..' . $backslash . 'public' . $backslash . 'file.txt',
113+
true,
114+
'....publicfile.txt',
115+
],
116+
'relative path with single backslash' => [
117+
'folder' . $backslash . 'file.txt',
118+
true,
119+
'folderfile.txt',
120+
],
121+
'filename traversal with double backslashes' => [
122+
'..' . $doubleBackslash . '..' . $doubleBackslash . 'public' . $doubleBackslash . 'file.txt',
123+
false,
124+
'....publicfile.txt',
125+
],
126+
'filename path with double backslashes' => [
127+
'folder' . $doubleBackslash . 'file.txt',
128+
false,
129+
'folderfile.txt',
130+
],
131+
'relative traversal with double backslashes' => [
132+
'..' . $doubleBackslash . '..' . $doubleBackslash . 'public' . $doubleBackslash . 'file.txt',
133+
true,
134+
'....publicfile.txt',
135+
],
136+
'relative path with double backslashes' => [
137+
'folder' . $doubleBackslash . 'file.txt',
138+
true,
139+
'folderfile.txt',
140+
],
141+
'filename traversal with triple backslashes' => [
142+
'..' . $tripleBackslash . '..' . $tripleBackslash . 'public' . $tripleBackslash . 'file.txt',
143+
false,
144+
'....publicfile.txt',
145+
],
146+
'filename path with triple backslashes' => [
147+
'folder' . $tripleBackslash . 'file.txt',
148+
false,
149+
'folderfile.txt',
150+
],
151+
'relative traversal with triple backslashes' => [
152+
'..' . $tripleBackslash . '..' . $tripleBackslash . 'public' . $tripleBackslash . 'file.txt',
153+
true,
154+
'....publicfile.txt',
155+
],
156+
'relative path with triple backslashes' => [
157+
'folder' . $tripleBackslash . 'file.txt',
158+
true,
159+
'folderfile.txt',
160+
],
161+
'filename traversal with four backslashes' => [
162+
'..' . $fourBackslashes . '..' . $fourBackslashes . 'public' . $fourBackslashes . 'file.txt',
163+
false,
164+
'....publicfile.txt',
165+
],
166+
'filename path with four backslashes' => [
167+
'folder' . $fourBackslashes . 'file.txt',
168+
false,
169+
'folderfile.txt',
170+
],
171+
'relative traversal with four backslashes' => [
172+
'..' . $fourBackslashes . '..' . $fourBackslashes . 'public' . $fourBackslashes . 'file.txt',
173+
true,
174+
'....publicfile.txt',
175+
],
176+
'relative path with four backslashes' => [
177+
'folder' . $fourBackslashes . 'file.txt',
178+
true,
179+
'folderfile.txt',
180+
],
181+
'filename traversal with eight backslashes' => [
182+
'..' . $eightBackslashes . '..' . $eightBackslashes . 'public' . $eightBackslashes . 'file.txt',
183+
false,
184+
'....publicfile.txt',
185+
],
186+
'filename path with eight backslashes' => [
187+
'folder' . $eightBackslashes . 'file.txt',
188+
false,
189+
'folderfile.txt',
190+
],
191+
'relative traversal with eight backslashes' => [
192+
'..' . $eightBackslashes . '..' . $eightBackslashes . 'public' . $eightBackslashes . 'file.txt',
193+
true,
194+
'....publicfile.txt',
195+
],
196+
'relative path with eight backslashes' => [
197+
'folder' . $eightBackslashes . 'file.txt',
198+
true,
199+
'folderfile.txt',
200+
],
201+
];
202+
}
203+
48204
public function testStripImageTags(): void
49205
{
50206
$this->assertSame('http://example.com/spacer.gif', strip_image_tags('http://example.com/spacer.gif'));

‎tests/system/Security/SecurityTest.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,16 @@ public function testSanitizeFilename(): void
317317
$this->assertSame('foo', $security->sanitizeFilename($filename));
318318
}
319319

320+
public function testSanitizeFilenameBackslashTraversal(): void
321+
{
322+
$security = $this->createMockSecurity();
323+
$separator = str_repeat(chr(92), 2);
324+
$filename = '..' . $separator . '..' . $separator . 'public' . $separator . 'file.txt';
325+
326+
$this->assertSame('....publicfile.txt', $security->sanitizeFilename($filename));
327+
$this->assertSame('file.txt', $security->sanitizeFilename('.' . chr(92) . './file.txt', true));
328+
}
329+
320330
public function testRegenerateWithFalseSecurityRegenerateProperty(): void
321331
{
322332
service('superglobals')

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,17 @@ SECURITY
3131
for migration instructions.
3232
See the `Security advisory GHSA-jv74-fx4h-m63x <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-jv74-fx4h-m63x>`_
3333
for more information.
34+
- **Security Helper:** Fixed a path traversal vulnerability in ``sanitize_filename()``.
35+
Backslashes were only removed by a final ``stripslashes()`` call after filtering.
36+
When ``$relativePath`` was ``true``, input such as ``.\./`` became ``../`` on all
37+
platforms. In both modes, repeated backslashes could survive as Windows path
38+
separators, and ``\0`` could become a NUL byte.
39+
Backslashes are now removed during filtering, and ``stripslashes()`` is no longer
40+
called. Forward slashes are still preserved when ``$relativePath`` is ``true``.
41+
The same applies to ``Security::sanitizeFilename()``.
42+
See :doc:`Upgrading from 4.7.5 to 4.7.6 </installation/upgrade_476>` and the
43+
`Security advisory GHSA-9j59-qcxw-q7qm <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-9j59-qcxw-q7qm>`_
44+
for more information.
3445

3546
********
3647
BREAKING

‎user_guide_src/source/helpers/security_helper.rst‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,13 @@ The following functions are available:
2929

3030
Provides protection against directory traversal.
3131

32+
Backslashes are always removed. If ``$relativePath`` is ``true``, forward slashes are
33+
preserved to allow relative paths such as ``path/to/file.txt``, while parent-directory
34+
sequences (``../``) are removed.
35+
36+
.. note:: When passing relative paths to this helper, use forward slashes (``/``) as
37+
directory separators, including on Windows.
38+
3239
For more info, please see the :doc:`Security Library <../libraries/security>`
3340
documentation.
3441

‎user_guide_src/source/installation/upgrade_476.rst‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,24 @@ See :ref:`model-replace` for details and replacement-specific precautions.
6262
Breaking Enhancements
6363
*********************
6464

65+
****************
66+
Behavior Changes
67+
****************
68+
69+
Filename Sanitization
70+
=====================
71+
72+
``sanitize_filename()`` and ``Security::sanitizeFilename()`` now remove all backslashes.
73+
Single backslashes between ordinary characters were already removed in previous versions,
74+
so paths such as ``folder\file.txt`` produce the same result. Ordinary filenames and
75+
relative paths with forward slashes are also unchanged. Results can differ for repeated
76+
backslashes, literal ``\0`` sequences, and inputs where removing backslashes exposes
77+
sequences stripped by the sanitizer.
78+
79+
Use forward slashes (``/``) for relative paths passed with ``$relativePath = true``,
80+
including on Windows. If your application stores or compares sanitized filenames created
81+
from these inputs, check for changed results and possible filename collisions.
82+
6583
*************
6684
Project Files
6785
*************

0 commit comments

Comments
 (0)