pidgin/pidgin

Change how we handle clicking on file:// links on Windows.
release-2.x.y
2014-01-12, Mark Doliner
b2571530fa8b
Parents 4e2416683223
Children 712a68049062
Change how we handle clicking on file:// links on Windows.

Previously we attempted to exec the file. This can be dangerous if
someone sends you a link to a malicious remote file. For now we're
going to open Explorer at the file's location. The user can decide
from there what they want to do--hopefully it'll be more obvious
what they're exec'ing and they can make a more informed decision.

This was a pretty easy change. We already had code to launch explorer.exe
that Eion wrote in this commit:
https://hg.pidgin.im/pidgin/main/rev/4377067bda01

But due to a bug it was only getting triggered if the URI was
"file://file://something"

A possibly better approach is for us to show an "are you sure you
want to do this?" prompt. I don't want to do that in 2.x.y, but
we could do it in default.


REGARDING ESCAPING
We weren't correctly escaping the file path that we passed to explorer.exe.
I believe this would have allowed a remote users to craft links that pass
arbitrary parameters to explorer.exe. I think it is not possible to craft
links that would exec other commands, and the arguments to explorer.exe
look fairly innocuous, so I don't think this is a major problem. But of
course we should fix it--we want to dictate how file:// are opened, we
don't want remote users to be able to dictate this.

The old code called g_shell_quote() to attempt to escape the URI, but it
didn't actually use the return value. Additionally g_shell_quote()
doesn't do what we want. It wrapps the string in single quotes and
escapes single quotes with a backslash. We really just want to escape
double quotes with a double quote.

Incidentally, explorer.exe argument parsing is bat shit crazy [1]. Args
are separated by commas or equals (not spaces). Double-quotes can be
used to wrap an argument but somehow double-quotes within an argument are
ignored. If the first field in an argument is not '/' then the entire
thing is interpreted as a path (until the next comma or equals, I guess?)
For something that's been around for 20 years and is a core piece of
the OS you'd think it would have half-way respectable argument parsing.
Then again, it's Windows.

