fix(Security): improve regex and quote handling in strip_image_tags()

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.

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 `<img ...>` 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(
-            [
-                '#<img[\s/]+.*?src\s*=\s*(["\'])([^\\1]+?)\\1.*?\>#i',
-                '#<img[\s/]+.*?src\s*=\s*?(([^\s"\'=<>`]+)).*?\>#i',
-            ],
-            '\\2',
-            $str,
-        );
+        if (! str_contains(strtolower($str), '<img')) {
+            return $str;
+        }
+
+        // The tag body: quoted values (which may contain ">") or any non-">" char.
+        $tagPattern  = '#<img(?=[\s/>])((?:"[^"]*"|\'[^\']*\'|[^>])*+)>#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, '<img', $offset)) !== false) {
+                $output .= substr($input, $offset, $pos - $offset);
+                $end   = $pos + 4;
+                $quote = '';
+
+                for (; $end < $length; $end++) {
+                    $char = $input[$end];
+
+                    if ($quote !== '') {
+                        if ($char === $quote) {
+                            $quote = '';
+                        }
+                    } elseif ($char === '"' || $char === "'") {
+                        $quote = $char;
+                    } elseif ($char === '>') {
+                        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), '<img'));
+
+        return $str;
     }
 }
 

tests/system/Helpers/SecurityHelperTest.php

@@ -208,6 +208,112 @@ public function testStripImageTags(): void
         $this->assertSame('http://example.com/spacer.gif', strip_image_tags('http://example.com/spacer.gif'));
 
         $this->assertSame('http://example.com/spacer.gif', strip_image_tags('<img src="http://example.com/spacer.gif" alt="Who needs CSS when you have a spacer.gif?" />'));
