[Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Jonathan K. Bullard]

I'm the developer for Tunnelblick (open source GUI for OS X), having taken
over from Angelo Laub a couple of years ago. I'd like to make a beta of
Tunnelblick with OpenVPN 2.3alpha1 available for testing, but the alpha has
a bug that makes it useless for most users of Tunnelblick. Lots of people
use Tunnelblick betas, so it would be a good way to get 2.3alpha1 into the
hands of a lot of people -- there were more than 30,000 downloads of the
last beta in the ten days it was available. (Users can easily switch back
to using OpenVPN 2.2.1 with two clicks in the Tunnelblick GUI, so there's
not much downside for them to try it.

Almost all Tunnelblick users use the standard Tunnelblick up/down scripts,
which fail on 2.3alpha1. I don't want to provide a beta of Tunnelblick that
includes OpenVPN 2.3alpha1 if it will only work for a small percentage of
users.

I don't know OpenVPN well enough to work on this, so Samuli Seppänen
suggested that I email to this mailing list. Could one of you developers
provide a patch to 2.3alpha1 that fixes it? I would rather not wait until
alpha2 to start testing.


*Details
*

OpenVPN 2.3alpha1 fails when the argument to "--up" contains more than an
execution path. The problem also occurs for the "--down" option and the new
"--route-pre-down" option (and presumably any other options that take more
than just an execution path).

OpenVPN 2.2.1 and earlier work fine with an identical command line.

This happens on OS X; I don't know about other platforms but I assume
option processing is not very platform specific.

2.3alpha1 is apparently treating the entire argument that follows "--up" as
the path for a file to execute, instead of treating it as a shell command
consisting of a path optionally followed by arguments.

On 2.3alpha1:

 test jkb$ *openvpn-2.3-alpha1/openvpn --config
/private/tmp/test/config.ovpn  --up "/private/tmp/test/up.sh  -x"*
 Options error: --up script fails with '/private/tmp/test/up.sh  -x':
No such file or directory
 Options error: Please correct these errors.
 Use --help for more information.

But on 2.2.1:
 test jkb$ *openvpn-2.2.1/openvpn --config
/private/tmp/test/config.ovpn --up "/private/tmp/test/up.sh  -x"*
 Wed Mar  7 07:30:33 2012 OpenVPN 2.2.1 i386-apple-darwin10.8.0 [SSL]
[LZO2] [PKCS11] [eurephia] built on Mar  1 2012
 Enter Auth Username:
 Wed Mar  7 07:30:46 2012 ERROR: Auth username is empty
 Wed Mar  7 07:30:46 2012 Exiting

And
 test jkb$ *openvpn-2.3-alpha1/openvpn --config
/private/tmp/test/config.ovpn  --up "/private/tmp/test/up.sh"*
 Wed Mar  7 07:35:56 2012 OpenVPN 2.3-alpha1 i386-apple-darwin10.8.0
[SSL (OpenSSL)] [LZO2] [eurephia] [MH] [PF_INET6] [IPv6 payload 20110522-1
(2.2.0)] built on Mar  1 2012
 Enter Auth Username:
 Wed Mar  7 07:35:59 2012 ERROR: Auth username is empty
 Wed Mar  7 07:35:59 2012 Exiting due to fatal error

Notes:

   1. The above commands are simplified to show the problem. They won't
   actually work for making a connection.
   2. The double-quotes in the above command lines are used so bash will
   create single arguments that contain spaces. When starting OpenVPN,
   Tunnelblick uses an OS X interface that provides the arguments as separate
   items, so they don't need to be enclosed in quotes. The actual argument
   that OpenVPN is passed will contain quotes if the path contains spaces.

If the path contains spaces, OpenVPN will be passed


 "/private/tmp/test path with spaces/up.sh"  -x

If the path does not contain spaces, OpenVPN will be passed

 /private/tmp/test/up.sh -x


I'd appreciate any help on this that I can get.

Thanks,

Jon Bullard

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/03/12 13:48, Jonathan K. Bullard wrote:
[...skip...]
> 
> *Details *
> 
> OpenVPN 2.3alpha1 fails when the argument to "--up" contains more
> than an execution path. The problem also occurs for the "--down"
> option and the new "--route-pre-down" option (and presumably any other
> options that take more than just an execution path).
> 
> OpenVPN 2.2.1 and earlier work fine with an identical command line.
> 
> This happens on OS X; I don't know about other platforms but I assume
>  option processing is not very platform specific.
> 
> 2.3alpha1 is apparently treating the entire argument that follows 
> "--up" as the path for a file to execute, instead of treating it as a 
> shell command consisting of a path optionally followed by arguments.
> 
> On 2.3alpha1:
> 
> test jkb$ *openvpn-2.3-alpha1/openvpn --config 
> /private/tmp/test/config.ovpn  --up "/private/tmp/test/up.sh -x"* 
> Options error: --up script fails with '/private/tmp/test/up.sh  -x': 
> No such file or directory Options error: Please correct these errors.
>  Use --help for more information.

Ouch!  I see that check_file_access() needs to strip out any arguments.
It will now basically 'access("/private/tmp/test/up.sh -x")' ... which is
a file which doesn't exist ... but if it tested for
'access("/private/tmp/test/up.sh")' it should find the file.

However, it isn't as easy to just skip through the string and "terminate
it" on the first space (0x20) value, as it might have been escaped.
Which can make quite typical paths like this fail:

   "C:\Program Files\OpenVPN\bin\up-script.bat"

I got my plate very full right now, so if you or someone else have time
to poke at this, that'd be great.


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9XbDkACgkQDC186MBRfrp1NgCeO7CLBygxocRknDbrYMNl3lg4
nnMAn0nSnMVMGTcSDDttD8zOp5JDhnX/
=rgox
-END PGP SIGNATURE-

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Jonathan K. Bullard]

