diff --git a/system/HTTP/DownloadResponse.php b/system/HTTP/DownloadResponse.php index b7ce79b8309a..997ab90529d5 100644 --- a/system/HTTP/DownloadResponse.php +++ b/system/HTTP/DownloadResponse.php @@ -71,7 +71,7 @@ public function __construct(string $filename, bool $setMime) { parent::__construct(config(App::class)); - $this->filename = $filename; + $this->filename = str_replace(["\r", "\n"], '', $filename); $this->setMime = $setMime; // Make sure the content type is either specified or detected @@ -113,7 +113,7 @@ public function setFilePath(string $filepath) */ public function setFileName(string $filename) { - $this->filename = $filename; + $this->filename = str_replace(["\r", "\n"], '', $filename); return $this; } @@ -161,8 +161,8 @@ private function setContentTypeByMimeType(): void */ private function getDownloadFileName(): string { - $filename = $this->filename; - $x = explode('.', $this->filename); + $filename = str_replace(["\r", "\n"], '', $this->filename); + $x = explode('.', $filename); $extension = end($x); /* It was reported that browsers on Android 2.1 (and possibly older as well) diff --git a/tests/system/HTTP/DownloadResponseTest.php b/tests/system/HTTP/DownloadResponseTest.php index d5fea35e380c..b0f34f2c1f4f 100644 --- a/tests/system/HTTP/DownloadResponseTest.php +++ b/tests/system/HTTP/DownloadResponseTest.php @@ -154,6 +154,30 @@ public function testDispositionInlineWithSetFileName(): void $this->assertSame('inline; filename="my\"quoted\"File.txt"; filename*=UTF-8\'\'my%22quoted%22File.txt', $response->getHeaderLine('Content-Disposition')); } + public function testContentDispositionSanitizesCRLF(): void + { + $response = new DownloadResponse("test\r\nfile.txt", false); + $response->buildHeaders(); + + $header = $response->getHeaderLine('Content-Disposition'); + $this->assertStringNotContainsString("\r", $header); + $this->assertStringNotContainsString("\n", $header); + $this->assertSame('attachment; filename="testfile.txt"; filename*=UTF-8\'\'testfile.txt', $header); + + $response = new DownloadResponse('report.pdf', false); + $response->setFileName("report\r\nSet-Cookie: evil=1\r\n.pdf"); + + // The filename must be sanitized to strip carriage returns and newlines, + // preventing HTTP response splitting / header injection without throwing an RFC 7230 error. + $response->buildHeaders(); + + $header = $response->getHeaderLine('Content-Disposition'); + $this->assertStringNotContainsString("\r", $header); + $this->assertStringNotContainsString("\n", $header); + $this->assertFalse($response->hasHeader('Set-Cookie')); + $this->assertSame('attachment; filename="reportSet-Cookie: evil=1.pdf"; filename*=UTF-8\'\'reportSet-Cookie%3A%20evil%3D1.pdf', $header); + } + public function testNoCache(): void { $response = new DownloadResponse('unit-test.txt', true); diff --git a/user_guide_src/source/changelogs/v4.7.5.rst b/user_guide_src/source/changelogs/v4.7.5.rst index f13a3e4b2b49..db1bc68d3875 100644 --- a/user_guide_src/source/changelogs/v4.7.5.rst +++ b/user_guide_src/source/changelogs/v4.7.5.rst @@ -43,6 +43,7 @@ Bugs Fixed - **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. +- **DownloadResponse:** Fixed a bug where filenames containing carriage returns or newlines were not sanitized, potentially leading to HTTP response splitting (CRLF / header injection). - **Files:** Fixed a bug where ``File::move()`` and ``UploadedFile::move()`` set executable and overly permissive file permissions (``0777 & ~umask()`` instead of ``0666 & ~umask()``), and ``UploadedFile::move()`` targeted the parent directory instead of the destination file for ``chmod()``. - **Helpers:** Fixed a bug where ``get_dir_file_info()`` returned incomplete entries for subdirectories and missing files instead of omitting them. - **Honeypot:** Fixed a bug where bot detection returned an HTTP 500 response instead of 403 (Forbidden).