diff --git a/lib/Controller/ProxyController.php b/lib/Controller/ProxyController.php index 3aa9a35f0f..ab591843f5 100644 --- a/lib/Controller/ProxyController.php +++ b/lib/Controller/ProxyController.php @@ -10,9 +10,11 @@ namespace OCA\Mail\Controller; +use OCA\Mail\Exception\ServiceException; use OCA\Mail\Html\ProxyHmacGenerator; use OCA\Mail\Http\ProxyDownloadResponse; use OCA\Mail\Service\MailManager; +use OCA\Mail\Service\SvgSanitizer; use OCP\AppFramework\Controller; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; @@ -28,6 +30,8 @@ use Psr\Log\LoggerInterface; use function file_get_contents; use function hash_equals; +use function is_resource; +use function stream_get_contents; #[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)] class ProxyController extends Controller { @@ -44,6 +48,7 @@ public function __construct( private ProxyHmacGenerator $hmacGenerator, private LoggerInterface $logger, private MailManager $mailManager, + private SvgSanitizer $svgSanitizer, private ?string $userId, ) { parent::__construct($appName, $request); @@ -65,6 +70,7 @@ public function __construct( * mail does not know whether the mail has been opened. * * @return Response|ProxyDownloadResponse + * @throws ServiceException */ #[UserRateLimit(limit: 50, period: 60)] public function proxy(string $src, ?int $id, ?string $hmac): Response { @@ -73,7 +79,7 @@ public function proxy(string $src, ?int $id, ?string $hmac): Response { // If strict cookies are set it means we come from the same domain so no open redirect if (!$this->request->passesStrictCookieCheck()) { - $content = file_get_contents(__DIR__ . '/../../img/blocked-image.png'); + $content = $this->getBlockedImage(); return new ProxyDownloadResponse($content, $src, 'application/octet-stream'); } @@ -95,17 +101,49 @@ public function proxy(string $src, ?int $id, ?string $hmac): Response { try { $response = $client->get($src); $content = $response->getBody(); + if (is_resource($content)) { + $content = stream_get_contents($content); + if ($content === false) { + $content = $this->getBlockedImage(); + } + } } catch (ClientExceptionInterface $e) { $this->logger->notice('Unable to proxy image', ['exception' => $e]); - $content = file_get_contents(__DIR__ . '/../../img/blocked-image.png'); + $content = $this->getBlockedImage(); } catch (LocalServerException $e) { $this->logger->warning('Prevented image proxy access to forbidden URL', [ 'blockedUrl' => $src, 'exception' => $e, ]); - $content = file_get_contents(__DIR__ . '/../../img/blocked-image.png'); + $content = $this->getBlockedImage(); + } + + // Browsers sniff raster image formats in tags, but they refuse to + // render SVG unless it is served with the image/svg+xml content type. + // Detect and sanitise SVG markup so external SVG logos are displayed + // instead of staying blank. Sanitising also strips any active content in + // case the response is fetched through a direct (non-) navigation. + if ($this->svgSanitizer->looksLikeSvg($content)) { + $sanitized = $this->svgSanitizer->sanitize($content); + if ($sanitized === '') { + $content = $this->getBlockedImage(); + return new ProxyDownloadResponse($content, $src, 'application/octet-stream'); + } + return new ProxyDownloadResponse($sanitized, $src, 'image/svg+xml'); } return new ProxyDownloadResponse($content, $src, 'application/octet-stream'); } + + /** + * @throws ServiceException + */ + private function getBlockedImage(): string { + $content = file_get_contents(__DIR__ . '/../../img/blocked-image.png'); + if ($content === false) { + throw new ServiceException('Could not read blocked image'); + } + + return $content; + } } diff --git a/lib/Service/SvgSanitizer.php b/lib/Service/SvgSanitizer.php new file mode 100644 index 0000000000..06dd995000 --- /dev/null +++ b/lib/Service/SvgSanitizer.php @@ -0,0 +1,163 @@ +/CID context where scripts do + * not execute, but they are still sanitised as defence in depth: any document + * that cannot be parsed safely is dropped entirely. + */ +class SvgSanitizer { + /** Elements that can carry or execute active content. */ + private const FORBIDDEN_ELEMENTS = [ + 'script', + 'foreignObject', + 'handler', + 'listener', + 'set', + ]; + + /** Attributes that carry URL references and must not point off-document. */ + private const URL_ATTRIBUTES = ['href', 'xlink:href', 'src', 'action', 'formaction']; + + /** Reject payloads larger than this to prevent DoS via oversized documents. */ + private const MAX_SVG_BYTES = 2 * 1024 * 1024; + + /** + * @param string $svg The raw (decoded) SVG markup + * @return string The sanitised markup, or an empty string if it cannot be + * parsed safely + */ + public function sanitize(string $svg): string { + if (trim($svg) === '' || strlen($svg) > self::MAX_SVG_BYTES) { + return ''; + } + + // A DOCTYPE or entity declaration is not needed for plain SVG graphics + // and is a common XXE / entity-expansion vector. Reject such documents. + if (preg_match('/loadXML($svg, LIBXML_NONET); + libxml_clear_errors(); + libxml_use_internal_errors($previousErrors); + + if (!$loaded || $dom->documentElement === null) { + return ''; + } + + $xpath = new DOMXPath($dom); + + // Remove processing instructions (e.g. ). + // Document-level PIs are already excluded by saveXML($dom->documentElement), + // but PIs nested inside the root element are handled here. + $pis = $xpath->query('//processing-instruction()'); + if ($pis !== false) { + foreach (iterator_to_array($pis) as $pi) { + $pi->parentNode?->removeChild($pi); + } + } + + // Remove dangerous elements. Matching on the local name catches them + // regardless of any namespace prefix (e.g. ). + foreach (self::FORBIDDEN_ELEMENTS as $tag) { + $nodes = $xpath->query('//*[local-name() = "' . $tag . '"]'); + if ($nodes !== false) { + foreach (iterator_to_array($nodes) as $node) { + $node->parentNode?->removeChild($node); + } + } + } + + // Sanitise ' + . ''; + + $result = $this->sanitizer->sanitize($svg); + + $this->assertStringNotContainsString('tracker.example', $result); + $this->assertStringContainsString('rect { fill: red; stroke: blue }' + . ''; + + $result = $this->sanitizer->sanitize($svg); + + $this->assertStringContainsString('fill: red', $result); + } + + public function testRejectsXslTransformNamespace(): void { + $svg = '' + . '' + . '' + . ''; + + $this->assertSame('', $this->sanitizer->sanitize($svg)); + } + + public function testStripsProcessingInstructionsInsideRoot(): void { + $svg = '' + . '' + . ''; + + $result = $this->sanitizer->sanitize($svg); + + $this->assertStringNotContainsString('assertStringContainsString('assertSame('', $this->sanitizer->sanitize($svg)); + } + + public function testLooksLikeSvgWithDirectSvgTag(): void { + $this->assertTrue($this->sanitizer->looksLikeSvg('')); + } + + public function testLooksLikeSvgWithXmlPrologue(): void { + $this->assertTrue($this->sanitizer->looksLikeSvg('')); + } + + public function testLooksLikeSvgWithUtf8Bom(): void { + $this->assertTrue($this->sanitizer->looksLikeSvg("\xEF\xBB\xBF")); + } + + public function testLooksLikeSvgReturnsFalseForHtml(): void { + $this->assertFalse($this->sanitizer->looksLikeSvg('')); + } + + public function testLooksLikeSvgReturnsFalseForRasterImage(): void { + $this->assertFalse($this->sanitizer->looksLikeSvg("\x89PNG\r\n")); + } + + public function testLooksLikeSvgReturnsFalseForHtmlComment(): void { + $this->assertFalse($this->sanitizer->looksLikeSvg('')); + } +}