Hi,
Thanks for the review.
On Tue, Feb 19, 2019 at 12:39 PM David Sommerseth <
open...@sf.lists.topphemmelig.net> wrote:
> On 17/02/2019 02:55, selva.n...@gmail.com wrote:
> > From: Selva Nair <selva.n...@gmail.com>
> >
> > - Add a new return value (-2) for openvpn_execve() when external
> > program execution is not allowed due to a low script-security
> > setting.
> >
> > - Add a corresponding error message
> >
> > Errors and warnings in such cases will now display as
> > "WARNING: failed running command (<cmd>) :" followed by
> >
> > "disallowed by script-security setting" on all platforms
> >
> > instead of the current
> >
> > "external program did not execute -- returned error code -1"
> > on Windows and
> > "external program fork failed" on other platforms.
> >
> > The error is FATAL for some scripts and that behaviour is unchanged.
> >
> > This helps the Windows GUI to detect when a connection failure
> > results from a safer script-security setting enforced by the GUI,
> > and show a relevant message.
> >
> > Signed-off-by: Selva Nair <selva.n...@gmail.com>
> > ---
> > This is being presented as a better alternative for patch 684.
> > A separate patch may be needed for 2.4 -- will do if this is found
> > acceptable.
>
> Generally speaking, this looks much better and should avoid changing the
> behaviour. It just clarifies the error better. Feature-ACK from me.
>
>
> [...]
> > diff --git a/src/openvpn/run_command.c b/src/openvpn/run_command.c
> > index 2d75a3e..7df7576 100644
> > --- a/src/openvpn/run_command.c
> > +++ b/src/openvpn/run_command.c
> > @@ -65,12 +65,23 @@ system_error_message(int stat, struct gc_arena *gc)
> > {
> > buf_printf(&out, "external program did not execute -- ");
> > }
> > - buf_printf(&out, "returned error code %d", stat);
> > + if (stat == -2)
> > + {
> > + buf_printf(&out, "disallowed by script-security setting");
> > + }
> > + else
> > + {
> > + buf_printf(&out, "returned error code %d", stat);
> > + }
>
> Perhaps we could swap out the -1 and -2 values with macro constants, for
> code
> clarity? Also, perhaps it would look more structured if using a switch()
> instead of if()/else if().
Because of branching based on if (WIFEXITED(stat)) etc., switch() may be
only
marginally cleaner and cause some deep indentation. But will give it a go.
> Otherwise, the code looks good.
>
> Another interesting are which most likely quite seldom fails, but we
> actually
> have yet another "undocumented" exit code ... This is not this patch's
> fault,
> but should probably be reviewed a bit more carefully.
>
> In run_command.c:149-153:
> --------------------------------------------------------------------
> pid = fork();
> if (pid == (pid_t)0) /* child side */
> {
> execve(cmd, argv, envp);
> exit(127);
> }
> --------------------------------------------------------------------
>
> If execve() fails, the exit code is 127. That would normally be caught by
> the
> waitpid() later on and this exit code would be returned by
> openvpn_execve().
>
> This should be improved in a separate patch though, but is not of high
> urgency. I don't expect most setups having a high execve() failure rate.
> But
> catching it and reporting this error path would be good.
>
In fact its easy to trigger this -- just have a shebang with a non-existent
path like
#!/bin/sh1 and child will exit with 127. In system_error_message() we
translate that as
"could not execute external program"
This "could not" instead of "did not" is a good choice of words, but would
be better
if we somehow marshal the actual errno set by execve (in this case ENOENT)
to the parent. Its lost in the current implementation. But that's beyond
this patch,
as you say.
I'll just add a macro for this 127 as well and leave it at that.
Selva
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
