ipc: fix D-Bus action denylist bypass via URL path/query ambiguity

src/gtk/ipc.c

@@ -222,12 +222,25 @@ gboolean ipc_object_set_current_reference(IpcObject *obj,
 	 * use one of the known navigation schemes/markers and must not request
 	 * a local-file action.  Reject everything else (fail closed).
 	 *
-	 * The action check has to look at the decoded action, not the raw
-	 * reference bytes: a reference like
-	 * "passagestudy.jsp?action=show%53tudypad" contains no literal
-	 * "showStudypad" substring, but libsword's URL::getParameterValue
-	 * percent-decodes the action before main_url_handler dispatches it,
-	 * so a substring denylist on the raw reference is bypassable.
+	 * The action check must look at whatever action main_url_handler()
+	 * will actually dispatch to, not at a hand-rolled re-parse of the
+	 * raw reference bytes.  A previous version of this check used
+	 * strstr(reference, "action=") directly on the raw reference, which
+	 * disagreed with main_url_handler()'s Sword-URL-based parsing in two
+	 * different ways:
+	 *   - percent-encoding: "passagestudy.jsp?action=show%53tudypad" has
+	 *     no literal "showStudypad" substring, but Sword's
+	 *     URL::getParameterValue() percent-decodes the action before
+	 *     dispatch;
+	 *   - path/query ambiguity: "passagestudy.jsp/action=showBookmark?
+	 *     action=showImage&value=..." makes strstr() find the "action="
+	 *     that sits in the URL *path* while Sword's parser -- and hence
+	 *     the actual dispatch -- only looks at the *query string*, and
+	 *     picks up the real, later "action=showImage".
+	 * main_url_get_action() (main/url.cc) reuses the exact same
+	 * re-encoding + Sword URL parsing that main_url_handler() uses to
+	 * dispatch, so the check below can never disagree with the actual
+	 * dispatch again.
 	 */
 	if (!reference ||
 	    (!g_strstr_len(reference, -1, "sword://") &&
@@ -239,32 +252,7 @@ gboolean ipc_object_set_current_reference(IpcObject *obj,
 		return FALSE;
 	}
 	{
-		const char *p = strstr(reference, "action=");
-		gchar *action = NULL;
-		if (p) {
-			p += 7; /* strlen("action=") */
-			const char *end = p;
-			while (*end && *end != '&' && *end != '#')
-				end++;
-			gsize len = end - p;
-			action = g_malloc(len + 1);
-			gsize i = 0, j = 0;
-			while (i < len) {
-				if (p[i] == '+') {
-					action[j++] = ' ';
-					i++;
-				} else if (p[i] == '%' && i + 2 < len &&
-					   g_ascii_isxdigit(p[i+1]) &&
-					   g_ascii_isxdigit(p[i+2])) {
-					gchar hex[3] = { p[i+1], p[i+2], 0 };
-					action[j++] = (gchar)strtoul(hex, NULL, 16);
-					i += 3;
-				} else {
-					action[j++] = p[i++];
-				}
-			}
-			action[j] = '\0';
-		}
+		gchar *action = main_url_get_action(reference);
 		if (action && *action &&
 		    (!strcmp(action, "showStudypad") ||
 		     !strcmp(action, "showImage"))) {

src/main/url.cc

@@ -845,6 +845,88 @@ gint sword_uri(const gchar *url, gboolean clicked)
 	return 1;
 }
 
+/**
+ * passagestudy_query_reencode:
+ * @url: a passagestudy.jsp/xiphos.url style URL
+ *
+ * Sword's URL class treats '/', ':' and ' ' in the query string
+ * specially, so main_url_handler() -- and anyone else who needs to know,
+ * in advance, which action a URL will dispatch to -- must re-encode them
+ * exactly the same way before handing the URL to Sword's URL parser.
+ * This is factored out into a single helper so the IPC security gate in
+ * ipc.c (via main_url_get_action(), below) and the actual dispatch here
+ * always see the identical query string and can never disagree about
+ * what "action" means.
+ *
+ * Return value: a newly allocated GString holding the URL up to and
+ * including the first '?', followed by the re-encoded query string, or
+ * NULL if @url has no '?' (no query string).  Caller must
+ * g_string_free() the result.
+ */
+static GString *passagestudy_query_reencode(const gchar *url)
+{
+	gchar *place = (gchar *)strchr(url, '?'); // url's beginning, as-is.
+	if (!place)
+		return NULL;
+
+	GString *tmpstr = g_string_new(NULL);
+	++place;
+	tmpstr = g_string_append_len(tmpstr, url, place - url);
+	for (/* */; *place; ++place) {
+		switch (*place) {
+		case '/':
+			tmpstr = g_string_append(tmpstr, "%2F");
+			break;
+		case ':':
+			tmpstr = g_string_append(tmpstr, "%3A");
+			break;
+		case ' ':
+			tmpstr = g_string_append(tmpstr, "%20");
+			break;
+		default:
+			tmpstr = g_string_append_c(tmpstr, *place);
+		}
+	}
+	return tmpstr;
+}
+
+/******************************************************************************
+ * Name
+ *   main_url_get_action
+ *
+ * Synopsis
+ *   #include "main/url.hh"
+ *
+ *   gchar *main_url_get_action(const gchar *url)
+ *
+ * Description
+ *   Extracts the "action" query parameter using the exact same parsing
+ *   path (query re-encoding + Sword's URL class) that main_url_handler()
+ *   itself uses to select which action to dispatch.  Any code that needs
+ *   to decide whether a URL is safe to hand to main_url_handler() --
+ *   such as the D-Bus IPC security gate in ipc.c -- must use this
+ *   function rather than re-implementing URL/query parsing, so the
+ *   check and the actual dispatch can never disagree on what the
+ *   action is.
+ *
+ * Return value
+ *   gchar * (newly allocated, g_free() by caller), or NULL if @url has
+ *   no query string or no "action" parameter.
+ */
+gchar *main_url_get_action(const gchar *url)
+{
+	GString *tmpstr = passagestudy_query_reencode(url);
+	if (!tmpstr)
+		return NULL;
+
+	URL m_url((const char *)tmpstr->str);
+	const char *action = m_url.getParameterValue("action");
+	gchar *result = (action && *action) ? g_strdup(action) : NULL;
+
+	g_string_free(tmpstr, TRUE);
+	return result;
+}
+
 /******************************************************************************
  * Name
  *   main_url_handler
@@ -887,31 +969,9 @@ gint main_url_handler(const gchar *url, gboolean clicked)
 		   strstr(url, "xiphos.url")) {
 
 		// another minor nightmare: re-encode / and : in hex.
-		gchar *place;
-		GString *tmpstr = g_string_new(NULL);
-
-		place = (char *)strchr(url, '?'); // url's beginning, as-is.
-		if (!place) {
-			g_string_free(tmpstr, TRUE);
+		GString *tmpstr = passagestudy_query_reencode(url);
+		if (!tmpstr)
 			return 0;
-		}
-		++place;
-		tmpstr = g_string_append_len(tmpstr, url, place - url);
-		for (/* */; *place; ++place) {
-			switch (*place) {
-			case '/':
-				tmpstr = g_string_append(tmpstr, "%2F");
-				break;
-			case ':':
-				tmpstr = g_string_append(tmpstr, "%3A");
-				break;
-			case ' ':
-				tmpstr = g_string_append(tmpstr, "%20");
-				break;
-			default:
-				tmpstr = g_string_append_c(tmpstr, *place);
-			}
-		}
 
 		/* passagestudy.jsp?action=showStrongs&type= */
 		URL m_url((const char *)tmpstr->str);

src/main/url.hh

@@ -37,6 +37,7 @@ enum {
 //gint main_url_handler_gecko(const gchar * url);
 gint sword_uri(const gchar *url, gboolean clicked);
 gint main_url_handler(const gchar *url, gboolean clicked);
+gchar *main_url_get_action(const gchar *url);
 gint main_main_get_mod_type_from_url(const gchar *url);
 const gchar *main_url_encode(const gchar *pram);
 GString *hex_decode(const gchar *url);