[5.x] Tighten SVG sanitization

composer.json

@@ -23,7 +23,7 @@
         "michelf/php-smartypants": "^1.8.1",
         "nesbot/carbon": "^2.62.1 || ^3.0",
         "rebing/graphql-laravel": "^9.8",
-        "rhukster/dom-sanitizer": "^1.0.7",
+        "rhukster/dom-sanitizer": "^1.0.19",
         "spatie/blink": "^1.3",
         "spatie/error-solutions": "^1.0 || ^2.0",
         "statamic/stringy": "^3.1.2",

src/Assets/Asset.php

@@ -952,7 +952,7 @@ public function reupload(ReplacementFile $file)
             $this->disk()->put(
                 $this->path(),
                 (new DOMSanitizer(DOMSanitizer::SVG))->sanitize($contents, [
-                    'remove-xml-tags' => ! Str::startsWith($contents, '<?xml'),
+                    'remove-xml-tags' => true,
                 ])
             );
         }

src/Assets/Uploader.php

@@ -5,7 +5,6 @@
 use Facades\Statamic\Imaging\ImageValidator;
 use Rhukster\DomSanitizer\DOMSanitizer;
 use Statamic\Facades\Glide;
-use Statamic\Support\Str;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
 
 abstract class Uploader
@@ -60,8 +59,8 @@ private function write($sourcePath, $destinationPath)
 
         if (config('statamic.assets.svg_sanitization_on_upload', true) && trim(strtolower(pathinfo($destinationPath, PATHINFO_EXTENSION))) === 'svg') {
             $sanitizer = new DOMSanitizer(DOMSanitizer::SVG);
-            $stream = $sanitizer->sanitize($svg = stream_get_contents($stream), [
-                'remove-xml-tags' => ! Str::startsWith($svg, '<?xml'),
+            $stream = $sanitizer->sanitize(stream_get_contents($stream), [
+                'remove-xml-tags' => true,
             ]);
         }
 

src/CP/Navigation/NavItem.php

@@ -212,7 +212,7 @@ private function sanitizeSvg(string $svg): string
             $sanitizer = new DOMSanitizer(DOMSanitizer::SVG);
 
             return $sanitizer->sanitize($svg, [
-                'remove-xml-tags' => ! Str::startsWith($svg, '<?xml'),
+                'remove-xml-tags' => true,
             ]);
         } catch (\Throwable $e) {
             return '';

src/Tags/Svg.php

@@ -106,7 +106,7 @@ private function sanitize($svg)
         $this->setAllowedTags($sanitizer);
 
         return $sanitizer->sanitize($svg, [
-            'remove-xml-tags' => ! Str::startsWith($svg, '<?xml'),
+            'remove-xml-tags' => true,
         ]);
     }
 

tests/Assets/AssetTest.php

@@ -2000,6 +2000,53 @@ public static function unnormalizedSvgExtensionProvider()
         ];
     }
 
+    #[Test]
+    #[DataProvider('xmlStylesheetSvgProvider')]
+    public function it_removes_xml_stylesheet_instructions_from_svgs_on_upload($svg)
+    {
+        Event::fake();
+
+        $asset = (new Asset)->container($this->container)->path('path/to/asset.svg')->syncOriginal();
+
+        Facades\AssetContainer::shouldReceive('findByHandle')->with('test_container')->andReturn($this->container);
+
+        $asset->upload(UploadedFile::fake()->createWithContent('asset.svg', $svg));
+
+        $this->assertStringNotContainsString('xml-stylesheet', $asset->contents());
+    }
+
+    #[Test]
+    #[DataProvider('xmlStylesheetSvgProvider')]
+    public function it_removes_xml_stylesheet_instructions_from_svgs_on_reupload($svg)
+    {
+        Event::fake();
+
+        $asset = (new Asset)->container($this->container)->path('path/to/asset.svg')->syncOriginal();
+
+        Facades\AssetContainer::shouldReceive('findByHandle')->with('test_container')->andReturn($this->container);
+
+        $asset->upload(UploadedFile::fake()->createWithContent('asset.svg', '<svg xmlns="http://www.w3.org/2000/svg"/>'));
+
+        Storage::fake('local')->put('path/to/replacement.svg', $svg);
+
+        $asset->reupload(new ReplacementFile('path/to/replacement.svg'));
+
+        $this->assertStringNotContainsString('xml-stylesheet', $asset->contents());
+    }
+
+    public static function xmlStylesheetSvgProvider()
+    {
+        // A processing instruction telling the browser to apply an inline xslt
+        // stylesheet, which would replace the image with arbitrary html.
+        $instruction = '<?xml-stylesheet type="text/xsl" href="data:text/xml;base64,PHhzbDpzdHlsZXNoZWV0Lz4="?>';
+
+        return [
+            'on its own' => [$instruction.'<svg xmlns="http://www.w3.org/2000/svg"/>'],
+            'after a declaration' => ['<?xml version="1.0"?>'.$instruction.'<svg xmlns="http://www.w3.org/2000/svg"/>'],
+            'on separate lines' => ['<?xml version="1.0"?>'."\n".$instruction."\n".'<svg xmlns="http://www.w3.org/2000/svg"/>'],
+        ];
+    }
+
     public static function nonGlideableFileExtensionsProvider()
     {
         return [

tests/Tags/SvgTagTest.php

@@ -3,6 +3,7 @@
 namespace Tests\Tags;
 
 use Illuminate\Support\Facades\File;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Test;
 use Statamic\Facades\Parse;
 use Statamic\Tags\Svg;
@@ -72,15 +73,34 @@ public function sanitizing_doesnt_add_xml_tag()
     }
 
     #[Test]
-    public function sanitizing_doesnt_remove_an_xml_tag()
+    public function sanitizing_removes_an_xml_tag()
     {
-        // We want to make sure that we haven't configured it to remove it if we wanted it there to begin with.
+        // The declaration is meaningless once the svg is inlined into an html document,
+        // and keeping it meant trusting an attacker controlled "<?xml" prefix.
 
-        $svg = '<?xml version="1.0" encoding="UTF-8"?><svg><path/></svg>';
+        File::put(resource_path('xmltag.svg'), '<?xml version="1.0" encoding="UTF-8"?><svg><path/></svg>');
 
-        File::put(resource_path('xmltag.svg'), $svg);
+        $this->assertEquals('<svg><path/></svg>', $this->tag('{{ svg src="xmltag" }}'));
+    }
+
+    #[Test]
+    #[DataProvider('xmlStylesheetProvider')]
+    public function sanitizing_removes_xml_stylesheet_processing_instructions($svg)
+    {
+        File::put(resource_path('stylesheet.svg'), $svg);
+
+        $this->assertStringNotContainsString('xml-stylesheet', $this->tag('{{ svg src="stylesheet" }}'));
+    }
+
+    public static function xmlStylesheetProvider()
+    {
+        $pi = '<?xml-stylesheet type="text/xsl" href="data:text/xml;base64,PHhzbDpzdHlsZXNoZWV0Lz4="?>';
 
-        $this->assertEquals($svg, $this->tag('{{ svg src="xmltag" }}'));
+        return [
+            'on its own' => [$pi.'<svg><path/></svg>'],
+            'after a declaration' => ['<?xml version="1.0"?>'.$pi.'<svg><path/></svg>'],
+            'on separate lines' => ['<?xml version="1.0"?>'."\n".$pi."\n".'<svg><path/></svg>'],
+        ];
     }
 
     #[Test]