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);