From 48615f231b8554a44f206cf499b8de3f0f985406 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Sat, 5 Sep 2026 22:09:39 +0200 Subject: [PATCH] fix: prevent path traversal in development server rewrite script --- system/rewrite.php | 15 +- tests/system/RewriteTest.php | 211 ++++++++++++++++++++ user_guide_src/source/changelogs/v4.7.5.rst | 1 + 3 files changed, 225 insertions(+), 2 deletions(-) create mode 100644 tests/system/RewriteTest.php diff --git a/system/rewrite.php b/system/rewrite.php index 4f9b79ade95c..f1a7458fa22c 100644 --- a/system/rewrite.php +++ b/system/rewrite.php @@ -31,13 +31,24 @@ // Full path $path = $_SERVER['DOCUMENT_ROOT'] . DIRECTORY_SEPARATOR . ltrim($uri, '/'); +// Security: prevent path traversal outside DOCUMENT_ROOT. +// realpath() resolves ../, symlinks and returns false for non-existing paths. +$realPath = realpath($path); +$realDocRoot = realpath($_SERVER['DOCUMENT_ROOT']); + // If $path is an existing file or folder within the public folder // then let the request handle it like normal. -if ($uri !== '/' && (is_file($path) || is_dir($path))) { +if ( + $uri !== '/' + && $realPath !== false + && $realDocRoot !== false + && ($realPath === $realDocRoot || str_starts_with($realPath, $realDocRoot . DIRECTORY_SEPARATOR)) + && (is_file($realPath) || is_dir($realPath)) +) { return false; } -unset($uri, $path); +unset($uri, $path, $realPath, $realDocRoot); // Otherwise, we'll load the index file and let // the framework handle the request from here. diff --git a/tests/system/RewriteTest.php b/tests/system/RewriteTest.php new file mode 100644 index 000000000000..df04df1bdc7b --- /dev/null +++ b/tests/system/RewriteTest.php @@ -0,0 +1,211 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter; + +use CodeIgniter\Test\CIUnitTestCase; +use FilesystemIterator; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use RecursiveDirectoryIterator; +use RecursiveIteratorIterator; + +/** + * Regression tests for system/rewrite.php path traversal (todo #36). + * + * Proves that the fixed rewrite.php does NOT serve files outside + * DOCUMENT_ROOT via ../ or encoded traversal, while still serving + * legitimate public assets. + * + * @internal + */ +#[Group('Others')] +final class RewriteTest extends CIUnitTestCase +{ + private string $tmpRoot = ''; + private string $docRoot = ''; + + protected function setUp(): void + { + parent::setUp(); + + $this->tmpRoot = sys_get_temp_dir() . '/ci4_rewrite_' . uniqid('', true); + $this->docRoot = $this->tmpRoot . '/public'; + + mkdir($this->docRoot . '/assets', 0777, true); + mkdir($this->tmpRoot . '/public2', 0777, true); + + // Legitimate public files + file_put_contents($this->docRoot . '/index.php', 'docRoot . '/test.txt', 'hello'); + file_put_contents($this->docRoot . '/assets/app.css', 'body{}'); + + // Files OUTSIDE docroot that must NOT be served + file_put_contents($this->tmpRoot . '/outside.txt', 'secret'); + file_put_contents($this->tmpRoot . '/public2/also.txt', 'also'); + file_put_contents($this->docRoot . '/../outside2.txt', 'secret2'); + } + + protected function tearDown(): void + { + parent::tearDown(); + $this->removeDir($this->tmpRoot); + } + + private function removeDir(string $dir): void + { + if (! is_dir($dir)) { + return; + } + $it = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($dir, FilesystemIterator::SKIP_DOTS), + RecursiveIteratorIterator::CHILD_FIRST, + ); + + foreach ($it as $file) { + $file->isDir() ? rmdir($file->getPathname()) : unlink($file->getPathname()); + } + rmdir($dir); + } + + /** + * Mirrors the FIXED logic from system/rewrite.php:24-49. + * Returns true if rewrite.php would "return false" (serve file directly). + */ + private function shouldServeFileFixed(string $requestUri, string $docRoot): bool + { + $uri = urldecode(parse_url('https://codeigniter.com' . $requestUri, PHP_URL_PATH) ?? ''); + + $path = $docRoot . DIRECTORY_SEPARATOR . ltrim($uri, '/'); + + $realPath = realpath($path); + $realDocRoot = realpath($docRoot); + + return $uri !== '/' + && $realPath !== false + && $realDocRoot !== false + && ($realPath === $realDocRoot || str_starts_with($realPath, $realDocRoot . DIRECTORY_SEPARATOR)) + && (is_file($realPath) || is_dir($realPath)); + } + + /** + * VULNERABLE logic before fix (for proof): no realpath prefix check. + */ + private function shouldServeFileVulnerable(string $requestUri, string $docRoot): bool + { + $uri = urldecode(parse_url('https://codeigniter.com' . $requestUri, PHP_URL_PATH) ?? ''); + $path = $docRoot . DIRECTORY_SEPARATOR . ltrim($uri, '/'); + + return $uri !== '/' && (is_file($path) || is_dir($path)); + } + + public function testServesLegitimatePublicFile(): void + { + $this->assertTrue($this->shouldServeFileFixed('/test.txt', $this->docRoot)); + $this->assertTrue($this->shouldServeFileFixed('/assets/app.css', $this->docRoot)); + $this->assertTrue($this->shouldServeFileFixed('/assets', $this->docRoot)); + } + + public function testDoesNotServeRoot(): void + { + $this->assertFalse($this->shouldServeFileFixed('/', $this->docRoot)); + } + + public function testDoesNotServeNonExistingFile(): void + { + $this->assertFalse($this->shouldServeFileFixed('/nope.txt', $this->docRoot)); + } + + #[DataProvider('provideBlocksPathTraversalOutsideDocRoot')] + public function testBlocksPathTraversalOutsideDocRoot(string $payload): void + { + // Fixed version must NOT serve + $this->assertFalse( + $this->shouldServeFileFixed($payload, $this->docRoot), + 'Fixed rewrite.php should block traversal payload: ' . $payload, + ); + } + + /** + * @return iterable + */ + public static function provideBlocksPathTraversalOutsideDocRoot(): iterable + { + yield 'plain dotdot' => ['/../outside.txt']; + + yield 'nested dotdot' => ['/../../outside.txt']; + + yield 'dotdot with subdir' => ['/assets/../../outside.txt']; + + yield 'encoded dotdot' => ['/%2e%2e/outside.txt']; + + yield 'double encoded' => ['/%252e%252e/outside.txt']; // becomes %2e after one urldecode -> safe + + yield 'sibling public2 file' => ['/../public2/also.txt']; + + yield 'sibling public2 dir' => ['/../public2']; + + yield 'traversal via subdir' => ['/assets/../../../outside.txt']; + + yield 'absolute outside2' => ['/../outside2.txt']; + + // Prefix bypass: docroot /public must not match /public2 + yield 'prefix bypass' => ['/../public2/also.txt']; + } + + public function testProvesVulnerabilityBeforeFix(): void + { + // This proves the OLD code WAS vulnerable: it would have served outside files + $this->assertTrue( + $this->shouldServeFileVulnerable('/../outside.txt', $this->docRoot), + 'Vulnerable logic serves /../outside.txt outside docroot', + ); + $this->assertTrue( + $this->shouldServeFileVulnerable('/%2e%2e/outside.txt', $this->docRoot), + 'Vulnerable logic serves encoded traversal', + ); + $this->assertTrue( + $this->shouldServeFileVulnerable('/../public2/also.txt', $this->docRoot), + 'Vulnerable logic serves sibling docroot file', + ); + + // Fixed logic blocks them + $this->assertFalse($this->shouldServeFileFixed('/../outside.txt', $this->docRoot)); + $this->assertFalse($this->shouldServeFileFixed('/%2e%2e/outside.txt', $this->docRoot)); + $this->assertFalse($this->shouldServeFileFixed('/../public2/also.txt', $this->docRoot)); + } + + public function testRewriteFileContainsTraversalProtection(): void + { + $content = (string) file_get_contents(SYSTEMPATH . 'rewrite.php'); + + // Must use realpath + prefix check + $this->assertStringContainsString('realpath', $content, 'rewrite.php must use realpath()'); + $this->assertStringContainsString('realpath($_SERVER', $content); + $this->assertStringContainsString('str_starts_with', $content, 'rewrite.php must verify prefix'); + $this->assertStringContainsString('realDocRoot', $content); + $this->assertStringContainsString('DIRECTORY_SEPARATOR', $content); + } + + public function testEncodedTraversalDecodedOnce(): void + { + // urldecode() in rewrite.php decodes %2e once; double encoding stays safe + $uri = urldecode(parse_url('https://codeigniter.com/%252e%252e/outside.txt', PHP_URL_PATH) ?? ''); + $this->assertSame('/%2e%2e/outside.txt', $uri, 'Double encoding decodes only once'); + + // Single encoding must decode to ../ + $uri2 = urldecode(parse_url('https://codeigniter.com/%2e%2e/outside.txt', PHP_URL_PATH) ?? ''); + $this->assertSame('/../outside.txt', $uri2); + $this->assertFalse($this->shouldServeFileFixed('/%2e%2e/outside.txt', $this->docRoot)); + } +} diff --git a/user_guide_src/source/changelogs/v4.7.5.rst b/user_guide_src/source/changelogs/v4.7.5.rst index f13a3e4b2b49..8094ac912b1c 100644 --- a/user_guide_src/source/changelogs/v4.7.5.rst +++ b/user_guide_src/source/changelogs/v4.7.5.rst @@ -40,6 +40,7 @@ Bugs Fixed ANSI color codes in the prompt (e.g., option defaults) are wrapped in readline's non-printing markers under GNU readline so cursor positioning stays accurate. - **CLIRequest:** Fixed a bug where ``parseCommand()`` could throw a TypeError when ``argv`` is missing. - **CodeIgniter:** Fixed a bug where ``gatherOutput()`` could be called twice when ``startController()`` returned a ``ResponseInterface`` (e.g., from filter attributes or closure routes). +- **Commands:** Fixed a bug in ``rewrite.php`` where files outside the document root could be served by the development server. - **Content Security Policy:** Fixed a bug where empty ``Content-Security-Policy``, ``Content-Security-Policy-Report-Only``, and ``Reporting-Endpoints`` response headers were generated when no corresponding values existed. - **Cookie:** Fixed a bug where ``Cookie`` instances created with ``raw: true`` allowed invalid characters in cookie values rejected by ``setrawcookie()``. - **Database:** Fixed a bug where rebuilding a SQLite3 table (e.g., ``Forge::dropColumn()``, ``Forge::modifyColumn()``, ``Forge::dropForeignKey()`` and ``Forge::dropPrimaryKey()``) corrupted the table names referenced by its foreign keys when ``DBPrefix`` was set.