+        $this->assertSame('http://example.com/spacer.gif', strip_image_tags('<img src=\'http://example.com/spacer.gif\' alt="test" />'));
+        $this->assertSame('http://example.com/spacer.gif', strip_image_tags('<img src=http://example.com/spacer.gif alt="test" />'));
+        $this->assertSame('http://example.com/spacer.gif', strip_image_tags('<img src  =  "http://example.com/spacer.gif" />'));
+        $this->assertSame('http://example.com/test.png', strip_image_tags('<img class="my-class" id="my-img" src="http://example.com/test.png">'));
+        $this->assertSame('http://example.com/test.jpg', strip_image_tags('<img alt="test\'s photo" src="http://example.com/test.jpg">'));
+        $this->assertSame('http://example.com/test.jpg', strip_image_tags('<img alt=\'test "quoted" photo\' src=\'http://example.com/test.jpg\'>'));
+
+        // Empty src or tags without src should be stripped
+        $this->assertSame('', strip_image_tags('<img src="" onerror="alert(1)">'));
+        $this->assertSame('', strip_image_tags('<img src=\'\' onerror=\'alert(1)\'>'));
+        $this->assertSame('', strip_image_tags('<img onerror="alert(1)">'));
+        $this->assertSame('', strip_image_tags('<img alt="no image">'));
+        $this->assertSame('Hello  World', strip_image_tags('Hello <img> World'));
+        $this->assertSame('Hello  World', strip_image_tags('Hello <img/> World'));
+        $this->assertSame('Hello  World', strip_image_tags('Hello <img > World'));
+
+        // Multiline img tag
+        $this->assertSame('http://example.com/test.png', strip_image_tags("<img\n  class=\"foo\"\n  src=\"http://example.com/test.png\"\n  alt=\"bar\">"));
+
+        // Intervening text between multiple img tags must not be swallowed
+        $this->assertSame('Text1  Text2 bar.jpg Text3', strip_image_tags('Text1 <img alt="foo"> Text2 <img src="bar.jpg"> Text3'));
+        $this->assertSame('Start 1.jpg Middle 2.jpg End', strip_image_tags('Start <img src="1.jpg"> Middle <img src="2.jpg"> End'));
+
+        // Attribute injection and quote handling
+        $this->assertSame('test', strip_image_tags('<img src="test" onmouseover="alert(1)">'));
+        $this->assertSame('test', strip_image_tags('<img src=\'test\' onmouseover=\'alert(1)\'>'));
+        $this->assertSame('test', strip_image_tags('<img src=\'test\' onmouseover="alert(1)">'));
+        $this->assertSame('test', strip_image_tags('<img src="test" onmouseover=\'alert(1)\'>'));
+
+        // Nested img tag bypass
+        $this->assertSame('x', strip_image_tags('<im<img>g src=x onerror=alert(1)>'));
+        $this->assertSame('x', strip_image_tags('<im<im<img>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('<img data-src="http://example.com/fake.gif">'));
+        $this->assertSame('http://example.com/real.gif', strip_image_tags('<img data-src="http://example.com/fake.gif" src="http://example.com/real.gif">'));
+        $this->assertSame('http://example.com/real.gif', strip_image_tags('<img src="http://example.com/real.gif" data-src="http://example.com/fake.gif">'));
+    }
+
+    public function testStripImageTagsGreaterThanSignInsideQuotedAttribute(): void
+    {
+        $this->assertSame('photo.jpg', strip_image_tags('<img alt="><svg onload=alert(1)>" src="photo.jpg">'));
+        $this->assertSame('photo.jpg', strip_image_tags("<img alt='><svg onload=alert(1)>' src='photo.jpg'>"));
+        $this->assertSame('photo.jpg', strip_image_tags('<img src="photo.jpg" alt="><svg onload=alert(1)>">'));
+        $this->assertSame('', strip_image_tags('<img alt="><svg onload=alert(1)>">'));
+        $this->assertSame('Before photo.jpg After', strip_image_tags('Before <img alt="a>b" src="photo.jpg"> After'));
+    }
+
+    public function testStripImageTagsNestedTags(): void
+    {
+        $this->assertSame('x', strip_image_tags('<im<img>g src=x onerror=alert(1)>'));
+        $this->assertSame('x', strip_image_tags('<im<im<img>g>g src=x onerror=alert(1)>'));
+        $this->assertStringNotContainsString('<img', strip_image_tags('<im<img>g src=x onerror=alert(1)>'));
+        $this->assertStringNotContainsString('onerror', strip_image_tags('<im<img>g src=x onerror=alert(1)>'));
+    }
+
+    public function testStripImageTagsIgnoresDataSrcAttribute(): void
+    {
+        $this->assertSame('real.gif', strip_image_tags('<img data-src="fake.gif" src="real.gif">'));
+        $this->assertSame('real.gif', strip_image_tags('<img src="real.gif" data-src="fake.gif">'));
+        $this->assertSame('', strip_image_tags('<img data-src="fake.gif">'));
+    }
+
+    public function testStripImageTagsWithoutSrcAttribute(): void
+    {
+        $this->assertSame('', strip_image_tags('<img>'));
+        $this->assertSame('', strip_image_tags('<img alt="test">'));
+        $this->assertSame('', strip_image_tags('<IMG ALT="test">'));
+    }
+
+    public function testStripImageTagsIsCaseInsensitive(): void
+    {
+        $this->assertSame('photo.jpg', strip_image_tags('<IMG SRC="photo.jpg">'));
+    }
+
+    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 = '<img src="photo.jpg" onerror="alert(1)">';
+            @preg_match('#<img(?=[\s/>])((?:"[^"]*"|\'[^\']*\'|[^>])*+)>#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('<img', $result);
+            $this->assertStringNotContainsString('onerror', $result);
+
+            $result = strip_image_tags('Hello <img alt="a>b" onerror=alert(1)> World');
+
+            $this->assertSame('Hello  World', $result);
+        } finally {
+            ini_set('pcre.backtrack_limit', (string) $oldBacktrack);
+            ini_set('pcre.jit', (string) $oldJit);
+        }
     }
 
     public function testEncodePhpTags(): void