[1] http://www.geoffchappell.com/studies/windows/shell/explorer/cmdline.htm
2 files changed, 31 insertions(+), 31 deletions(-)
  • +3 -0
    ChangeLog
  • +28 -31
    pidgin/gtkutils.c
    • --- a/ChangeLog Sat Jan 11 23:00:56 2014 -0800
    +++ b/ChangeLog Sun Jan 12 00:50:02 2014 -0800
    @@ -16,6 +16,9 @@
    * Fix handling of multibyte UTF-8 characters in smiley themes. (#15756)
    Windows-Specific Changes:
    + * When clicking file:// links, show the file in Explorer rather than
    + attempting to run the file. This reduces the chances of a user
    + clicking on a link and mistakenly running a malicious file.
    * Fix Tcl scripts. (#15520)
    * Fix crash-on-startup when ASLR is always on. (#15521)
    * Updates to dependencies:
    --- a/pidgin/gtkutils.c Sat Jan 11 23:00:56 2014 -0800
    +++ b/pidgin/gtkutils.c Sun Jan 12 00:50:02 2014 -0800
    @@ -3267,33 +3267,28 @@
    return TRUE;
    }
    +/**
    + * @param filename The path to a file. Specifically this is the link target
    + * from a link in an IM window with the leading "file://" removed.
    + */
    static void
    -file_open_uri(GtkIMHtml *imhtml, const char *uri)
    +open_file(GtkIMHtml *imhtml, const char *filename)
    {
    /* Copied from gtkft.c:open_button_cb */
    #ifdef _WIN32
    /* If using Win32... */
    int code;
    - if (purple_str_has_prefix(uri, "file://"))
    - {
    - gchar *escaped = g_shell_quote(uri);
    - gchar *param = g_strconcat("/select,\"", uri, "\"", NULL);
    - wchar_t *wc_param = g_utf8_to_utf16(param, -1, NULL, NULL, NULL);
    -
    - code = (int)ShellExecuteW(NULL, L"OPEN", L"explorer.exe", wc_param, NULL, SW_NORMAL);
    -
    - g_free(wc_param);
    - g_free(param);
    - g_free(escaped);
    - } else {
    - wchar_t *wc_filename = g_utf8_to_utf16(
    - uri, -1, NULL, NULL, NULL);
    -
    - code = (int)ShellExecuteW(NULL, NULL, wc_filename, NULL, NULL,
    - SW_SHOW);
    -
    - g_free(wc_filename);
    - }
    + /* Escape URI by replacing double-quote with 2 double-quotes. */
    + gchar *escaped = purple_strreplace(filename, "\"", "\"\"");
    + gchar *param = g_strconcat("/select,\"", escaped, "\"", NULL);
    + wchar_t *wc_param = g_utf8_to_utf16(param, -1, NULL, NULL, NULL);
    +
    + /* TODO: Better to use SHOpenFolderAndSelectItems()? */
    + code = (int)ShellExecuteW(NULL, L"OPEN", L"explorer.exe", wc_param, NULL, SW_NORMAL);
    +
    + g_free(wc_param);
    + g_free(param);
    + g_free(escaped);
    if (code == SE_ERR_ASSOCINCOMPLETE || code == SE_ERR_NOASSOC)
    {
    @@ -3304,7 +3299,8 @@
    {
    purple_notify_error(imhtml, NULL,
    _("An error occurred while opening the file."), NULL);
    - purple_debug_warning("gtkutils", "filename: %s; code: %d\n", uri, code);
    + purple_debug_warning("gtkutils", "filename: %s; code: %d\n",
    + filename, code);
    }
    #else
    char *command = NULL;
    @@ -3313,15 +3309,15 @@
    if (purple_running_gnome())
    {
    - char *escaped = g_shell_quote(uri);
    + char *escaped = g_shell_quote(filename);
    command = g_strdup_printf("gnome-open %s", escaped);
    g_free(escaped);
    }
    else if (purple_running_kde())
    {
    - char *escaped = g_shell_quote(uri);
    -
    - if (purple_str_has_suffix(uri, ".desktop"))
    + char *escaped = g_shell_quote(filename);
    +
    + if (purple_str_has_suffix(filename, ".desktop"))
    command = g_strdup_printf("kfmclient openURL %s 'text/plain'", escaped);
    else
    command = g_strdup_printf("kfmclient openURL %s", escaped);
    @@ -3329,7 +3325,7 @@
    }
    else
    {
    - purple_notify_uri(NULL, uri);
    + purple_notify_uri(NULL, filename);
    return;
    }
    @@ -3339,7 +3335,7 @@
    if (!g_spawn_command_line_sync(command, NULL, NULL, &exit_status, &error))
    {
    tmp = g_strdup_printf(_("Error launching %s: %s"),
    - uri, error->message);
    + filename, error->message);
    purple_notify_error(imhtml, NULL, _("Unable to open file."), tmp);
    g_free(tmp);
    g_error_free(error);
    @@ -3360,8 +3356,9 @@
    static gboolean
    file_clicked_cb(GtkIMHtml *imhtml, GtkIMHtmlLink *link)
    {
    - const char *uri = gtk_imhtml_link_get_url(link) + FILELINKSIZE;
    - file_open_uri(imhtml, uri);
    + /* Strip "file://" from the URI. */
    + const char *filename = gtk_imhtml_link_get_url(link) + FILELINKSIZE;
    + open_file(imhtml, filename);
    return TRUE;
    }
    @@ -3369,7 +3366,7 @@
    open_containing_cb(GtkIMHtml *imhtml, const char *url)
    {
    char *dir = g_path_get_dirname(url + FILELINKSIZE);
    - file_open_uri(imhtml, dir);
    + open_file(imhtml, dir);
    g_free(dir);
    return TRUE;
    }