On Wed, Mar 7, 2012 at 9:10 AM, David Sommerseth
 wrote:
[skipped]
> > OpenVPN 2.3alpha1 fails when the argument to "--up" contains more
> > than an execution path. The problem also occurs for the "--down"
> > option and the new "--route-pre-down" option (and presumably any other
> > options that take more than just an execution path).
[skipped]

> Ouch!  I see that check_file_access() needs to strip out any arguments.
> It will now basically 'access("/private/tmp/test/up.sh -x")' ... which is
> a file which doesn't exist ... but if it tested for
> 'access("/private/tmp/test/up.sh")' it should find the file.
>
> However, it isn't as easy to just skip through the string and "terminate
> it" on the first space (0x20) value, as it might have been escaped.
> Which can make quite typical paths like this fail:
>
>   "C:\Program Files\OpenVPN\bin\up-script.bat"


Below is a patch to fix this problem.

It causes the affected options (--up, --down, etc.) to call a new
function for verification: check_cmd_access(). It is called with the
same calling sequence as check_file_access() so as to simplify things,
and it uses check_file_access() to check the file once the path for it
has been isolated. I wrote a new function so that (a) I wouldn't
disturb the existing function, and (b) it could be extended -- for
example, to check for any problems with any arguments that follow the
path to the functions (mismatched quotes, for example).

It's my first proposed patch for OpenVPN, so please examine it _very_
carefully. I've tested it only OS X (and it works fine), but I don't
have a full debugging environment, so it was tested as a "black box"
using msg() calls to verify its behavior. I tried to conform to the
style I saw elsewhere in the source, but may have missed or
misunderstood something.

There are a couple of anomalies in the existing code that I have not dealt with:

1. --iproute also takes a "cmd" argument but the options.c code does
not call check_file_access() for it. (So I have not included it in
this patch),

