Ignore security-relevant OPML attributes for dynamic OPML sources (#9276)

* Ignore security-relevant OPML attributes for dynamic OPML sources

OPML content can come from an untrusted source (a dynamic OPML category re-fetches its remote content on every refresh cycle). Such content must not be able to configure feed cURL parameters (CURLOPT_COOKIE, CURLOPT_PROXY, CURLOPT_POSTFIELDS, etc.) nor create further dynamic OPML categories. importOpml() therefore defaults to untrusted, and the local file import controller explicitly opts in.

* Review

---------

Co-authored-by: Alexandre Alapetite <alexandre@alapetite.fr>

app/Controllers/importExportController.php

@@ -123,7 +123,7 @@ class FreshRSS_importExport_Controller extends FreshRSS_ActionController {
 			if ($opml_file === false) {
 				continue;
 			}
-			$importService->importOpml($opml_file);
+			$importService->importOpml($opml_file, trusted_source: true);
 			if (!$importService->lastStatus()) {
 				$ok = false;
 				if (FreshRSS_Context::$isCli) {

app/Models/Category.php

@@ -224,7 +224,7 @@ class FreshRSS_Category extends Minz_Model {
 		} else {
 			$dryRunCategory = new FreshRSS_Category();
 			$importService = new FreshRSS_Import_Service();
-			$importService->importOpml($opml, $dryRunCategory, true);
+			$importService->importOpml($opml, $dryRunCategory, dry_run: true, trusted_source: false);
 			if ($importService->lastStatus()) {
 				$feedDAO = FreshRSS_Factory::createFeedDao();
 				$limits = FreshRSS_Context::systemConf()->limits;

app/Services/ImportService.php

@@ -32,8 +32,11 @@ class FreshRSS_Import_Service {
 	 * @param string $opml_file the OPML file content.
 	 * @param FreshRSS_Category|null $forced_category force the feeds to be associated to this category.
 	 * @param bool $dry_run true to not create categories and feeds in database.
+	 * @param bool $trusted_source true when the OPML content is explicitly provided by the user (e.g. local file import);
+	 *        false by default, and for content fetched from a remote source (e.g. dynamic OPML categories), in which case
+	 *        security-relevant attributes (feed cURL parameters, nested dynamic categories) are ignored.
 	 */
-	public function importOpml(string $opml_file, ?FreshRSS_Category $forced_category = null, bool $dry_run = false): void {
+	public function importOpml(string $opml_file, ?FreshRSS_Category $forced_category = null, bool $dry_run = false, bool $trusted_source = false): void {
 		if (function_exists('set_time_limit')) {
 			@set_time_limit(300);
 		}
@@ -101,7 +104,7 @@ class FreshRSS_Import_Service {
 
 				if ($can_create_category) {
 					// Import category in the exact order as the outline's placement in the OPML, at the end of positioned categories
-					$category = $this->createCategory($category_element, $dry_run, ++$highest_position);
+					$category = $this->createCategory($category_element, $dry_run, ++$highest_position, $trusted_source);
 					if ($category !== null) {
 						$categories_by_names[$category->name()] = $category;
 						$nb_categories++;
@@ -132,7 +135,7 @@ class FreshRSS_Import_Service {
 					break;
 				}
 
-				if ($this->createFeed($feed_element, $category, $dry_run) !== null) {
+				if ($this->createFeed($feed_element, $category, $dry_run, $trusted_source) !== null) {
 					// TODO what if the feed already exists in the database?
 					$nb_feeds++;
 				} else {
@@ -148,9 +151,10 @@ class FreshRSS_Import_Service {
 	 * @param array<string,string> $feed_elt An OPML element (must be a feed element).
 	 * @param FreshRSS_Category $category The category to associate to the feed.
 	 * @param bool $dry_run true to not create the feed in database.
+	 * @param bool $trusted_source false to ignore security-relevant attributes (feed cURL parameters); see {@see FreshRSS_Import_Service::importOpml()}.
 	 * @return FreshRSS_Feed|null The created feed, or null if it failed.
 	 */
-	private function createFeed(array $feed_elt, FreshRSS_Category $category, bool $dry_run): ?FreshRSS_Feed {
+	private function createFeed(array $feed_elt, FreshRSS_Category $category, bool $dry_run, bool $trusted_source = false): ?FreshRSS_Feed {
 		$url = Minz_Helper::htmlspecialchars_utf8($feed_elt['xmlUrl']);
 		$name = $feed_elt['text'] ?? $feed_elt['title'] ?? '';
 		$name = Minz_Helper::htmlspecialchars_utf8($name);
@@ -338,7 +342,9 @@ class FreshRSS_Import_Service {
 			if (isset($feed_elt['frss:CURLOPT_USERAGENT'])) {
 				$curl_params[CURLOPT_USERAGENT] = $feed_elt['frss:CURLOPT_USERAGENT'];
 			}
-			if (!empty($curl_params)) {
+			// Feed cURL parameters are only honored for OPML content explicitly provided by the user;
+			// a remote (dynamic) OPML must not be able to configure the cURL behavior of the instance.
+			if ($trusted_source && !empty($curl_params)) {
 				$feed->_attribute('curl_params', FreshRSS_http_Util::sanitizeCurlParams($curl_params));
 			}
 
@@ -379,14 +385,15 @@ class FreshRSS_Import_Service {
 	 *
 	 * @param array<string,string> $category_element An OPML element (must be a category element).
 	 * @param bool $dry_run true to not create the category in database.
+	 * @param bool $trusted_source false to ignore security-relevant attributes (dynamic OPML); see {@see FreshRSS_Import_Service::importOpml()}.
 	 * @return FreshRSS_Category|null The created category, or null if it failed.
 	 */
-	private function createCategory(array $category_element, bool $dry_run, int $position): ?FreshRSS_Category {
+	private function createCategory(array $category_element, bool $dry_run, int $position, bool $trusted_source = false): ?FreshRSS_Category {
 		$name = $category_element['text'] ?? $category_element['title'] ?? '';
 		$name = Minz_Helper::htmlspecialchars_utf8($name);
 		$category = new FreshRSS_Category($name);
 
-		if (isset($category_element['frss:opmlUrl'])) {
+		if ($trusted_source && isset($category_element['frss:opmlUrl'])) {
 			$opml_url = FreshRSS_http_Util::checkUrl($category_element['frss:opmlUrl']);
 			if ($opml_url != '') {
 				$category->_kind(FreshRSS_Category::KIND_DYNAMIC_OPML);

p/api/greader.php

@@ -325,7 +325,7 @@ final class GReaderAPI {
 	private static function subscriptionImport(string $opml): never {
 		$user = Minz_User::name() ?? Minz_User::INTERNAL_USER;
 		$importService = new FreshRSS_Import_Service($user);
-		$importService->importOpml($opml);
+		$importService->importOpml($opml, trusted_source: true);
 		if ($importService->lastStatus()) {
 			FreshRSS_feed_Controller::actualizeFeedsAndCommit();
 			invalidateHttpCache($user);