Command injection through URL in Pidgin

[Mark Doliner]

On Fri, Jun 21, 2013 at 3:47 AM, Tomasz Wasilczyk <tomkiewi at gmail.com> wrote:
> 2013/6/20 Mark Doliner <mark at kingant.net>:

(...)

> g_shell_parse_argv is only used in "custom" browser mode - for all
> pre-defined browsers arg list is built manually. For this (rarely
> used) mode, it may be small gain - for this single case we could use
> old g_spawn_command_line_async() method. But my implementation have
> two advantages over it:
> - "custom" and pre-defined browsers use the same function for
> executing it (uri_command()), it's useful for debugging
> - there is no possibility for theoretical potential bug that could
> split execution command into more arguments than user configured,
> because the command is splitted into arguments and then %s is replaced
> with the url, so the whole process is totally independent from the url
>
>> Tomasz code might be a little more efficient, but this code is called
>> so rarely that I don't think it matters.
>
> Actually, it's not meant to be more efficient. It's main advantage is
> 100% fixed arg list for predefined browsers. It doesn't fix any actual
> bug, it's preventive action.

Ok, you've mostly convinced me.  One more question: How did you decide
whether to use uri or uri_escaped for the various browsers? Obviously
you would use uri_escaped for xdg-open, since that's the thing we're
trying to fix here. I guess you don't need uri_escaped with Chrome
because it doesn't do silly things with passing the argument on the
command line? Is there harm to using uri_escaped? If we're trying to
be preventative then maybe we should ALWAYS use uri_escaped, in case a
browser starts doing silly things in the future?

>> If we do decide to stick with this version of the changes, I have a
>> few requests:
>> - Please use GSList instead of GList.
>
> Certain version of my implementation just used g_list_prepend. I'm OK
> with switching it to gslist.

FYI you can totally still use g_list_prepend with a GSList.

>> - 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.
>
> Is there any reasonable limit for url size? I'd like to avoid
> unnecessary mallocs.

I think using 16384 is probably fine, but in this case I think you're
better off using dynamically allocated memory.  purple_strreplace()
already returns a new string to you--you would just need to move the
g_free to the bottom of the function instead of copying it.

>> - I think there is a missing g_free for the return value from
>> purple_strreplace()
>
> My bad, thanks. It's another remains of another implementation version.

Other requests (in addition to the above):
- Can you not check for null before calling g_strfreev()?  The
documentation says that function checks for null itself.
- Can you move the declaration of usercmd_argv inside the else-if
block where it's used?
- You should g_free(uri_command) before calling return in two places
in the custom browser code.