From 2c887b55e50241770155d56ce895551aefc0bfa0 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Sun, 8 Nov 2026 23:08:13 +0100 Subject: [PATCH] fix(Security): improve regex and quote handling in strip_image_tags() --- docs/changelogs/v4.7.7.md | 1 + system/Helpers/security_helper.php | 80 +++++++++++++-- tests/system/Helpers/SecurityHelperTest.php | 106 ++++++++++++++++++++ 3 files changed, 179 insertions(+), 8 deletions(-) diff --git a/docs/changelogs/v4.7.7.md b/docs/changelogs/v4.7.7.md index ac104dc1bb40..09fcf6189c5e 100644 --- a/docs/changelogs/v4.7.7.md +++ b/docs/changelogs/v4.7.7.md @@ -19,6 +19,7 @@ Release Date: Unreleased - **CORS:** Fixed a bug where `Cors::hasResponseHeaders()` matched header substrings via `str_contains()`, causing false positives when existing exposed headers shared a prefix with configured exposed headers. - **HTTP:** Fixed a bug where `UploadedFile::move()` and `UploadedFile::store()` on Windows could store a file under a different name than `getName()` reported, or write its contents to an NTFS alternate data stream. Filenames ending in a period or space or containing a colon are now rejected, including explicitly supplied names. - **Security:** Fixed a bug where `Security::__construct()` threw a `TypeError` when the CSRF cookie was an array, causing an HTTP 500 response for affected requests. +- **Security:** Fixed a bug in `strip_image_tags()` where an invalid PCRE character class caused attribute injection and improper quote handling, and ensured tags without `src` are stripped. - **Worker Mode:** Fixed a bug where every request that ended with an uncaught exception, including a `404 Page Not Found` without a 404 override, called `exit()` in the exception handler. This ended the worker script, so FrankenPHP booted a new one, and the worker loop skipped its cleanup after the request. The exception handler now returns to the worker loop. See the repo's [CHANGELOG.md](https://github.com/codeigniter4/CodeIgniter4/blob/develop/CHANGELOG.md) for a complete list of bugs fixed. diff --git a/system/Helpers/security_helper.php b/system/Helpers/security_helper.php index 2a89f6838498..d1a6d36acd84 100644 --- a/system/Helpers/security_helper.php +++ b/system/Helpers/security_helper.php @@ -86,17 +86,81 @@ function sanitize_filename(string $filename, bool $relativePath = false): string if (! function_exists('strip_image_tags')) { /** * Strip Image Tags + * + * Replaces every `` tag with the value of its `src` attribute + * (or removes it when there is none). A `>` inside a quoted attribute + * value does not end the tag. If PCRE fails, the image tags are removed + * without regular expressions, so unfiltered input is never returned. */ function strip_image_tags(string $str): string { - return preg_replace( - [ - '##i', - '#`]+)).*?\>#i', - ], - '\\2', - $str, - ); + if (! str_contains(strtolower($str), '") or any non-">" char. + $tagPattern = '#])((?:"[^"]*"|\'[^\']*\'|[^>])*+)>#i'; + $attrPattern = '#[\s/]+([^\s"\'<>/=]+)(?:\s*=\s*(?:"([^"]*)"|\'([^\']*)\'|([^\s"\'<>`]+)))?#'; + + $replaceTag = static function (array $matches) use ($attrPattern): string { + if (preg_match_all($attrPattern, $matches[1], $attributes, PREG_SET_ORDER) === false) { + return ''; + } + + foreach ($attributes as $attribute) { + if (strtolower($attribute[1]) === 'src') { + return ($attribute[2] ?? '') . ($attribute[3] ?? '') . ($attribute[4] ?? ''); + } + } + + return ''; + }; + + // Regex-free removal of image tags, used only when PCRE fails. + $removeTags = static function (string $input): string { + $output = ''; + $length = strlen($input); + $offset = 0; + + while (($pos = stripos($input, '') { + break; + } + } + + $offset = $end + 1; + } + + return $output . substr($input, $offset); + }; + + $iterations = 0; + + do { + if (++$iterations > 25) { + $str = $removeTags($str); + break; + } + + $original = $str; + $result = preg_replace_callback($tagPattern, $replaceTag, $str); + $str = $result ?? $removeTags($str); + } while ($str !== $original && str_contains(strtolower($str), 'assertSame('http://example.com/spacer.gif', strip_image_tags('http://example.com/spacer.gif')); $this->assertSame('http://example.com/spacer.gif', strip_image_tags('Who needs CSS when you have a spacer.gif?')); + $this->assertSame('http://example.com/spacer.gif', strip_image_tags('test')); + $this->assertSame('http://example.com/spacer.gif', strip_image_tags('test')); + $this->assertSame('http://example.com/spacer.gif', strip_image_tags('')); + $this->assertSame('http://example.com/test.png', strip_image_tags('')); + $this->assertSame('http://example.com/test.jpg', strip_image_tags('test\'s photo')); + $this->assertSame('http://example.com/test.jpg', strip_image_tags('\'test')); + + // Empty src or tags without src should be stripped + $this->assertSame('', strip_image_tags('')); + $this->assertSame('', strip_image_tags('')); + $this->assertSame('', strip_image_tags('')); + $this->assertSame('', strip_image_tags('no image')); + $this->assertSame('Hello World', strip_image_tags('Hello World')); + $this->assertSame('Hello World', strip_image_tags('Hello World')); + $this->assertSame('Hello World', strip_image_tags('Hello World')); + + // Multiline img tag + $this->assertSame('http://example.com/test.png', strip_image_tags("")); + + // Intervening text between multiple img tags must not be swallowed + $this->assertSame('Text1 Text2 bar.jpg Text3', strip_image_tags('Text1 foo Text2 Text3')); + $this->assertSame('Start 1.jpg Middle 2.jpg End', strip_image_tags('Start Middle End')); + + // Attribute injection and quote handling + $this->assertSame('test', strip_image_tags('')); + $this->assertSame('test', strip_image_tags('')); + $this->assertSame('test', strip_image_tags('')); + $this->assertSame('test', strip_image_tags('')); + + // Nested img tag bypass + $this->assertSame('x', strip_image_tags('g src=x onerror=alert(1)>')); + $this->assertSame('x', strip_image_tags('g>g src=x onerror=alert(1)>')); + + // Only actual src attribute should be matched, not attributes ending with src (e.g. data-src) + $this->assertSame('', strip_image_tags('')); + $this->assertSame('http://example.com/real.gif', strip_image_tags('')); + $this->assertSame('http://example.com/real.gif', strip_image_tags('')); + } + + public function testStripImageTagsGreaterThanSignInsideQuotedAttribute(): void + { + $this->assertSame('photo.jpg', strip_image_tags('><svg onload=alert(1)>')); + $this->assertSame('photo.jpg', strip_image_tags("><svg onload=alert(1)>")); + $this->assertSame('photo.jpg', strip_image_tags('><svg onload=alert(1)>')); + $this->assertSame('', strip_image_tags('><svg onload=alert(1)>')); + $this->assertSame('Before photo.jpg After', strip_image_tags('Before a>b After')); + } + + public function testStripImageTagsNestedTags(): void + { + $this->assertSame('x', strip_image_tags('g src=x onerror=alert(1)>')); + $this->assertSame('x', strip_image_tags('g>g src=x onerror=alert(1)>')); + $this->assertStringNotContainsString('g src=x onerror=alert(1)>')); + $this->assertStringNotContainsString('onerror', strip_image_tags('g src=x onerror=alert(1)>')); + } + + public function testStripImageTagsIgnoresDataSrcAttribute(): void + { + $this->assertSame('real.gif', strip_image_tags('')); + $this->assertSame('real.gif', strip_image_tags('')); + $this->assertSame('', strip_image_tags('')); + } + + public function testStripImageTagsWithoutSrcAttribute(): void + { + $this->assertSame('', strip_image_tags('')); + $this->assertSame('', strip_image_tags('test')); + $this->assertSame('', strip_image_tags('test')); + } + + public function testStripImageTagsIsCaseInsensitive(): void + { + $this->assertSame('photo.jpg', strip_image_tags('')); + } + + public function testStripImageTagsFallbackWhenPcreFails(): void + { + $oldBacktrack = ini_get('pcre.backtrack_limit'); + $oldJit = ini_get('pcre.jit'); + + try { + // Force PCRE failure: JIT does not honour backtrack_limit. + ini_set('pcre.jit', '0'); + ini_set('pcre.backtrack_limit', '1'); + + // Make sure PCRE really fails for this input, so the fallback is exercised. + // This also keeps the test independent of the test order: a pattern + // already compiled and JIT-cached by earlier tests still honours + // backtrack_limit once JIT is disabled, and the assertion below turns + // any silent PCRE success into an explicit failure. + $input = ''; + @preg_match('#])((?:"[^"]*"|\'[^\']*\'|[^>])*+)>#i', $input); + $this->assertNotSame(PREG_NO_ERROR, preg_last_error(), 'PCRE was expected to fail with backtrack_limit=1.'); + + $result = strip_image_tags($input); + + $this->assertStringNotContainsString('assertStringNotContainsString('onerror', $result); + + $result = strip_image_tags('Hello a>b World'); + + $this->assertSame('Hello World', $result); + } finally { + ini_set('pcre.backtrack_limit', (string) $oldBacktrack); + ini_set('pcre.jit', (string) $oldJit); + } } public function testEncodePhpTags(): void