2. Although the man page [1] says most of the "cmd" arguments can be a
"shell command", that doesn't seem correct. On OS X, for example,
backslashes are discarded (not used to escape the next character) and
double-quotes can only be used at the start and end of an individual
argument in "cmd". The bash shell implements both of these (rendering
 abc"def"ghi.\ txt
as
 abcdefghi.txt
for example. This patch is very restrictive: it allows a "cmd" to be a
path (optionally enclosed in double-quotes), optionally followed by a
space, which may be followed by anything.

I would particularly like input on these lines in the new routine (but
of course all input is welcome):

ASSERT((path_size <= OPTION_PARM_SIZE) || (path_size > 0));
msg (M_NOPREFIX|M_OPTERR, "%s fails with '%s': param_size BAD:
stop_char = '%c'; start_ix = %d; "
 "command = " ptr_format "; stop_ptr = " ptr_format ";
path_size = %d; OPTION_PARAM_SIZE = %d",
 opt, command, stop_char, start_ix,
 (ptr_type) command, (ptr_type) stop_ptr, path_size, (int)
OPTION_PARM_SIZE);
return true;

These lines test to make sure that OPTION_PARM_SIZE is reasonable. I
don't know your conventions, so I included both an assert and an error
message to be output before a failure return ("true" is a failure
indicator).

Let me know of any changes you'd like me to make.

- Jon Bullard

[1] http://openvpn.net/index.php/manuals/523-openvpn-23.html

===

--- openvpn/options.c   (revision 1964)
+++ openvpn/options.c   (working copy)
@@ -2656,6 +2656,72 @@
 }

 /*
+ * Check the command that comes after certain script options (e.g., --up).
+ *
+ * The command should consist of a path, which may be enclosed in
double-quotes, and may be
+ * optionally followed by a space which may be followed by arbitrary arguments.
+ *
+ * Once the path has been extracted from the command (if that is
necessary), check_file_access()
+ * is used to do the  sanity checking on it. The type, mode, and opt
arguments to this routine
+ * are the same as the corresponding check_file_access() arguments to
facilitate this.
+ */
+static bool
+check_cmd_access(const int type, const char *command, const int mode,
const char *opt)
+{
+  /* If no command configured, no errors to look for */
+  if (! command)
+return false;
+
+  /* Test for a quote as the first char of command
+ and for presence of a space in command */
+
+  int   start_ix  = 0;  /* Where the path starts within command
   (0 or 1) */
+  char  stop_char = '\000'; /* Character that terminates the path
within command (' ' or '"') */
+  char *stop_ptr  = NULL;   /* Pointer past end of path   (NULL
or points inside command) */
+
+  if (command[0] == '"')
+  {
+start_ix  = 1;
+stop_char = '"';
+stop_ptr  = strchr(command+1, '"');
+if (stop_ptr == NULL)
+{
+  msg (M_NOPREFIX|M_OPTERR, "%s fails with '%s': Unbalanced quote",
+  

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Gert Doering]

Hi,

sorry for not coming back to this.  We *do* appreciate (very much!) that 
you are interested and working on getting 2.3-alpha into tunnelblick - but
life got in the way, as usual.

On Thu, Mar 08, 2012 at 04:57:11PM -0500, Jonathan K. Bullard wrote:
> Below is a patch to fix this problem.
> 
> It causes the affected options (--up, --down, etc.) to call a new
> function for verification: check_cmd_access(). It is called with the
> same calling sequence as check_file_access() so as to simplify things,
> and it uses check_file_access() to check the file once the path for it
> has been isolated. I wrote a new function so that (a) I wouldn't
> disturb the existing function, and (b) it could be extended -- for
> example, to check for any problems with any arguments that follow the
> path to the functions (mismatched quotes, for example).

I think the generall approach is reasonable.

Quoting of arguments in general is an *interesting* problem, given the
possible sources of --up arguments - config file, shell (with the shell
having its own ideas which bits to keep and which to remove), windows
cli (no idea).  But this is sort of outside the scope of this initial
patch, just something to keep in mind.


> It's my first proposed patch for OpenVPN, so please examine it _very_
> carefully. I've tested it only OS X (and it works fine), but I don't
> have a full debugging environment, so it was tested as a "black box"
> using msg() calls to verify its behavior. I tried to conform to the
> style I saw elsewhere in the source, but may have missed or
> misunderstood something.
> 
> There are a couple of anomalies in the existing code that I have not dealt 
> with:
> 
> 1. --iproute also takes a "cmd" argument but the options.c code does
> not call check_file_access() for it. (So I have not included it in
> this patch),

Well spotted.  For completeness, this would need such a check as well - or
the option could be thrown out (personally I'm tempted to throw it out,
as all other external system utilities are called with compiled-in paths,
but that's a different discussion again)

> 2. Although the man page [1] says most of the "cmd" arguments can be a
> "shell command", that doesn't seem correct. On OS X, for example,
> backslashes are discarded (not used to escape the next character) and
> double-quotes can only be used at the start and end of an individual
> argument in "cmd". The bash shell implements both of these (rendering
>  abc"def"ghi.\ txt
> as
>  abcdefghi.txt
> for example. 

The shell will "eat" quotes, if not double-quoted, like:

 openvpn --up 'my_command "with arguments in quotes"'

will pass everything but the single quotes to openvpn as a single argv[]
argument, including the double quotes.  OTOH I would need to check what
the config file parser does regarding single and double quotes...

> This patch is very restrictive: it allows a "cmd" to be a
> path (optionally enclosed in double-quotes), optionally followed by a
> space, which may be followed by anything.

Conceptionally, I'd say this is good enough for Alpha2.  Then let's see
what else will break.

> I would particularly like input on these lines in the new routine (but
> of course all input is welcome):
> 
> ASSERT((path_size <= OPTION_PARM_SIZE) || (path_size > 0));
> msg (M_NOPREFIX|M_OPTERR, "%s fails with '%s': param_size BAD:
> stop_char = '%c'; start_ix = %d; "
>  "command = " ptr_format "; stop_ptr = " ptr_format ";
> path_size = %d; OPTION_PARAM_SIZE = %d",
>  opt, command, stop_char, start_ix,
>  (ptr_type) command, (ptr_type) stop_ptr, path_size, (int)
> OPTION_PARM_SIZE);
> return true;
> 
> These lines test to make sure that OPTION_PARM_SIZE is reasonable. I
> don't know your conventions, so I included both an assert and an error
> message to be output before a failure return ("true" is a failure
> indicator).

Well, both are used throughout the code...

ASSERT, if there is no way to recover, and this really shouldn't happen 
in the first place.  But this should be an "&&" there, because with an
"||", this will *always* be true...

msg(), if the program can go on, or this is inside option parsing (M_FATAL),
but we want a "friendly" exit instead of a hard abort.  msg() output is
normally meant to be understandable by a user of the code, not only
targetted to the programmer.


Now, the msg() above looks very much like "targetted to the programmer"
to me - printing pointer addresses is not something a user expects to
see, and it wouldn't help them fix the problem.  So if this is a "this
should never happen!" bit of the code, ASSERT() is good enough - and if
it's "the user has input a funny string" it should be spelled differently :-)


After all the preliminaries (am I making sense?), now looking at the actual
patch :-)


> --- openvpn/options.c (revision 1964)
> +++ openvpn/options.c (working copy)
> @@ -2656,6 +2656,72 @@
>  }
> 
>  /*
> + * Check the command that comes after certain scrip

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Jonathan K. Bullard]

On Fri, Mar 23, 2012 at 10:18 AM, Gert Doering  wrote:

> Hi,



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

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Fabian Knittel]

Hi Jonathan,

2012/3/28 Jonathan K. Bullard :
> 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().

gc_arena instances are used by explicitly passing a pointer to it. So,
unless one of the functions takes an instance of gc_arena as a
parameter, you don't need to prepare one. As many functions in OpenVPN
take one, there's some dead code scattered about that needlessly
creates and frees gc arenas (probably because uses have come and gone
over time). So you can drop gc because it's not used.

The argv family of functions apparently has its own memory management
and therefore argv_new() should be paired with an argv_reset().
Otherwise the memory allocated by argv_printf() is leaked.

Cheers
Fabian

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Jonathan K. Bullard]

On Wed, Mar 28, 2012 at 9:57 AM, Fabian Knittel
wrote:

gc_arena instances are used by explicitly passing a pointer to it. So,
> unless one of the functions takes an instance of gc_arena as a
> parameter, you don't need to prepare one. As many functions in OpenVPN
> take one, there's some dead code scattered about that needlessly
> creates and frees gc arenas (probably because uses have come and gone
> over time). So you can drop gc because it's not used.
>
> The argv family of functions apparently has its own memory management
> and therefore argv_new() should be paired with an argv_reset().
> Otherwise the memory allocated by argv_printf() is leaked.
>

Thanks, Fabian.

If I don't hear about anything else in the next couple of days I'll
resubmit with these fixes.

Regards, Jon

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Gert Doering]

Hi,

On Wed, Mar 28, 2012 at 09:03:44AM -0400, Jonathan K. Bullard wrote:
> 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.

Cool :-) - thanks for investigation, I have learned something new today
(I have copy-paste used the argv_sprintf() stuff, but not fully investigated
what it does).

> 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. 

As already noted by Fabian, the "gc" stuff is not needed, and you need
an argv_reset() call - but other than that, this looks close to inclusion,
I'd say :-)

> 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.

Makes sense.

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgpfqkkoSwxBL.pgp
Description: PGP signature

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 28/03/12 15:03, Jonathan K. Bullard wrote:
> On Fri, Mar 23, 2012 at 10:18 AM, Gert Doering  > wrote:
> 
> Hi,
> 
> 
> 
> 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.

Thanks a lot!  I have one more comment to what Gert and Fabian has
already covered.
Instead of adding  wrapper function, check_cmd_access(), would it be
possible to
integrate this with check_file_access() and add another type flag, f.ex:

#define CHKACC_EXEC (1<<5)  /** Filename is an executable, ignore exec
args */

Then you can just flip the type flag from CHKACC_FILE to CHKACC_EXEC.  If
this
type is checked for, enforcing an X_OK mode check in addition is probably
reasonable
too.


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9zVFcACgkQDC186MBRfrpH4ACfQ/9ySeXkOnuxI/jaEpgKmpXG
c6kAnRVnqYIvQ6dH1rfXjyj5IUpBMh6+
=bTBw
-END PGP SIGNATURE-

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Jonathan K. Bullard]

On Wed, Mar 28, 2012 at 2:11 PM, David Sommerseth <
openvpn.l...@topphemmelig.net> wrote:


> > 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.
>
> Thanks a lot!  I have one more comment to what Gert and Fabian has
> already covered.
> Instead of adding  wrapper function, check_cmd_access(), would it be
> possible to
> integrate this with check_file_access() and add another type flag, f.ex:
>
> #define CHKACC_EXEC (1<<5)  /** Filename is an executable, ignore exec
> args */
>
> Then you can just flip the type flag from CHKACC_FILE to CHKACC_EXEC.  If
> this
> type is checked for, enforcing an X_OK mode check in addition is probably
> reasonable
> too.


Thanks for this suggestion, David, but I believe it is better to create a
separate routine for this:

   1. The input is not a path, but a "command". A "command" is processed
   (single- and double-quote and backslashes are processed and leading spaces
   are removed), and it consists not only of a path, but may include arguments.
   2. Having two separate functions makes two cleaner and more readable (to
   me) functions, instead of a single function that accepts strings with two
   different formats and is more complicated.

The first attached patch incorporates Fabian's comments. But if the
consensus is that I should incorporate David's suggestion, I will do that.

The first patch also updates the usage message to clarify what a "cmd" is.
(That is, it is *not* a shell script or path.)

The second patch updates the man page to:

   - Clarify what a "cmd" is;
   - Change the descriptions of several options to note that they accept a
   "command";
   - Change the description of --client-connect and --client-disconnect
   indicate that the temporary file's path is passed as the *last* argument
   to the command, not the *first* argument; and
   - Adds a description of --route-pre-down to the descriptions of the
   other --route options.

Thanks again to Fabian, Gert, and David for their help.


options.c.diff
Description: Binary data


openvpn.8.diff
Description: Binary data

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Gert Doering]

Hi,

On Sat, Mar 31, 2012 at 07:47:34AM -0400, Jonathan K. Bullard wrote:
> Thanks for this suggestion, David, but I believe it is better to create a
> separate routine for this:
> 
>1. The input is not a path, but a "command". A "command" is processed
>(single- and double-quote and backslashes are processed and leading spaces
>are removed), and it consists not only of a path, but may include 
> arguments.
>2. Having two separate functions makes two cleaner and more readable (to
>me) functions, instead of a single function that accepts strings with two
>different formats and is more complicated.
> 
> The first attached patch incorporates Fabian's comments. But if the
> consensus is that I should incorporate David's suggestion, I will do that.
> 
> The first patch also updates the usage message to clarify what a "cmd" is.
> (That is, it is *not* a shell script or path.)
> 
> The second patch updates the man page to:

I'm fine with these changes (and I don't particularily mind if it's a
separate function or integrated in the other one - both arguments have
their merits).  So I'll ACK both, and leave it to David to NAK it if
he insists on "I want it integrated".

