On Fri, Mar 23, 2012 at 10:18 AM, Gert Doering <g...@greenie.muc.de> wrote:
> Hi, <snip> Thank you, Gert, for your detailed comments on my first attempt at this patch. The patch is meant to fix problems in the new-in-2.3 checking of options before trying to create the connection. Options that accept a command parameter instead of a path parameter fail if the command parameter includes spaces or more than just a command path. The following options are affected: --tls-verify, --up, --down, --ipchange, --route-up, --route-pre-down, and --learn-address. I've looked into this a bit more, and have found that where the options are actually used, argv_printf() is called to parse the command line into an argv structure. argv_printf uses parse_line() to do the actual parsing, and parse_line() processes single- and double-quotes and backslashes. I think that when options.c __checks__ an options command arguments, it should accept exactly the same input as the part of OpenVPN that __uses__ the option's commands. Attached is a heavily revised version of my original patch. It uses argv_printf() to __check__ an option's commands, so it accepts exactly the same input as the parts of OpenVPN that __use__ the options' commands. It also makes all the relevant changes suggested by Gert except having the argument following --iproute checked. The --iproute code is handled differently than the other options, and I think it is OK that we don't do checking in 2.3 on something that wasn't checked in 2.2 and (apparently) might go away sometime soon. The patch is now very straightforward and short: 1. As before, it calls a new function, check_cmd_access() -- instead of check_file_access() -- for all options that take a command as an parameter instead of a path. 2. check_cmd_access() uses argv_printf() to extract the command path and arguments from the command, then passes the command path to check_file_access(). It ignores the arguments because I can't think of anything about them that it can check. There is one part of check_cmd_access() which I took from code in run_up_down() and I am not sure about -- the three lines: gc_arena gc; and gc = gc_new (); and gc_free (&gc); Are they needed? I put them in because I assume that they set up and tear down a global garbage-collection area where allocations are made, and that that area is used by argv_new() and whatever eventually allocates the argv structure's components when they are created by argv_printf() and/or parse_line().
option-checking.diff
Description: Binary data
