Change how we handle clicking on file:// links on Windows. release-2.x.y
authorMark Doliner <mark@kingant.net>
Sun, 12 Jan 2014 00:50:02 -0800
branchrelease-2.x.y
changesetb2571530fa8b pushlog
parent 4e2416683223
child 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
ChangeLog
pidgin/gtkutils.c
      1.1 --- a/ChangeLog
      1.2 +++ b/ChangeLog
      1.3 @@ -16,6 +16,9 @@
      1.4  	* Fix handling of multibyte UTF-8 characters in smiley themes. (#15756)
      1.5  
      1.6  	Windows-Specific Changes:
      1.7 +	* When clicking file:// links, show the file in Explorer rather than
      1.8 +	  attempting to run the file. This reduces the chances of a user
      1.9 +	  clicking on a link and mistakenly running a malicious file.
     1.10  	* Fix Tcl scripts. (#15520)
     1.11  	* Fix crash-on-startup when ASLR is always on. (#15521)
     1.12  	* Updates to dependencies:
      2.1 --- a/pidgin/gtkutils.c
      2.2 +++ b/pidgin/gtkutils.c
      2.3 @@ -3267,33 +3267,28 @@
      2.4  	return TRUE;
      2.5  }
      2.6  
      2.7 +/**
      2.8 + * @param filename The path to a file. Specifically this is the link target
      2.9 + *        from a link in an IM window with the leading "file://" removed.
     2.10 + */
     2.11  static void
     2.12 -file_open_uri(GtkIMHtml *imhtml, const char *uri)
     2.13 +open_file(GtkIMHtml *imhtml, const char *filename)
     2.14  {
     2.15  	/* Copied from gtkft.c:open_button_cb */
     2.16  #ifdef _WIN32
     2.17  	/* If using Win32... */
     2.18  	int code;
     2.19 -	if (purple_str_has_prefix(uri, "file://"))
     2.20 -	{
     2.21 -		gchar *escaped = g_shell_quote(uri);
     2.22 -		gchar *param = g_strconcat("/select,\"", uri, "\"", NULL);
     2.23 -		wchar_t *wc_param = g_utf8_to_utf16(param, -1, NULL, NULL, NULL);
     2.24 -
     2.25 -		code = (int)ShellExecuteW(NULL, L"OPEN", L"explorer.exe", wc_param, NULL, SW_NORMAL);
     2.26 -
     2.27 -		g_free(wc_param);
     2.28 -		g_free(param);
     2.29 -		g_free(escaped);
     2.30 -	} else {
     2.31 -		wchar_t *wc_filename = g_utf8_to_utf16(
     2.32 -				uri, -1, NULL, NULL, NULL);
     2.33 -
     2.34 -		code = (int)ShellExecuteW(NULL, NULL, wc_filename, NULL, NULL,
     2.35 -				SW_SHOW);
     2.36 -
     2.37 -		g_free(wc_filename);
     2.38 -	}
     2.39 +	/* Escape URI by replacing double-quote with 2 double-quotes. */
     2.40 +	gchar *escaped = purple_strreplace(filename, "\"", "\"\"");
     2.41 +	gchar *param = g_strconcat("/select,\"", escaped, "\"", NULL);
     2.42 +	wchar_t *wc_param = g_utf8_to_utf16(param, -1, NULL, NULL, NULL);
     2.43 +
     2.44 +	/* TODO: Better to use SHOpenFolderAndSelectItems()? */
     2.45 +	code = (int)ShellExecuteW(NULL, L"OPEN", L"explorer.exe", wc_param, NULL, SW_NORMAL);
     2.46 +
     2.47 +	g_free(wc_param);
     2.48 +	g_free(param);
     2.49 +	g_free(escaped);
     2.50  
     2.51  	if (code == SE_ERR_ASSOCINCOMPLETE || code == SE_ERR_NOASSOC)
     2.52  	{
     2.53 @@ -3304,7 +3299,8 @@
     2.54  	{
     2.55  		purple_notify_error(imhtml, NULL,
     2.56  				_("An error occurred while opening the file."), NULL);
     2.57 -		purple_debug_warning("gtkutils", "filename: %s; code: %d\n", uri, code);
     2.58 +		purple_debug_warning("gtkutils", "filename: %s; code: %d\n",
     2.59 +				filename, code);
     2.60  	}
     2.61  #else
     2.62  	char *command = NULL;
     2.63 @@ -3313,15 +3309,15 @@
     2.64  
     2.65  	if (purple_running_gnome())
     2.66  	{
     2.67 -		char *escaped = g_shell_quote(uri);
     2.68 +		char *escaped = g_shell_quote(filename);
     2.69  		command = g_strdup_printf("gnome-open %s", escaped);
     2.70  		g_free(escaped);
     2.71  	}
     2.72  	else if (purple_running_kde())
     2.73  	{
     2.74 -		char *escaped = g_shell_quote(uri);
     2.75 -
     2.76 -		if (purple_str_has_suffix(uri, ".desktop"))
     2.77 +		char *escaped = g_shell_quote(filename);
     2.78 +
     2.79 +		if (purple_str_has_suffix(filename, ".desktop"))
     2.80  			command = g_strdup_printf("kfmclient openURL %s 'text/plain'", escaped);
     2.81  		else
     2.82  			command = g_strdup_printf("kfmclient openURL %s", escaped);
     2.83 @@ -3329,7 +3325,7 @@
     2.84  	}
     2.85  	else
     2.86  	{
     2.87 -		purple_notify_uri(NULL, uri);
     2.88 +		purple_notify_uri(NULL, filename);
     2.89  		return;
     2.90  	}
     2.91  
     2.92 @@ -3339,7 +3335,7 @@
     2.93  		if (!g_spawn_command_line_sync(command, NULL, NULL, &exit_status, &error))
     2.94  		{
     2.95  			tmp = g_strdup_printf(_("Error launching %s: %s"),
     2.96 -							uri, error->message);
     2.97 +							filename, error->message);
     2.98  			purple_notify_error(imhtml, NULL, _("Unable to open file."), tmp);
     2.99  			g_free(tmp);
    2.100  			g_error_free(error);
    2.101 @@ -3360,8 +3356,9 @@
    2.102  static gboolean
    2.103  file_clicked_cb(GtkIMHtml *imhtml, GtkIMHtmlLink *link)
    2.104  {
    2.105 -	const char *uri = gtk_imhtml_link_get_url(link) + FILELINKSIZE;
    2.106 -	file_open_uri(imhtml, uri);
    2.107 +	/* Strip "file://" from the URI. */
    2.108 +	const char *filename = gtk_imhtml_link_get_url(link) + FILELINKSIZE;
    2.109 +	open_file(imhtml, filename);
    2.110  	return TRUE;
    2.111  }
    2.112  
    2.113 @@ -3369,7 +3366,7 @@
    2.114  open_containing_cb(GtkIMHtml *imhtml, const char *url)
    2.115  {
    2.116  	char *dir = g_path_get_dirname(url + FILELINKSIZE);
    2.117 -	file_open_uri(imhtml, dir);
    2.118 +	open_file(imhtml, dir);
    2.119  	g_free(dir);
    2.120  	return TRUE;
    2.121  }