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  }