[Openvpn-devel,v3] Make up/down script errors not FATAL

Message ID 1531491395-29049-1-git-send-email-selva.nair@gmail.com
State New
Headers show
Series
  • [Openvpn-devel,v3] Make up/down script errors not FATAL
Related show

Commit Message

Selva Nair July 13, 2018, 2:16 p.m.
From: Selva Nair <selva.nair@gmail.com>

The error is treated as a warning only if its triggered due
to script_security < SSEC_SCRIPTS.

This helps user interfaces enforce a safer script-security setting
without causing a FATAL error.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
v3 changes:
- script_security --> script_security() following
  commit bf97c00f7dba441b504881f38e40afcbb610a39f

v2 changes:
- Have script errors continue to trigger a FATAL error.
- Update the commit message to match this change.

 src/openvpn/init.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Steffan Karger Oct. 5, 2018, 9:43 a.m. | #1
Hi,

On 13-07-18 16:16, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> The error is treated as a warning only if its triggered due
> to script_security < SSEC_SCRIPTS.
> 
> This helps user interfaces enforce a safer script-security setting
> without causing a FATAL error.

But does it make sense at all to accept configs that have a --up script
without a sufficiently-high script-security set?

I do agree that the current place where the code checks this not nice,
so maybe we should perform this check somewhere in the
options_postprocess_verify* functions?  That way we error out early,
instead of only when trying to execute the script.

-Steffan
Selva Nair Oct. 5, 2018, 3:30 p.m. | #2
Hi,

On Fri, Oct 5, 2018 at 5:44 AM Steffan Karger <steffan@karger.me> wrote:

> Hi,
>
> On 13-07-18 16:16, selva.nair@gmail.com wrote:
> > From: Selva Nair <selva.nair@gmail.com>
> >
> > The error is treated as a warning only if its triggered due
> > to script_security < SSEC_SCRIPTS.
> >
> > This helps user interfaces enforce a safer script-security setting
> > without causing a FATAL error.
>
> But does it make sense at all to accept configs that have a --up script
> without a sufficiently-high script-security set?
>

This came out of a proposed patch for the GUI to protect lay users from
malicious
scripts embedded in config files.

Recall the ado about exploiting scripts using unsuspecting "inline"
commands.
To defeat such exploits we want to enforce a safer script security setting
from
the GUI but do not want to cause a FATAL error as that would be counter
productive.

Please see GUI PR #271 https://github.com/OpenVPN/openvpn-gui/pull/271 and
my comment dated Jul 3 under it.

The discussion that led to this is here:
https://github.com/OpenVPN/openvpn-gui/issues/270

Thanks,

Selva
<div dir="ltr"><div dir="ltr"><div dir="ltr">Hi,<br><div><br><div class="gmail_quote"><div dir="ltr">On Fri, Oct 5, 2018 at 5:44 AM Steffan Karger &lt;<a href="mailto:steffan@karger.me" target="_blank">steffan@karger.me</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 13-07-18 16:16, <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; The error is treated as a warning only if its triggered due<br>
&gt; to script_security &lt; SSEC_SCRIPTS.<br>
&gt; <br>
&gt; This helps user interfaces enforce a safer script-security setting<br>
&gt; without causing a FATAL error.<br>
<br>
But does it make sense at all to accept configs that have a --up script<br>
without a sufficiently-high script-security set?<br></blockquote><div><br>This came out of a proposed patch for the GUI to protect lay users from malicious<br></div><div>scripts embedded in config files.<br></div><div><br>Recall the ado about exploiting scripts using unsuspecting &quot;inline&quot; commands.<br>To defeat such exploits we want to enforce a safer script security setting from<br>the GUI but do not want to cause a FATAL error as that would be counter productive.<br><br></div><div>Please see GUI PR #271 <a href="https://github.com/OpenVPN/openvpn-gui/pull/271" target="_blank">https://github.com/OpenVPN/openvpn-gui/pull/271</a> and <br>my comment dated Jul 3 under it.<br></div><div><br></div><div>The discussion that led to this is here:<br><a href="https://github.com/OpenVPN/openvpn-gui/issues/270">https://github.com/OpenVPN/openvpn-gui/issues/270</a><br><br></div><div>Thanks,<br></div><br></div><div class="gmail_quote">Selva<br></div></div></div></div></div>
Steffan Karger Nov. 11, 2018, 1:43 p.m. | #3
Hi,

I've postponed replying to this mail a couple of times because I felt I
missed something and needed to look closer, but today once again I don't
get it. So here goes for a potentially stupid reply:

