Command injection through URL in Pidgin

[Mark Doliner]

On Fri, Jun 14, 2013 at 3:28 PM, Tomasz Wasilczyk <tomkiewi at gmail.com> wrote:
> I did a proof-of-concept implementation. I haven't had time to review
> it, because I'm going out of town for another few days, but feel free
> to comment the idea.
>
> Concept:
> - store all arguments as a GList of const gchar* (and then convert it to argv)
> - pass original uri to known browsers, escaped one for unknown (and
> xdg-open &co)
>
> The patch:
> https://dl.dropboxusercontent.com/u/5448886/contrib/pidgin-uri-notification.patch
>
> Did I missed something? Should I add more characters to don't-escape list?

Thanks again for working on this, Tomasz.  I'm worried about one
aspect of the new code and would love a second opinion.

Previously we passed a command line string to
g_spawn_command_line_async().  This function basically just calls
g_shell_parse_argv() then g_spawn_async().  The documentation for
g_shell_parse_argv() says:
"Parses a command line into an argument vector, in much the same way
the shell would, but without many of the expansions the shell would
perform (variable expansion, globs, operators, filename expansion,
etc. are not supported). The results are defined to be the same as
those you would get from a UNIX98 /bin/sh, as long as the input
contains none of the unsupported shell expansions. If the input does
contain such expansions, they are passed through literally."

If I'm reading that correctly, $(blah) and other oddities should not
get expanded, right?  They should be passed through to the browser
command literally.

Tomasz's new code creates a list of args, converts that list to a
char**, then calls g_spawn_async().  It seems like we've kind of
created our own implementation of g_shell_parse_argv().  And unless
I'm overlooking something, it seems like we don't gain anything from
it and it makes our code more complex.  Anyone have thoughts on this?
Tomasz code might be a little more efficient, but this code is called
so rarely that I don't think it matters.

Tomasz's new code now uses this to escape the URL:
  g_uri_escape_string(uri, ":/%#,+", FALSE);
Instead of
  g_shell_quote(uri);
I think this change is wonderful and is sufficient to fix the original problem.

If we do decide to stick with this version of the changes, I have a
few requests:
- Please use GSList instead of GList.
- Please don't use a fixed-size buffer of 1024 for arg_buff.  I can
pretty much guarantee that some users will have URLs longer than that.
- I think there is a missing g_free for the return value from
purple_strreplace()