[Openvpn-devel] Better error message when script fails due to script-security setting

From: Selva Nair <selva.nair@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.nair@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.

 src/openvpn/run_command.c | 25 +++++++++++++++++++++----
 src/openvpn/win32.c       | 10 +++++++---
 2 files changed, 28 insertions(+), 7 deletions(-)

Hi,

On Sat, Feb 16, 2019 at 08:55:41PM -0500, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@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

I find this a useful way forward (because it will not introduce new FATALs
where we had none before, just make the errors more clear).  

Since David had reservations on the other patch, I want to have explicit
agreement from him before going on - and will poke on IRC to make sure I
get attention.

Nevertheless, as we agreed to tag 2.4.7 *today* to make the release tomorrow
and meet the Debian deadline, I will not include this in 2.4.7.

We can always do a 2.4.8 release "really soon", and include this (plus
other potential 2.4 material which is important for Windows, like the 
OpenSSL 1.1.1 / cryptoapi stuff).


"Release early, release often" used to be the mantra... :-)

gert

Hi

On Mon, Feb 18, 2019 at 9:24 AM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Sat, Feb 16, 2019 at 08:55:41PM -0500, selva.nair@gmail.com wrote:
> > From: Selva Nair <selva.nair@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
>
> I find this a useful way forward (because it will not introduce new FATALs
> where we had none before, just make the errors more clear).
>
> Since David had reservations on the other patch, I want to have explicit
> agreement from him before going on - and will poke on IRC to make sure I
> get attention.
>

Just to be explicit:

Since David pointed out the "regressive" nature of the earlier patch, I too
have
reservations about it, and prefer this approach.

Nevertheless, as we agreed to tag 2.4.7 *today* to make the release tomorrow
> and meet the Debian deadline, I will not include this in 2.4.7.


The planned script-security over-ride feature in the GUI is pretty
intrusive. Taking
time to give it more thought, and preferably get some user feedback, is not
a bad thing.

Selva
<div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Feb 18, 2019 at 9:24 AM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Sat, Feb 16, 2019 at 08:55:41PM -0500, <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a> wrote:<br>
&gt; From: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
&gt; <br>
&gt; - Add a new return value (-2) for openvpn_execve() when external<br>
&gt; program execution is not allowed due to a low script-security<br>
&gt; setting.<br>
&gt; <br>
&gt; - Add a corresponding error message<br>
<br>
I find this a useful way forward (because it will not introduce new FATALs<br>
where we had none before, just make the errors more clear).  <br>
<br>
Since David had reservations on the other patch, I want to have explicit<br>
agreement from him before going on - and will poke on IRC to make sure I<br>
get attention.<br></blockquote><div><br></div><div>Just to be explicit:</div><div><br></div><div>Since David pointed out the &quot;regressive&quot; nature of the earlier patch, I too have</div><div>reservations about it, and prefer this approach.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Nevertheless, as we agreed to tag 2.4.7 *today* to make the release tomorrow<br>and meet the Debian deadline, I will not include this in 2.4.7.</blockquote><div><br></div><div>The planned script-security over-ride feature in the GUI is pretty intrusive. Taking</div><div>time to give it more thought, and preferably get some user feedback, is not a bad thing.</div><div><br></div><div>Selva</div></div></div></div></div>

On 17/02/2019 02:55, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@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.nair@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.

Hi,

Thanks for the review.

On Tue, Feb 19, 2019 at 12:39 PM David Sommerseth <
openvpn@sf.lists.topphemmelig.net> wrote:

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