From 09c81da25109fe43837db4f2551394270cdb320c Mon Sep 17 00:00:00 2001 From: Acts1631 Date: Tue, 7 Jul 2026 21:32:48 -0400 Subject: [PATCH] ipc: fix D-Bus action denylist bypass via URL path/query ambiguity ipc_object_set_current_reference() rejects setCurrentReference calls that request the showStudypad or showImage actions, since those touch the local filesystem or spawn external programs and the D-Bus session bus is reachable by any local peer. The check extracted the action value with strstr(reference, "action=") on the raw reference, but main_url_handler() splits the URL at the first '?' and asks Sword's URL class to parse the action from the query string only. A reference such as passagestudy.jsp/action=showBookmark?action=showImage&value=evil.png has "action=showBookmark" appear first in the URL path, so the raw strstr() picks that up (and it doesn't match the denylist), while Sword's parser only looks at the query string after the '?' and resolves the real action to showImage, which then gets dispatched. Fix this by not re-implementing URL/query parsing a second time in ipc.c. Factor the re-encoding + Sword URL parsing that main_url_handler() already uses for dispatch into main_url_get_action(), and have the IPC gate call that instead. The check and the actual dispatch now always agree, since they're the same code path, so this class of parsing-asymmetry bypass isn't possible regardless of how the URL is structured. --- src/gtk/ipc.c | 52 +++++++++-------------- src/main/url.cc | 108 +++++++++++++++++++++++++++++++++++++----------- src/main/url.hh | 1 + 3 files changed, 105 insertions(+), 56 deletions(-) diff --git a/src/gtk/ipc.c b/src/gtk/ipc.c index 1b307c4f4..0407ce236 100644 --- a/src/gtk/ipc.c +++ b/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"))) { diff --git a/src/main/url.cc b/src/main/url.cc index a5ffae642..5ee39a416 100644 --- a/src/main/url.cc +++ b/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); diff --git a/src/main/url.hh b/src/main/url.hh index 4696773ef..4491c500e 100644 --- a/src/main/url.hh +++ b/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);