Modified patch originally from Alexandre Erwin Ittner <alexandre@ittner.com.br>

lwindolf · lwindolf · commit 1981e1e161cd · 2023-03-09T23:16:37.000+01:00
Liferea feed auto-discovery feature can create subscriptions to command
feeds, allowing malicious websites to run arbitrary commands on the
local system.

For example, trying feed discovery in a site which returns a document
like this:

    &lt;!DOCTYPE html&gt;
    &lt;html&gt;
     &lt;head&gt;
      &lt;title&gt;Feed auto-discovery RCE PoC&lt;/title&gt;
      &lt;link rel="alternate" type="application/rss+xml" href="|date &amp;gt;/tmp/bad-feed-discovery.txt"&gt;
     &lt;/head&gt;
     &lt;body&gt;
     Oooops.
     &lt;/body&gt;
    &lt;/html&gt;

will cause Liferea to subscribe to command
"date &gt;/tmp/bad-feed-discovery.txt". The command runs before the user
has a chance of checking the subscription.

This happens because function common_build_url, used to normalize the
discovered URL candidates, fails to create an absolute URL if any of
the parts has some special characters (including "|"), returning the
original, non-escaped, relative URL instead.

- Filter out unwanted link strings
- Add testcase for HTML5 autodiscovery
- Add testcase for item links
diff --git a/.gitignore b/.gitignore
@@ -50,8 +50,9 @@ src/Liferea-3.0.gir
 src/Liferea-3.0.typelib
 src/tests/favicon
 src/tests/html_auto
-src/tests/parse_html
 src/tests/parse_date
+src/tests/parse_html
+src/tests/parse_rss
 src/tests/parse_xml
 src/tests/social
 xslt/*.xml
diff --git a/src/common.c b/src/common.c
@@ -138,7 +138,9 @@ common_uri_escape (const xmlChar *url)
 	g_assert (NULL != url);
 
 	/* xmlURIEscape returns NULL if spaces are in the URL,
-	   so we need to replace them first (see SF #2965158) */
+	   so we need to replace them first (see SF #2965158).
+	   TODO: perhaps replace xmlURIEscape with g_uri_escape_string ?
+	 */
 	tmp = (xmlChar *)common_strreplace (g_strdup ((gchar *)url), " ", "%20");
 	result = xmlURIEscape (tmp);
 	g_free (tmp);
diff --git a/src/feed.c b/src/feed.c
@@ -460,7 +460,7 @@ feed_get_node_type (void)
 		NODE_CAPABILITY_EXPORT |
 		NODE_CAPABILITY_EXPORT_ITEMS,
 		"feed",		/* not used, feed format ids are used instead */
-		NULL,
+		ICON_DEFAULT,
 		feed_import,
 		feed_export,
 		feed_load,
@@ -472,7 +472,6 @@ feed_get_node_type (void)
 		feed_properties,
 		feed_free
 	};
-	nti.icon = icon_get (ICON_DEFAULT);
 
 	return &nti;
 }
diff --git a/src/fl_sources/node_source.c b/src/fl_sources/node_source.c
@@ -1,7 +1,7 @@
 /*
  * @file node_source.c  generic node source provider implementation
  *
- * Copyright (C) 2005-2022 Lars Windolf <lars.windolf@gmx.de>
+ * Copyright (C) 2005-2023 Lars Windolf <lars.windolf@gmx.de>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -622,7 +622,7 @@ node_source_get_node_type (void)
 		/* derive the node source node type from the folder node type */
 		nodeType = (nodeTypePtr) g_new0 (struct nodeType, 1);
 		nodeType->id			= "source";
-		nodeType->icon			= icon_get (ICON_DEFAULT);
+		nodeType->icon			= ICON_DEFAULT;
 		nodeType->capabilities		= NODE_CAPABILITY_SHOW_UNREAD_COUNT |
 						  NODE_CAPABILITY_SHOW_ITEM_FAVICONS |
 						  NODE_CAPABILITY_UPDATE_CHILDS |