On 05-10-18 17:30, Selva Nair wrote:
> On Fri, Oct 5, 2018 at 5:44 AM Steffan Karger <steffan@karger.me
> <mailto:steffan@karger.me>> wrote:
> 
>     Hi,
> 
>     On 13-07-18 16:16, selva.nair@gmail.com
>     <mailto:selva.nair@gmail.com> wrote:
>     > From: Selva Nair <selva.nair@gmail.com <mailto:selva.nair@gmail.com>>
>     >
>     > The error is treated as a warning only if its triggered due
>     > to script_security < SSEC_SCRIPTS.
>     >
>     > This helps user interfaces enforce a safer script-security setting
>     > without causing a FATAL error.
> 
>     But does it make sense at all to accept configs that have a --up script
>     without a sufficiently-high script-security set?
> 
> 
> This came out of a proposed patch for the GUI to protect lay users from
> malicious
> scripts embedded in config files.
> 
> Recall the ado about exploiting scripts using unsuspecting "inline"
> commands.
> To defeat such exploits we want to enforce a safer script security
> setting from
> the GUI but do not want to cause a FATAL error as that would be counter
> productive.
> 
> Please see GUI PR #271 https://github.com/OpenVPN/openvpn-gui/pull/271 and
> my comment dated Jul 3 under it.
> 
> The discussion that led to this is here:
> https://github.com/OpenVPN/openvpn-gui/issues/270

I fail to understand why you believe a fatal error in this case is
counterproductive.

A config file that has a script configured, but is not allowed to run
that script is a faulty setup that might result is connections that are
not working properly or maybe even not providing the expected security
level. Consider for example someone changing DNS or proxy settings in a
--up script to avoid leaking data. Most users will never reads warnings
if the connection pretends to be set up correctly. This makes me believe
that the best thing we can do is to error out if someone tries to
connect using such a configuration, and have them review their
configuration.

Why is a fatal error not exactly what you would want?

-Steffan
Selva Nair Nov. 11, 2018, 5:03 p.m. | #4
Hi Steffan,

On Sun, Nov 11, 2018 at 8:43 AM Steffan Karger <steffan@karger.me> wrote:

> Hi,
>
> I've postponed replying to this mail a couple of times because I felt I
> missed something and needed to look closer, but today once again I don't
> get it. So here goes for a potentially stupid reply:
>

Not so fast, likely its my stupidity to push for this.


>
> On 05-10-18 17:30, Selva Nair wrote:
> > On Fri, Oct 5, 2018 at 5:44 AM Steffan Karger <steffan@karger.me
> > <mailto:steffan@karger.me>> wrote:
> >
> >     Hi,
> >
> >     On 13-07-18 16:16, selva.nair@gmail.com
> >     <mailto:selva.nair@gmail.com> wrote:
> >     > From: Selva Nair <selva.nair@gmail.com <mailto:
> selva.nair@gmail.com>>
> >     >
> >     > The error is treated as a warning only if its triggered due
> >     > to script_security < SSEC_SCRIPTS.
> >     >
> >     > This helps user interfaces enforce a safer script-security setting
> >     > without causing a FATAL error.
> >
> >     But does it make sense at all to accept configs that have a --up
> script
> >     without a sufficiently-high script-security set?
> >
> >
> > This came out of a proposed patch for the GUI to protect lay users from
> > malicious
> > scripts embedded in config files.
> >
> > Recall the ado about exploiting scripts using unsuspecting "inline"
> > commands.
> > To defeat such exploits we want to enforce a safer script security
> > setting from
> > the GUI but do not want to cause a FATAL error as that would be counter
> > productive.
> >
> > Please see GUI PR #271 https://github.com/OpenVPN/openvpn-gui/pull/271
> and
> > my comment dated Jul 3 under it.
> >
> > The discussion that led to this is here:
> > https://github.com/OpenVPN/openvpn-gui/issues/270
>
> I fail to understand why you believe a fatal error in this case is
> counterproductive.
>
> A config file that has a script configured, but is not allowed to run
> that script is a faulty setup that might result is connections that are
> not working properly or maybe even not providing the expected security
> level. Consider for example someone changing DNS or proxy settings in a
> --up script to avoid leaking data. Most users will never reads warnings
> if the connection pretends to be set up correctly. This makes me believe
> that the best thing we can do is to error out if someone tries to
> connect using such a configuration, and have them review their
> configuration.
>
> Why is a fatal error not exactly what you would want?
>

When you put it that way I can't disagree. Either I hadn't thought through
it properly, or have forgotten my reasoning (it has been a while since I
tested the GUI patch that prompted this).

I will drop this patch at least for now and go ahead with the
feature to override script security setting from the GUI.

Thanks for your time, and sorry for wasting it.

