Use a keyed hash for favicon cache keys (#9518)

* Use keyed hash (HMAC-SHA256) for favicon cache keys

The favicon cache key was a CRC-32 checksum of the salt-prefixed feed
website URL (or feed icon URL). CRC-32 is linear, so collisions for
equal-length inputs are independent of the salt prefix: a user could
compute offline a website URL for their own feed colliding into
another feed's cache slot, overwrite the shared
data/favicons/<hash>.txt, and have the unauthenticated public favicon
endpoint fetch and serve their image to every user of the instance.

Replace the checksum with a keyed hash (HMAC-SHA256, instance salt as
key, truncated to 16 hex chars) so the cache key cannot be forged
without the secret salt.

* Preserve legacy custom favicons when upgrading cache keys

* Base64 hash (shorter) and native copy

---------

Co-authored-by: Alexandre Alapetite <alexandre@alapetite.fr>
Co-authored-by: Inverle <inverle@proton.me>

app/Models/Feed.php

@@ -264,7 +264,10 @@ class FreshRSS_Feed extends Minz_Model {
 				$params = $feedIconUrl !== '' ? $feedIconUrl . $this->proxyParam()
 					: $this->website(fallback: true) . $this->proxyParam();
 			}
-			$this->hashFavicon = hash('crc32b', $salt . (is_string($params) ? $params : ''));
+			$hash = hash_hmac('sha256', is_string($params) ? $params : '', $salt, binary: true);
+			$hash = substr($hash, 0, 8);	// Keep 64 bits
+			$hash = strtr(rtrim(base64_encode($hash), '='), '+/', '-_');	// URL-safe base64
+			$this->hashFavicon = $hash;	// Length 11
 		}
 		return $this->hashFavicon;
 	}
