Browse Source

Only allow http/https URI schemes for enclosure and thumbnail URLs (#9272)

* Only allow http/https URI schemes for enclosure and thumbnail URLs

Feed enclosure URLs were stored and rendered without URI scheme validation:
SimplePie's scheme check only applies to URLs inside content HTML
(Sanitize::replace_urls()), not to enclosure metadata, so a `javascript:`
URL survived the whole pipeline and was served as a clickable link in the
article view, including through the Google Reader compatible API.

Add FreshRSS_http_Util::isAllowedUrlScheme() and apply it:
- at store time, when building enclosure attributes in FreshRSS_Feed
- at render time, in FreshRSS_Entry::content(), so entries already stored
  in existing databases are protected as well

https://github.com/FreshRSS/FreshRSS/security/advisories/GHSA-3fw6-j8m4-82vj

* Reuse existing SimplePie is_remote_uri() function

* Use more realistic test examples

* Add tests for protocol relative
And simplify tests further

---------

Co-authored-by: Alexandre Alapetite <alexandre@alapetite.fr>
Ali Gündoğar 14 hours ago
parent
commit
18dfada959
3 changed files with 104 additions and 6 deletions
  1. 4 3
      app/Models/Entry.php
  2. 5 3
      app/Models/Feed.php
  3. 95 0
      tests/app/Models/EntryTest.php

+ 4 - 3
app/Models/Entry.php

@@ -227,7 +227,8 @@ class FreshRSS_Entry extends Minz_Model {
 		$thumbnailAttribute = $this->attributeArray('thumbnail') ?? [];
 		if (!empty($thumbnailAttribute['url'])) {
 			$elink = $thumbnailAttribute['url'];
-			if (is_string($elink) && ($allowDuplicateEnclosures || !self::containsLink($content, $elink))) {
+			if (is_string($elink) && \SimplePie\Misc::is_remote_uri($elink) &&
+				($allowDuplicateEnclosures || !self::containsLink($content, $elink))) {
 				$content .= <<<HTML
 					<figure class="enclosure">
 						<p class="enclosure-content">
@@ -248,7 +249,7 @@ class FreshRSS_Entry extends Minz_Model {
 				continue;
 			}
 			$elink = $enclosure['url'] ?? '';
-			if ($elink == '' || !is_string($elink)) {
+			if ($elink == '' || !is_string($elink) || !\SimplePie\Misc::is_remote_uri($elink)) {
 				continue;
 			}
 			if (!$allowDuplicateEnclosures && self::containsLink($content, $elink)) {
@@ -269,7 +270,7 @@ class FreshRSS_Entry extends Minz_Model {
 			$content .= '<figure class="enclosure">';
 
 			foreach ($thumbnails as $thumbnail) {
-				if (is_string($thumbnail)) {
+				if (is_string($thumbnail) && \SimplePie\Misc::is_remote_uri($thumbnail)) {
 					$content .= '<p><img class="enclosure-thumbnail" src="' . $thumbnail . '" alt="" title="' . $etitle . '" /></p>';
 				}
 			}

+ 5 - 3
app/Models/Feed.php

@@ -841,7 +841,8 @@ class FreshRSS_Feed extends Minz_Model {
 			$content = html_only_entity_decode($item->get_content());
 
 			$attributeThumbnail = $item->get_thumbnail() ?? [];
-			if (empty($attributeThumbnail['url'])) {
+			if (empty($attributeThumbnail['url']) || !is_string($attributeThumbnail['url']) ||
+				!\SimplePie\Misc::is_remote_uri($attributeThumbnail['url'])) {
 				$attributeThumbnail['url'] = '';
 			}
 
@@ -851,7 +852,7 @@ class FreshRSS_Feed extends Minz_Model {
 			if (!empty($enclosures)) {
 				foreach ($enclosures as $enclosure) {
 					$elink = $enclosure->get_link();
-					if ($elink != '') {
+					if (is_string($elink) && $elink !== '' && \SimplePie\Misc::is_remote_uri($elink)) {
 						$etitle = $enclosure->get_title() ?? '';
 						$credits = $enclosure->get_credits() ?? null;
 						$description = $enclosure->get_description() ?? '';
@@ -894,7 +895,8 @@ class FreshRSS_Feed extends Minz_Model {
 
 						if (!empty($enclosure->get_thumbnails())) {
 							foreach ($enclosure->get_thumbnails() as $thumbnail) {
-								if ($thumbnail !== $attributeThumbnail['url']) {
+								if (is_string($thumbnail) && \SimplePie\Misc::is_remote_uri($thumbnail) &&
+									$thumbnail !== $attributeThumbnail['url']) {
 									$attributeEnclosure['thumbnails'][] = $thumbnail;
 								}
 							}

+ 95 - 0
tests/app/Models/EntryTest.php

@@ -0,0 +1,95 @@
+<?php
+declare(strict_types=1);
+
+final class EntryTest extends \PHPUnit\Framework\TestCase {
+
+	#[\Override]
+	public static function setUpBeforeClass(): void {
+		FreshRSS_Context::initSystem();
+	}
+
+	/**
+	 * Parse a raw RSS payload through the real feed processing pipeline.
+	 * @return list<FreshRSS_Entry>
+	 */
+	private static function entriesFromRss(string $rss): array {
+		$feed = new FreshRSS_Feed('http://example.net/feed.xml', validate: false);
+		$feed->_id(1);
+		$simplePie = new FreshRSS_SimplePieCustom();
+		$simplePie->enable_cache(false);
+		$simplePie->set_raw_data($rss);
+		self::assertTrue($simplePie->init());
+		return array_values(iterator_to_array($feed->loadEntries($simplePie)));
+	}
+
+	public function test_content_dropsUnsafeEnclosureAndThumbnailUrls(): void {
+		$rss = <<<XML
+			<?xml version="1.0" encoding="UTF-8"?>
+			<rss version="2.0" xmlns:media="http://search.yahoo.com/mrss/">
+				<channel>
+					<title>Malicious feed</title>
+					<link>https://example.net/</link>
+					<description>Feed with malicious enclosure URLs</description>
+					<item>
+						<title>Victim Article</title>
+						<link>https://example.net/article</link>
+						<guid isPermaLink="false">poc-001</guid>
+						<pubDate>Tue, 14 Nov 2023 20:13:20 +0000</pubDate>
+						<description>Hello</description>
+						<enclosure url="javascript:alert(document.domain)//" length="0" type="application/octet-stream" />
+						<enclosure url="https://example.com/podcast.mp3" length="1234" type="audio/mpeg" />
+						<enclosure url="//cdn.example.com/podcast2.mp3" length="1234" type="audio/mpeg" />
+						<media:content url="https://example.com/pic.jpg" type="image/jpeg">
+							<media:thumbnail url="javascript:alert(1)" />
+							<media:thumbnail url="https://example.com/thumb.jpg" />
+							<media:thumbnail url="//cdn.example.com/thumb2.jpg" />
+						</media:content>
+						<media:thumbnail url="javascript:alert(2)" />
+					</item>
+				</channel>
+			</rss>
+			XML;
+
+		$entries = self::entriesFromRss($rss);
+		self::assertCount(1, $entries);
+		$entry = $entries[0];
+		self::assertSame('Victim Article', $entry->title());
+
+		// The malicious `<media:thumbnail>` of the item must not be stored as an attribute
+		self::assertNull($entry->attributeArray('thumbnail'));
+
+		// The malicious enclosure must not even be stored as an attribute
+		$enclosureUrls = array_column($entry->attributeArray('enclosures') ?? [], 'url');
+		self::assertNotContains('javascript:alert(document.domain)//', $enclosureUrls);
+		self::assertContains('https://example.com/podcast.mp3', $enclosureUrls);
+		// SimplePie must absolutise protocol-relative URLs against the feed URL
+		self::assertContains('https://cdn.example.com/podcast2.mp3', $enclosureUrls);
+
+		$html = $entry->content();
+
+		self::assertStringNotContainsString('javascript:', $html);
+		self::assertStringContainsString('href="https://example.com/podcast.mp3"', $html);
+		self::assertStringContainsString('src="https://example.com/thumb.jpg"', $html);
+		self::assertStringContainsString('href="https://cdn.example.com/podcast2.mp3"', $html);
+		self::assertStringContainsString('src="https://cdn.example.com/thumb2.jpg"', $html);
+		self::assertStringContainsString('Hello', $html);
+	}
+
+	public function test_content_dropsUnsafeUrlsFromLegacyAttributes(): void {
+		$entry = new FreshRSS_Entry(1, 'poc-003', 'Victim Article', '', 'Hello', 'https://example.net/article');
+		$entry->_attributes([
+			'thumbnail' => ['url' => 'javascript:alert(1)'],
+			'enclosures' => [
+				['url' => 'javascript:alert(2)', 'title' => 'evil'],
+				['url' => 'https://example.com/podcast.mp3', 'thumbnails' => ['javascript:alert(3)', 'https://example.com/thumb.jpg']],
+			],
+		]);
+
+		$html = $entry->content();
+
+		self::assertStringNotContainsString('javascript:', $html);
+		self::assertStringContainsString('href="https://example.com/podcast.mp3"', $html);
+		self::assertStringContainsString('src="https://example.com/thumb.jpg"', $html);
+		self::assertStringContainsString('Hello', $html);
+	}
+}