diff --git a/src/folder.c b/src/folder.c
@@ -119,7 +119,7 @@ folder_get_node_type (void)
 		NODE_CAPABILITY_UPDATE_CHILDS |
 		NODE_CAPABILITY_EXPORT,
 		"folder",
-		NULL,
+		ICON_FOLDER,
 		folder_import,
 		folder_export,
 		folder_load,
@@ -131,7 +131,6 @@ folder_get_node_type (void)
 		feed_list_view_rename_node,
 		NULL
 	};
-	fnti.icon = icon_get (ICON_FOLDER);
 
 	return &fnti;
 }
@@ -150,7 +149,7 @@ root_get_node_type (void)
 		NODE_CAPABILITY_UPDATE_CHILDS |
 		NODE_CAPABILITY_EXPORT,
 		"root",
-		NULL,		/* and no need for an icon */
+		0,		/* and no need for an icon */
 		folder_import,
 		folder_export,
 		folder_load,
diff --git a/src/html.c b/src/html.c
@@ -221,7 +221,7 @@ html_auto_discover_collect_meta (xmlNodePtr match, gpointer user_data)
 GSList *
 html_auto_discover_feed (const gchar* data, const gchar *defaultBaseUri)
 {
-	GSList		*iter, *links = NULL;
+	GSList		*iter, *links = NULL, *valid_links = NULL;
 	gchar		*baseUri = NULL;
 	xmlDocPtr	doc;
 	xmlNodePtr	node, root;
@@ -253,17 +253,25 @@ html_auto_discover_feed (const gchar* data, const gchar *defaultBaseUri)
 	/* Turn relative URIs into absolute URIs */
 	iter = links;
 	while (iter) {
-		gchar *tmp = iter->data;
-		iter->data = common_build_url (tmp, baseUri);
-		g_free (tmp);
-		debug1 (DEBUG_UPDATE, "search result: %s", (gchar *)iter->data);
+		gchar *tmp = (gchar *)common_build_url (iter->data, baseUri);
+
+		/* We expect only relative URIs starting with '/' or absolute URIs starting with 'http://' or 'https://' */
+		if ('h' == tmp[0] || '/' == tmp[0]) {
+			debug1 (DEBUG_UPDATE, "search result: %s", (gchar *)iter->data);
+			valid_links = g_slist_append (valid_links, tmp);
+		} else {
+			debug1 (DEBUG_UPDATE, "html_auto_discover_feed: discarding invalid URL %s", tmp ? tmp : "NULL");
+			g_free (tmp);
+		}
+
 		iter = g_slist_next (iter);
 	}
+	g_slist_free_full (links, g_free);
 
 	g_free (baseUri);
 	xmlFreeDoc (doc);
 
-	return links;
+	return valid_links;
 }
 
 GSList *
