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().
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.
--
kind regards,
David Sommerseth
OpenVPN Inc
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