> Thanks again to Fabian, Gert, and David for their help.

Thanks for contributing :-) - there's only so much we can do, given
the amount of different scenarios where people are using OpenVPN, so
we can't test all of it...

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgp2AUJekWIMt.pgp
Description: PGP signature

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[David Sommerseth]

On 31/03/12 13:47, Jonathan K. Bullard wrote:
> On Wed, Mar 28, 2012 at 2:11 PM, David Sommerseth
> mailto:openvpn.l...@topphemmelig.net>>
> wrote:
> 
> 
> > 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.
> 
> Thanks a lot!  I have one more comment to what Gert and Fabian has
> already covered.
> Instead of adding  wrapper function, check_cmd_access(), would it be
> possible to
> integrate this with check_file_access() and add another type flag, f.ex:
> 
> #define CHKACC_EXEC (1<<5)  /** Filename is an executable, ignore exec
> args */
> 
> Then you can just flip the type flag from CHKACC_FILE to
> CHKACC_EXEC.  If
> this
> type is checked for, enforcing an X_OK mode check in addition is
> probably
> reasonable
> too.
> 
> 
> Thanks for this suggestion, David, but I believe it is better to create
> a separate routine for this:
> 
>  1. The input is not a path, but a "command". A "command" is processed
> (single- and double-quote and backslashes are processed and leading
> spaces are removed), and it consists not only of a path, but may
> include arguments.
>  2. Having two separate functions makes two cleaner and more readable
> (to me) functions, instead of a single function that accepts strings
> with two different formats and is more complicated.
> 
> The first attached patch incorporates Fabian's comments. But if the
> consensus is that I should incorporate David's suggestion, I will do that.
> 
> The first patch also updates the usage message to clarify what a "cmd"
> is. (That is, it is /not/ a shell script or path.)
> 
> The second patch updates the man page to:
> 
>   * Clarify what a "cmd" is;
>   * Change the descriptions of several options to note that they accept
> a "command";
>   * Change the description of --client-connect and --client-disconnect
> indicate that the temporary file's path is passed as the /last/
> argument to the command, not the /first/ argument; and
>   * Adds a description of --route-pre-down to the descriptions of the
> other --route options.
> 
> Thanks again to Fabian, Gert, and David for their help.