Selva
<div dir="ltr"><br>Hi Steffan,<div><br><div class="gmail_quote"><div dir="ltr">On Sun, Nov 11, 2018 at 8:43 AM Steffan Karger &lt;<a href="mailto:steffan@karger.me">steffan@karger.me</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
I&#39;ve postponed replying to this mail a couple of times because I felt I<br>
missed something and needed to look closer, but today once again I don&#39;t<br>
get it. So here goes for a potentially stupid reply:<br></blockquote><div><br></div><div>Not so fast, likely its my stupidity to push for this.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 05-10-18 17:30, Selva Nair wrote:<br>
&gt; On Fri, Oct 5, 2018 at 5:44 AM Steffan Karger &lt;<a href="mailto:steffan@karger.me" target="_blank">steffan@karger.me</a><br>
&gt; &lt;mailto:<a href="mailto:steffan@karger.me" target="_blank">steffan@karger.me</a>&gt;&gt; wrote:<br>
&gt; <br>
&gt;     Hi,<br>
&gt; <br>
&gt;     On 13-07-18 16:16, <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a><br>
&gt;     &lt;mailto:<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt; wrote:<br>
&gt;     &gt; From: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a> &lt;mailto:<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;&gt;<br>
&gt;     &gt;<br>
&gt;     &gt; The error is treated as a warning only if its triggered due<br>
&gt;     &gt; to script_security &lt; SSEC_SCRIPTS.<br>
&gt;     &gt;<br>
&gt;     &gt; This helps user interfaces enforce a safer script-security setting<br>
&gt;     &gt; without causing a FATAL error.<br>
&gt; <br>
&gt;     But does it make sense at all to accept configs that have a --up script<br>
&gt;     without a sufficiently-high script-security set?<br>
&gt; <br>
&gt; <br>
&gt; This came out of a proposed patch for the GUI to protect lay users from<br>
&gt; malicious<br>
&gt; scripts embedded in config files.<br>
&gt; <br>
&gt; Recall the ado about exploiting scripts using unsuspecting &quot;inline&quot;<br>
&gt; commands.<br>
&gt; To defeat such exploits we want to enforce a safer script security<br>
&gt; setting from<br>
&gt; the GUI but do not want to cause a FATAL error as that would be counter<br>
&gt; productive.<br>
&gt; <br>
&gt; Please see GUI PR #271 <a href="https://github.com/OpenVPN/openvpn-gui/pull/271" rel="noreferrer" target="_blank">https://github.com/OpenVPN/openvpn-gui/pull/271</a> and<br>
&gt; my comment dated Jul 3 under it.<br>
&gt; <br>
&gt; The discussion that led to this is here:<br>
&gt; <a href="https://github.com/OpenVPN/openvpn-gui/issues/270" rel="noreferrer" target="_blank">https://github.com/OpenVPN/openvpn-gui/issues/270</a><br>
<br>
I fail to understand why you believe a fatal error in this case is<br>
counterproductive.<br>
<br>
A config file that has a script configured, but is not allowed to run<br>
that script is a faulty setup that might result is connections that are<br>
not working properly or maybe even not providing the expected security<br>
level. Consider for example someone changing DNS or proxy settings in a<br>
--up script to avoid leaking data. Most users will never reads warnings<br>
if the connection pretends to be set up correctly. This makes me believe<br>
that the best thing we can do is to error out if someone tries to<br>
connect using such a configuration, and have them review their<br>
configuration.<br>
<br>
Why is a fatal error not exactly what you would want?<br></blockquote><div> </div><div>When you put it that way I can&#39;t disagree. Either I hadn&#39;t thought through</div><div>it properly, or have forgotten my reasoning (it has been a while since I</div><div>tested the GUI patch that prompted this).</div><div><br></div><div>I will drop this patch at least for now and go ahead with the</div><div>feature to override script security setting from the GUI.</div><div><br></div><div>Thanks for your time, and sorry for wasting it.</div><div><br></div><div>Selva</div></div></div></div>

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index d28d1fd..4969565 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -169,13 +169,14 @@  run_up_down(const char *command,
     if (command)
     {
         struct argv argv = argv_new();
+        int flags = (script_security() >= SSEC_SCRIPTS)? S_FATAL : 0;
         ASSERT(arg);
         setenv_str(es, "script_type", script_type);
         argv_parse_cmd(&argv, command);
         argv_printf_cat(&argv, "%s %d %d %s %s %s", arg, tun_mtu, link_mtu,
                         ifconfig_local, ifconfig_remote, context);
         argv_msg(M_INFO, &argv);
-        openvpn_run_script(&argv, es, S_FATAL, "--up/--down");
+        openvpn_run_script(&argv, es, flags, "--up/--down");
         argv_reset(&argv);
     }