diff --git a/src/item.c b/src/item.c
@@ -140,7 +140,9 @@ void
 item_set_source (LifereaItem *item, const gchar * source)
 {
 	g_free (item->source);
-	if (source)
+
+	/* We expect only relative URIs starting with '/' or absolute URIs starting with 'http://' or 'https://' */
+	if (source && ('/' == source[0] || 'h' == source[0]))
 		item->source = g_strstrip (g_strdup (source));
 	else
 		item->source = NULL;
diff --git a/src/newsbin.c b/src/newsbin.c
@@ -217,7 +217,7 @@ newsbin_get_node_type (void)
 		                                  NODE_CAPABILITY_SHOW_ITEM_COUNT |
 		                                  NODE_CAPABILITY_EXPORT_ITEMS;
 		nodeType->id			= "newsbin";
-		nodeType->icon			= icon_get (ICON_NEWSBIN);
+		nodeType->icon			= ICON_NEWSBIN;
 		nodeType->load			= feed_get_node_type()->load;
 		nodeType->import		= newsbin_import;
 		nodeType->export		= newsbin_export;
diff --git a/src/node.c b/src/node.c
@@ -43,6 +43,7 @@
 #include "date.h"
 #include "fl_sources/node_source.h"
 #include "ui/feed_list_view.h"
+#include "ui/icons.h"
 #include "ui/liferea_shell.h"
 
 static GHashTable *nodes = NULL;	/*<< node id -> node lookup table */
@@ -431,7 +432,7 @@ gpointer
 node_get_icon (nodePtr node)
 {
 	if (!node->icon)
-		return (gpointer) NODE_TYPE(node)->icon;
+		return (gpointer) icon_get (NODE_TYPE(node)->icon);
 
 	return node->icon;
 }
diff --git a/src/node_type.h b/src/node_type.h
@@ -54,7 +54,7 @@ enum {
 typedef struct nodeType {
 	gulong		capabilities;	/**< bitmask of node type capabilities */
 	const gchar	*id;		/**< type id (used for type attribute in OPML export) */
-	const GIcon	*icon;		/**< default icon for nodes of this type (if no favicon available) */
+	guint		icon;		/**< default icon for nodes of this type (if no favicon available) */
 	
 	/* For method documentation see the wrappers defined below! 
 	   All methods are mandatory for each node type. */
diff --git a/src/tests/Makefile.am b/src/tests/Makefile.am
@@ -2,7 +2,7 @@
 
 noinst_PROGRAMS = $(TEST_PROGS)
 
-TEST_PROGS = parse_html favicon parse_date parse_xml social
+TEST_PROGS = parse_html favicon parse_date parse_rss parse_xml social
 
 test: $(TEST_PROGS)
 	echo $(TEST_PROGS) |\
@@ -93,6 +93,9 @@ parse_html_LDADD = $(favicon_LDADD)
 parse_date_CFLAGS = $(AM_CPPFLAGS)
 parse_date_LDADD = $(favicon_LDADD)
 
+parse_rss_CFLAGS = $(AM_CPPFLAGS)
+parse_rss_LDADD = $(favicon_LDADD)
+
 parse_xml_CFLAGS = $(AM_CPPFLAGS)
 parse_xml_LDADD = $(favicon_LDADD)
 
diff --git a/src/tests/parse_html.c b/src/tests/parse_html.c
@@ -1,7 +1,7 @@
 /**
- * @file html.c  Test cases for feed link auto discovery
+ * @file parse_html.c  Test cases for feed link auto discovery
  *
- * Copyright (C) 2014-2019 Lars Windolf <lars.windolf@gmx.de>
+ * Copyright (C) 2014-2023 Lars Windolf <lars.windolf@gmx.de>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -20,6 +20,7 @@
 
 #include <glib.h>
 
+#include "debug.h"
 #include "html.h"
 
 /* We need two groups of autodiscovery test cases, one for the tag soup fuzzy
@@ -115,6 +116,13 @@ gchar *tc_xml_atom3[] = {
 	NULL
 };
 
+// Injection via "|"" command must not result in command subscription
+gchar *tc_xml_rce[] = {
+	"<html><head><link rel=\"alternate\" type=\"application/rss+xml\" href=\"|date &gt;/tmp/bad-feed-discovery.txt\"></html>",
+	NULL,
+	NULL
+};
+
 /* HTML5 extraction test cases */
 
 gchar *tc_article[] = {
@@ -214,6 +222,9 @@ main (int argc, char *argv[])
 {
 	g_test_init (&argc, &argv, NULL);
 
+	if (argv[1] && g_str_equal (argv[1], "--debug"))
+		set_debug_level (DEBUG_UPDATE | DEBUG_HTML | DEBUG_PARSING);
+
 	g_test_add_data_func ("/html/auto_discover_link_xml", &tc_xml, &tc_auto_discover_link);
 	g_test_add_data_func ("/html/auto_discover_link_xml_base_url", &tc_xml_base_url, &tc_auto_discover_link);
 	g_test_add_data_func ("/html/auto_discover_link_rss", &tc_rss, &tc_auto_discover_link);
@@ -225,6 +236,7 @@ main (int argc, char *argv[])
 	g_test_add_data_func ("/html/auto_discover_link_xml_atom", &tc_xml_atom, &tc_auto_discover_link);
 	g_test_add_data_func ("/html/auto_discover_link_xml_atom2", &tc_xml_atom2, &tc_auto_discover_link);
 	g_test_add_data_func ("/html/auto_discover_link_xml_atom3", &tc_xml_atom3, &tc_auto_discover_link);
+	g_test_add_data_func ("/html/auto_discover_link_xml_rce", &tc_xml_rce, &tc_auto_discover_link);
 
 	g_test_add_data_func ("/html/html5_extract_article", &tc_article, &tc_get_article);
 	g_test_add_data_func ("/html/html5_extract_article_main", &tc_article_main, &tc_get_article);
diff --git a/src/tests/parse_rss.c b/src/tests/parse_rss.c
@@ -0,0 +1,128 @@
+/**
+ * @file parse_rss.c  Test cases for RSS parsing
+ *
+ * Copyright (C) 2023 Lars Windolf <lars.windolf@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <glib.h>
+#include <string.h>
+
+#include "debug.h"
+#include "feed.h"
+#include "feed_parser.h"
+#include "item.h"
+#include "subscription.h"
+#include "xml.h"
+
+/* Format of test cases:
+
+   1.     feed XML string
+   2.     "true" for successfully parsed feed, "false" for unparseable
+   3.     number of items
+   4..n   string of XML serialized items
+ */
+
+gchar *tc_rss_feed1[] = {
+	"<rss version=\"2.0\"><channel><title>T</title><link>http://localhost</link><item><title>i1</title><link>http://localhost/item1.html</link><description>D</description></item><item><title>i2</title><link>https://localhost/item2.html</link></item></channel></rss>",
+	"true",
+	"2",
+	"<item><title>i1</title><description>&lt;div xmlns=\"http://www.w3.org/1999/xhtml\"&gt;&lt;p&gt;D&lt;/p&gt;&lt;/div&gt;</description><source>http://localhost/item1.html</source><nr>0</nr><readStatus>0</readStatus><updateStatus>0</updateStatus><mark>0</mark><time>1678397817</time><sourceId/><sourceNr>0</sourceNr><attributes/></item>",
+	"<item><title>i2</title><source>https://localhost/item2.html</source><nr>0</nr><readStatus>0</readStatus><updateStatus>0</updateStatus><mark>0</mark><time>1678397817</time><sourceId/><sourceNr>0</sourceNr><attributes/></item>",
+	NULL
+};
+
+/* Test case to prevent | command injection in item link which could trigger
+   a HTML5 extraction */
+gchar *tc_rss_feed2_rce[] = {
+	"<rss version=\"2.0\"><channel><title>T</title><item><title>i1</title><link>|date >/tmp/bad-item-link.txt</link></item></channel></rss>",
+	"true",
+	"1",
+	"<item><title>i1</title><nr>0</nr><readStatus>0</readStatus><updateStatus>0</updateStatus><mark>0</mark><time>1678397817</time><sourceId/><sourceNr>0</sourceNr><attributes/></item>",
+	NULL
+};
+
+static void
+tc_parse_feed (gconstpointer user_data)
+{
+	gchar			**tc = (gchar **)user_data;
+	nodePtr			node;
+	feedParserCtxtPtr 	ctxt;
+	int			i;
+	GList			*iter;
+
+	node = node_new (feed_get_node_type ());
+	node_set_data (node, feed_new ());
+ 	node_set_subscription (node, subscription_new (NULL, NULL, NULL));
+	ctxt = feed_parser_ctxt_new (node->subscription, tc[0], strlen(tc[0]));
+
+	g_assert_cmpstr (feed_parse (ctxt)?"true":"false", ==, tc[1]);
+	g_assert (g_list_length (ctxt->items) == atoi(tc[2]));
+
+	i = 2;
+	iter = ctxt->items;
+	while (tc[++i]) {
+		gchar		*buffer, *tmp, *tmp2;
+		gint		buffersize;
+		xmlDocPtr	doc = xmlNewDoc (BAD_CAST"1.0");
+		xmlNodePtr	rootNode = xmlNewDocNode (doc, NULL, BAD_CAST"result", NULL);
+
+		xmlDocSetRootElement (doc, rootNode);
+
+		// Force time and delete <timestr> to make result compareable
+		itemPtr item = (itemPtr)iter->data;
+		item->time = 1678397817;
+		item_to_xml (item, rootNode);
+
+		xmlNode *timestr = xpath_find (rootNode, "//timestr");
+		if (timestr) {
+			xmlUnlinkNode (timestr);
+			xmlFreeNode (timestr);
+		}
+		xmlDocDumpMemory(doc, (xmlChar **)&buffer, &buffersize);
+
+		/* strip boilerplate */
+		tmp = buffer;
+		if ((tmp = strstr (tmp, "<result>")))
+			tmp += 8;		
+		if ((tmp2 = strstr (tmp, "</result>")))
+			*tmp2 = 0;
+
+		g_assert_cmpstr (tc[i], ==, tmp);
+
+		xmlFreeDoc (doc);
+		xmlFree (buffer);
+
+		iter = g_list_next (iter);
+	}	
+
+	feed_parser_ctxt_free (ctxt);
+	node_free (node);
+}
+
+int
+main (int argc, char *argv[])
+{
+	g_test_init (&argc, &argv, NULL);
+
+	if (argv[1] && g_str_equal (argv[1], "--debug"))
+		set_debug_level (DEBUG_UPDATE | DEBUG_HTML | DEBUG_PARSING);
+
+	g_test_add_data_func ("/rss/feed1",	&tc_rss_feed1,		&tc_parse_feed);
+	g_test_add_data_func ("/rss/feed2_rce",	&tc_rss_feed2_rce,	&tc_parse_feed);
+
+	return g_test_run();
+}
diff --git a/src/vfolder.c b/src/vfolder.c
@@ -308,7 +308,7 @@ vfolder_get_node_type (void)
 		NODE_CAPABILITY_SHOW_UNREAD_COUNT |
 		NODE_CAPABILITY_EXPORT_ITEMS,
 		"vfolder",
-		NULL,
+		ICON_VFOLDER,
 		vfolder_import,
 		vfolder_export,
 		vfolder_load,
@@ -320,7 +320,6 @@ vfolder_get_node_type (void)
 		vfolder_properties,
 		vfolder_free
 	};
-	nti.icon = icon_get (ICON_VFOLDER);
 
 	return &nti;
 }

Original file line number	Diff line number	Diff line change
`@@ -43,6 +43,7 @@`
`43`	`43`	`#include "date.h"`
`44`	`44`	`#include "fl_sources/node_source.h"`
`45`	`45`	`#include "ui/feed_list_view.h"`
	`46`	`+#include "ui/icons.h"`
`46`	`47`	`#include "ui/liferea_shell.h"`
`47`	`48`
`48`	`49`	`static GHashTable nodes = NULL; /<< node id -> node lookup table */`
`@@ -431,7 +432,7 @@ gpointer`
`431`	`432`	`node_get_icon (nodePtr node)`
`432`	`433`	`{`
`433`	`434`	`if (!node->icon)`
`434`		`- return (gpointer) NODE_TYPE(node)->icon;`
	`435`	`+ return (gpointer) icon_get (NODE_TYPE(node)->icon);`
`435`	`436`
`436`	`437`	`return node->icon;`
`437`	`438`	`}`