@@ -444,6 +447,7 @@ class FreshRSS_Feed extends Minz_Model {
 	public function faviconPrepare(bool $force = false): void {
 		require_once LIB_PATH . '/favicons.php';
 		if ($this->customFavicon()) {
+			$this->migrateCustomFavicon();
 			return;
 		}
 		$feedIconUrl = $this->attributeString('feedIconUrl') ?? '';
@@ -476,8 +480,29 @@ class FreshRSS_Feed extends Minz_Model {
 		}
 	}
 
+	/** Legacy favicon migration from FreshRSS < 1.31.0 */
+	private function migrateCustomFavicon(): void {
+		if (!$this->customFavicon()) {
+			return;
+		}
+		$salt = FreshRSS_Context::systemConf()->salt;
+		$params = $this->id . Minz_User::name();
+		$hookParams = Minz_ExtensionManager::callHook(Minz_HookType::CustomFaviconHash, $this);
+		if ($hookParams !== null) {
+			$params = $hookParams;
+		}
+		$newPath = FAVICONS_DIR . $this->hashFavicon() . '.ico';
+		if (!file_exists($newPath)) {
+			$legacyHash = hash('crc32b', $salt . (is_string($params) ? $params : ''));
+			$legacyPath = FAVICONS_DIR . $legacyHash . '.ico';
+			if (is_file($legacyPath)) {
+				@copy($legacyPath, $newPath);
+			}
+		}
+	}
+
 	public static function faviconDelete(string $hash): void {
-		if (!ctype_xdigit($hash)) {
+		if (preg_match('/\\A[A-Za-z0-9_-]{1,11}\\z/', $hash) !== 1) {
 			return;
 		}
 		$path = DATA_PATH . '/favicons/' . $hash;
@@ -485,6 +510,8 @@ class FreshRSS_Feed extends Minz_Model {
 		@unlink($path . '.txt');
 	}
 	public function favicon(bool $absolute = false): string {
+		require_once LIB_PATH . '/favicons.php';
+		$this->migrateCustomFavicon();
 		$hash = $this->hashFavicon();
 		$url = '/f.php?h=' . $hash;
 		if ($this->customFavicon()

docs/en/developers/03_Backend/05_Extensions.md

@@ -181,7 +181,7 @@ The following events are available:
 * `Minz_HookType::CustomFaviconBtnUrl` (`function(FreshRSS_Feed $feed): string | null`): Allows extensions to implement a button for setting a custom favicon for individual feeds by providing an URL. The URL will be sent a POST request with the `extAction` field set to either `query_icon_info` or `update_icon`, along with an `id` field which describes the feed's ID.
 Example response for a `query_icon_info` request:
 ```json
-{"extName":"YouTube Video Feed","iconUrl":"..\/f.php?h=40838a43"}
+{"extName":"YouTube Video Feed","iconUrl":"../f.php?h=7fYbyMvIWCI"}
 ```
 * `Minz_HookType::CustomFaviconHash` (`function(FreshRSS_Feed $feed): string | null`): Enables the modification of custom favicon hashes by returning params from the hook function. The hook should check if the `customFaviconExt` attribute of `$feed` is set to the extension's name before returning a custom value. Otherwise, the return value should be null.
 * `Minz_HookType::EntriesFavorite` (`function(array $ids, bool $is_favorite): void`):

p/api/fever.php

@@ -390,6 +390,9 @@ final class FeverAPI
 			if ($feed->priority() <= FreshRSS_Feed::PRIORITY_HIDDEN) {
 				continue;
 			}
+			if ($feed->customFavicon()) {
+				$feed->faviconPrepare();
+			}
 			$id = $feed->hashFavicon();
 			$filename = DATA_PATH . '/favicons/' . $id . '.ico';
 			if (!file_exists($filename)) {

p/f.php

@@ -27,7 +27,7 @@ function show_default_favicon(int $cacheSeconds = 3600): void {
 }
 
 $id = $_GET['h'] ?? '0';
-if (!is_string($id) || !ctype_xdigit($id)) {
+if (!is_string($id) || preg_match('/\\A[A-Za-z0-9_-]{1,11}\\z/', $id) !== 1) {
 	$id = '0';
 }
 

tests/app/Models/FeedTest.php

@@ -43,4 +43,72 @@ final class FeedTest extends \PHPUnit\Framework\TestCase {
 			'system proxy' => ['http://192.168.1.2/feed', 'http://192.168.1.2/feed', 'proxy.example.test:3128'],
 		];
 	}
+
+	public function test_hashFavicon_isKeyedBySalt(): void {
+		FreshRSS_Context::initSystem();
+		$systemConf = FreshRSS_Context::systemConf();
+		$originalSalt = $systemConf->salt;
+		$systemConf->_attribute('salt', 'test-instance-salt');
+
+		try {
+			$feed = new FreshRSS_Feed('http://example.net/feed.xml', validate: false);
+			$feed->_website('https://example.com/', validate: false);
+
+			$hash = $feed->hashFavicon();
+			self::assertMatchesRegularExpression('/^[A-Za-z0-9_-]{11}$/', $hash);
+			$hmac = substr(hash_hmac('sha256', 'https://example.com/' . $feed->proxyParam(), 'test-instance-salt', true), 0, 8);
+			self::assertSame(rtrim(strtr(base64_encode($hmac), '+/', '-_'), '='), $hash);
+			$feed->_website('https://example.org/', validate: false);
+			self::assertNotSame($hash, $feed->hashFavicon());
+			$hash = $feed->hashFavicon();
+
+			$systemConf->_attribute('salt', 'another-instance-salt');
+			self::assertNotSame($hash, $feed->hashFavicon(skipCache: true));
+		} finally {
+			$systemConf->_attribute('salt', $originalSalt);
+		}
+	}
+
+	public function test_favicon_migratesLegacyCustomIconWithoutDeletingSharedSource(): void {
+		FreshRSS_Context::initSystem();
+		require_once LIB_PATH . '/favicons.php';
+		$feed = new FreshRSS_Feed('http://example.net/feed.xml', validate: false);
+		$feed->_id(987654);
+		$contents = file_get_contents(DEFAULT_FAVICON);
+		self::assertIsString($contents);
+		$newPath = $feed->setCustomFavicon($contents, updateFeed: false);
+		self::assertIsString($newPath);
+		unlink($newPath);
+		$legacyPath = FAVICONS_DIR . hash('crc32b', FreshRSS_Context::systemConf()->salt . $feed->id() . Minz_User::name()) . '.ico';
+		file_put_contents($legacyPath, $contents);
+
+		$feed->favicon();
+
+		self::assertSame($contents, file_get_contents($newPath));
+		self::assertSame($contents, file_get_contents($legacyPath));
+		unlink($legacyPath);
+		unlink($newPath);
+	}
+
+	public function test_favicon_migrationDoesNotOverwriteDestination(): void {
+		FreshRSS_Context::initSystem();
+		require_once LIB_PATH . '/favicons.php';
+		$feed = new FreshRSS_Feed('http://example.net/feed.xml', validate: false);
+		$feed->_id(987655);
+		$contents = file_get_contents(DEFAULT_FAVICON);
+		self::assertIsString($contents);
+		$newPath = $feed->setCustomFavicon($contents, updateFeed: false);
+		self::assertIsString($newPath);
+		$destination = 'destination-icon';
+		file_put_contents($newPath, $destination);
+		$legacyPath = FAVICONS_DIR . hash('crc32b', FreshRSS_Context::systemConf()->salt . $feed->id() . Minz_User::name()) . '.ico';
+		file_put_contents($legacyPath, $contents);
+
+		$feed->favicon();
+
+		self::assertSame($destination, file_get_contents($newPath));
+		self::assertSame($contents, file_get_contents($legacyPath));
+		unlink($legacyPath);
+		unlink($newPath);
+	}
 }