diff options
| -rw-r--r-- | app/Factories/ImageFactory.php | 96 | ||||
| -rw-r--r-- | tests/app/Factories/ImageFactoryTest.php | 133 | ||||
| -rw-r--r-- | tests/data/media/safe.svg | 1 | ||||
| -rw-r--r-- | tests/data/media/unsafe.svg | 1 |
4 files changed, 226 insertions, 5 deletions
diff --git a/app/Factories/ImageFactory.php b/app/Factories/ImageFactory.php index 7141bb1497..9f5394d8f0 100644 --- a/app/Factories/ImageFactory.php +++ b/app/Factories/ImageFactory.php @@ -19,6 +19,8 @@ declare(strict_types=1); namespace Fisharebest\Webtrees\Factories; +use DOMDocument; +use DOMElement; use Fig\Http\Message\StatusCodeInterface; use Fisharebest\Webtrees\Auth; use Fisharebest\Webtrees\Contracts\ImageFactoryInterface; @@ -47,11 +49,17 @@ use function addcslashes; use function basename; use function get_class; use function implode; +use function libxml_clear_errors; +use function libxml_use_internal_errors; use function pathinfo; +use function preg_replace; use function response; -use function str_contains; +use function str_starts_with; +use function stripos; +use function strtolower; use function view; +use const LIBXML_NONET; use const PATHINFO_EXTENSION; class ImageFactory implements ImageFactoryInterface @@ -64,6 +72,15 @@ class ImageFactory implements ImageFactoryInterface protected const int THUMBNAIL_CACHE_TTL = 8640000; + private const array DANGEROUS_SVG_TAGS = [ + 'script', + 'foreignobject', + 'iframe', + 'object', + 'embed', + 'handler', + ]; + public const array SUPPORTED_FORMATS = [ 'image/jpeg' => 'jpg', 'image/png' => 'png', @@ -267,9 +284,16 @@ class ImageFactory implements ImageFactoryInterface protected function imageResponse(string $data, string $mime_type, string $filename): ResponseInterface { - if ($mime_type === 'image/svg+xml' && str_contains(haystack: $data, needle: '<script')) { - return $this->replacementImageResponse(text: 'XSS') - ->withHeader('x-image-exception', 'SVG image blocked due to XSS.'); + if ($mime_type === 'image/svg+xml') { + if (!$this->php_service->extensionLoaded(extension: 'dom')) { + return $this->replacementImageResponse(text: 'DOM') + ->withHeader('x-image-exception', 'Need the PHP dom extension to verify SVG files.'); + } + + if ($this->svgContainsActiveContent(data: $data)) { + return $this->replacementImageResponse(text: 'XSS') + ->withHeader('x-image-exception', 'SVG image blocked due to XSS.'); + } } // HTML files may contain JavaScript and iframes, so use content-security-policy to disable them. @@ -282,7 +306,69 @@ class ImageFactory implements ImageFactoryInterface } return $response - ->withHeader('content-disposition', 'attachment; filename="' . addcslashes(string: basename(path: $filename), characters: '"')); + ->withHeader('content-disposition', 'attachment; filename="' . addcslashes(string: basename(path: $filename), characters: '"') . '"'); + } + + /** + * Determine whether an SVG document contains active content (script elements, + * event-handler attributes, javascript: URLs) that could execute in a browser. + * + * Although we have a content-security-policy to disable scripts, a user may + * download this file and distribute it, so better to block it. + */ + private function svgContainsActiveContent(string $data): bool + { + $previous_error_state = libxml_use_internal_errors(true); + + try { + $document = new DOMDocument(); + // LIBXML_NONET disables network access so external DTDs and entities + // cannot be fetched — mitigates XXE/SSRF on malformed payloads. + $loaded = $document->loadXML($data, LIBXML_NONET); + + if ($loaded === false || !$document->documentElement instanceof DOMElement) { + // Malformed SVG — treat conservatively and block. + return true; + } + + return $this->svgElementIsDangerous($document->documentElement); + } finally { + libxml_clear_errors(); + libxml_use_internal_errors($previous_error_state); + } + } + + private function svgElementIsDangerous(DOMElement $element): bool + { + if (in_array(strtolower($element->localName), self::DANGEROUS_SVG_TAGS, true)) { + return true; + } + + foreach ($element->attributes as $attribute) { + $name = strtolower($attribute->nodeName); + + // Event-handler attributes such as "onload" + if (str_starts_with($name, 'on')) { + return true; + } + + // Normalize whitespace to catch malformed values that browsers might accept, + // such as "java\tscript:". + $value = (string) $attribute->nodeValue; + $value_compact = preg_replace('/\s+/', '', $value) ?? ''; + + if (str_starts_with(strtolower($value_compact), 'javascript:')) { + return true; + } + } + + foreach ($element->childNodes as $child) { + if ($child instanceof DOMElement && $this->svgElementIsDangerous(element: $child)) { + return true; + } + } + + return false; } protected function imageManager(): ImageManager diff --git a/tests/app/Factories/ImageFactoryTest.php b/tests/app/Factories/ImageFactoryTest.php index 3f77d78c1c..bfe43cfffd 100644 --- a/tests/app/Factories/ImageFactoryTest.php +++ b/tests/app/Factories/ImageFactoryTest.php @@ -21,8 +21,13 @@ namespace Fisharebest\Webtrees\Factories; use Fisharebest\Webtrees\Services\PhpService; use Fisharebest\Webtrees\TestCase; +use League\Flysystem\Filesystem; +use League\Flysystem\FilesystemOperator; +use League\Flysystem\Local\LocalFilesystemAdapter; use PHPUnit\Framework\Attributes\CoversClass; +use function dirname; + #[CoversClass(ImageFactory::class)] class ImageFactoryTest extends TestCase { @@ -38,4 +43,132 @@ class ImageFactoryTest extends TestCase $response->getHeaderLine('content-security-policy'), ); } + + public function testFileResponseAddsDownloadHeaderForSafeSvg(): void + { + $php_service = $this->createStub(PhpService::class); + $image_factory = new ImageFactory($php_service); + $filesystem = $this->mediaFilesystem(); + + $php_service + ->method('extensionLoaded') + ->willReturnCallback(static fn (string $extension): bool => $extension === 'dom'); + + $response = $image_factory->fileResponse( + filesystem: $filesystem, + path: 'safe.svg', + download: true, + ); + + self::assertSame('image/svg+xml', $response->getHeaderLine('content-type')); + self::assertSame('default-src none', $response->getHeaderLine('content-security-policy')); + self::assertSame('attachment; filename="safe.svg"', $response->getHeaderLine('content-disposition')); + } + + public function testFileResponseBlocksSvgWithActiveContent(): void + { + $php_service = $this->createStub(PhpService::class); + $image_factory = new ImageFactory($php_service); + $filesystem = $this->mediaFilesystem(); + + $php_service + ->method('extensionLoaded') + ->willReturnCallback(static fn (string $extension): bool => $extension === 'dom'); + + $response = $image_factory->fileResponse( + filesystem: $filesystem, + path: 'unsafe.svg', + download: false, + ); + + self::assertSame('image/svg+xml', $response->getHeaderLine('content-type')); + self::assertSame('SVG image blocked due to XSS.', $response->getHeaderLine('x-image-exception')); + } + + public function testFileResponseBlocksSvgWithoutDomExtension(): void + { + $php_service = $this->createStub(PhpService::class); + $image_factory = new ImageFactory($php_service); + $filesystem = $this->mediaFilesystem(); + + $php_service + ->method('extensionLoaded') + ->willReturnCallback(static fn (string $extension): bool => false); + + $response = $image_factory->fileResponse( + filesystem: $filesystem, + path: 'safe.svg', + download: false, + ); + + self::assertSame('image/svg+xml', $response->getHeaderLine('content-type')); + self::assertSame( + 'Need the PHP dom extension to verify SVG files.', + $response->getHeaderLine('x-image-exception'), + ); + } + + public function testFileResponseReturnsNotFoundForMissingFile(): void + { + $image_factory = new ImageFactory(new PhpService()); + $filesystem = $this->mediaFilesystem(); + + $response = $image_factory->fileResponse( + filesystem: $filesystem, + path: 'missing.svg', + download: false, + ); + + self::assertSame('image/svg+xml', $response->getHeaderLine('content-type')); + self::assertStringContainsString( + 'UnableToReadFile', + $response->getHeaderLine('x-thumbnail-exception'), + ); + } + + public function testThumbnailResponseReturnsImageForJpeg(): void + { + $image_factory = new ImageFactory(new PhpService()); + $filesystem = $this->mediaFilesystem(); + + $response = $image_factory->thumbnailResponse( + filesystem: $filesystem, + path: 'Elizabeth_II.jpg', + width: 40, + height: 40, + fit: 'contain', + ); + + self::assertSame('image/jpeg', $response->getHeaderLine('content-type')); + self::assertSame('default-src none', $response->getHeaderLine('content-security-policy')); + self::assertSame('', $response->getHeaderLine('content-disposition')); + self::assertNotSame('', $response->getBody()->getContents()); + } + + public function testThumbnailResponseReturnsNotFoundForMissingFile(): void + { + $image_factory = new ImageFactory(new PhpService()); + $filesystem = $this->mediaFilesystem(); + + $response = $image_factory->thumbnailResponse( + filesystem: $filesystem, + path: 'missing.jpg', + width: 40, + height: 40, + fit: 'contain', + ); + + self::assertSame('image/svg+xml', $response->getHeaderLine('content-type')); + self::assertStringContainsString( + 'UnableTo', + $response->getHeaderLine('x-thumbnail-exception'), + ); + } + + private function mediaFilesystem(): FilesystemOperator + { + $root = dirname(__DIR__, 2) . '/data/media'; + + return new Filesystem(new LocalFilesystemAdapter($root)); + } } diff --git a/tests/data/media/safe.svg b/tests/data/media/safe.svg new file mode 100644 index 0000000000..b802fe2e26 --- /dev/null +++ b/tests/data/media/safe.svg @@ -0,0 +1 @@ +<svg xmlns="http://www.w3.org/2000/svg" width="2" height="2"><rect width="2" height="2" fill="#0f0"/></svg> diff --git a/tests/data/media/unsafe.svg b/tests/data/media/unsafe.svg new file mode 100644 index 0000000000..f1e1a68567 --- /dev/null +++ b/tests/data/media/unsafe.svg @@ -0,0 +1 @@ +<svg xmlns="http://www.w3.org/2000/svg" width="2" height="2"><script>alert(1)</script></svg> |
