On 31/03/12 13:47, Jonathan K. Bullard wrote: > On Wed, Mar 28, 2012 at 2:11 PM, David Sommerseth > <openvpn.l...@topphemmelig.net <mailto:openvpn.l...@topphemmelig.net>> > wrote: > <snip> > > > 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 <jkbull...@gmail.com>
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 <jkbull...@gmail.com>
Acked-by: Gert Doering <g...@greenie.muc.de>
Message-Id:
CAEsd45RkyJw6yUk1Jwkip70HkCjKYoU+V=do3n7sh7joahb...@mail.gmail.com
URL: http://article.gmane.org/gmane.network.openvpn.devel/6194
Signed-off-by: David Sommerseth <dav...@redhat.com>
commit a050bcef9cf71e7479551677b116879e6c563bd5
Author: Jonathan K. Bullard <jkbull...@gmail.com>
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: Jonathan K. Bullard <jkbull...@gmail.com>
Acked-by: Gert Doering <g...@greenie.muc.de>
Message-Id:
CAEsd45RkyJw6yUk1Jwkip70HkCjKYoU+V=do3n7sh7joahb...@mail.gmail.com
URL: http://article.gmane.org/gmane.network.openvpn.devel/6194
Signed-off-by: David Sommerseth <dav...@redhat.com>
kind regards,
David Sommerseth
signature.asc
Description: OpenPGP digital signature