Sorry for the late response, but this got applied yesterday to the master 
branches on -stable and -testing trees.

I took the freedom to re-arrange your patches and make the commits more related 
to each other and to quickly create a couple of commit messages.

commit d62859980c30362b36b7338fc99fe76e4ecb2cbd
Author: Jonathan K. Bullard 
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Sat Mar 31 07:47:34 2012 -0400

Clarified the docs and help screen about what a 'cmd' is

This also changes the descriptions of several options to note that they 
accept
a "command"; change the description of --client-connect and 
--client-disconnect
indicate that the temporary file's path is passed as the last argument to 
the
command, not the first argument; and Adds a description of --route-pre-down 
to
the descriptions of the other --route options.

[DS: This patch is based on parts of the options.c.diff and the complete
 openvpn.8.diff patch sent to the mailing list - where these docs 
changes
 are merged together into this patch]

Signed-off-by: Jonathan K. Bullard 
Acked-by: Gert Doering 
Message-Id: 
CAEsd45RkyJw6yUk1Jwkip70HkCjKYoU+V=do3n7sh7joahb...@mail.gmail.com
URL: http://article.gmane.org/gmane.network.openvpn.devel/6194
Signed-off-by: David Sommerseth 

commit a050bcef9cf71e7479551677b116879e6c563bd5
Author: Jonathan K. Bullard 
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Sat Mar 31 07:47:34 2012 -0400

Fix file access checks on commands

The current implementation of check_file_access() does not consider that
some options take scripts and executables as input.  When some of these
commands are given arguments in the OpenVPN configuration,
check_file_access() would take those arguments as a part of the file name
to the command.  Thus the file check would fail.

This patch improves that by introducing a check_cmd_access() function which
first splits out the arguments to the command before checking if the file
with the command is available.

[DS: This patch is splitted out from the options.c.diff patch sent to the
 mailing list - where only the function changes is included here]

Signed-off-by: