| Message ID | 1549917961-632-1-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Exit early when external scripts are specified with script-security < 2 | expand |
On 11/02/2019 21:46, selva.nair@gmail.com wrote: > From: Selva Nair <selva.nair@gmail.com> > > Currently this raises a warning only. A fatal error is triggered > later with a confusing message that script failed to execute. > > This helps the Windows GUI to show a relevant error message when > script-security is over-ridden as a security measure. > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > --- > src/openvpn/init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index 3c44967..5863828 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -3206,7 +3206,7 @@ do_option_warnings(struct context *c) > } > else > { > - msg(M_WARN, "NOTE: starting with " PACKAGE_NAME " 2.1, '--script-security 2' or higher is required to call user-defined scripts or executables"); > + msg(M_FATAL, "ERROR: starting with " PACKAGE_NAME " 2.1, '--script-security 2' or higher is required to call user-defined scripts or executables"); Generally speaking, I am fine with this (so Feature-ACK). What I am struggling with though is that this may break existing configurations for users who do have an invalid configuration file. In this case trying to use scripts without --script-security *and* ignoring that their scripts does not work. The cynical me says "scr** them, they need to fix their configs". But I also got lots of complaints from Fedora users when we changed _incorrect_ configurations to fail in similar ways. It's just amazing how few users who really *read* their log files. So with this in mind, I think this behavioural change should go in 2.5 only. So I can give this a full ACK for git master only.
Hi,
On Fri, Feb 15, 2019 at 09:26:50PM +0100, David Sommerseth wrote:
> So I can give this a full ACK for git master only.
I can see your line of reasoning, but there is another aspect to it - we
want to change the windows GUI to override script-security, to disarm
possibly dangerous .ovpn files that have "--up $eviltrickery" in them.
With this change, the GUI has a chance to tell people "look, this is
the problem. If you trust the source, you can enable scripts in this
config (press <here>), if not, you better leave the ovpn alone!" -
without, things will silently stop working for windows users...
But we *do* want this on windows, disable script-security unless
explicitely activated *outside* the .ovpn file...
(I'm halfway tempted to make this an #ifdef _WIN32, but that cannot
truly be the way forward)
So - I had planned to give this an ACK both for 2.4 and master, but
since you have reservations, we need to find the best way forward.
gert
Hi, On Sat, Feb 16, 2019 at 8:19 AM David Sommerseth < openvpn@sf.lists.topphemmelig.net> wrote: > On 15/02/2019 21:31, Selva Nair wrote: > > Hi > > > > On Fri, Feb 15, 2019 at 3:26 PM David Sommerseth > > <openvpn@sf.lists.topphemmelig.net <mailto: > openvpn@sf.lists.topphemmelig.net>> > > wrote: > > > > On 11/02/2019 21:46, selva.nair@gmail.com <mailto: > selva.nair@gmail.com> wrote: > > > From: Selva Nair <selva.nair@gmail.com <mailto: > selva.nair@gmail.com>> > > > > > > Currently this raises a warning only. A fatal error is triggered > > > later with a confusing message that script failed to execute. > > > > > > This helps the Windows GUI to show a relevant error message when > > > script-security is over-ridden as a security measure. > > > > > > Signed-off-by: Selva Nair <selva.nair@gmail.com > > <mailto:selva.nair@gmail.com>> > > > --- > > > src/openvpn/init.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > > > index 3c44967..5863828 100644 > > > --- a/src/openvpn/init.c > > > +++ b/src/openvpn/init.c > > > @@ -3206,7 +3206,7 @@ do_option_warnings(struct context *c) > > > } > > > else > > > { > > > - msg(M_WARN, "NOTE: starting with " PACKAGE_NAME " 2.1, > > '--script-security 2' or higher is required to call user-defined > scripts > > or executables"); > > > + msg(M_FATAL, "ERROR: starting with " PACKAGE_NAME " > 2.1, > > '--script-security 2' or higher is required to call user-defined > scripts > > or executables"); > > > > Generally speaking, I am fine with this (so Feature-ACK). > > > > What I am struggling with though is that this may break existing > > configurations for users who do have an invalid configuration file. > In this > > case trying to use scripts without --script-security *and* ignoring > that their > > scripts does not work. The cynical me says "scr** them, they need > to fix > > their configs". > > > > I fail to get this.. Users cannot ignore it as currently they do get a > FATAL > > error in such cases. > > I'm only moving the error to happen earlier. > > Am I missing something? > > So another M_FATAL occurs later on? I haven't checked the code yet, but I > was > quite sure there were scenarios where scripts failed to run - with with > some > other complaints in the log. > > If all script hooks results in M_FATAL later on (server and client > configs), > then this is a non-issue. > That got me worried as I had checked this only with up/down scripts on client in which case it causes a FATAL error later in openvpn_run_script. But you are right, not all scripts cause FATAL error when not executed or failed.. In that case, a better option may be to get a proper error msg -- instead of "external program fork failed" we want "script not executed because of script-security < xxx " or something like that.. Selva <div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Feb 16, 2019 at 8:19 AM David Sommerseth <<a href="mailto:openvpn@sf.lists.topphemmelig.net" target="_blank">openvpn@sf.lists.topphemmelig.net</a>> 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 15/02/2019 21:31, Selva Nair wrote:<br> > Hi<br> > <br> > On Fri, Feb 15, 2019 at 3:26 PM David Sommerseth<br> > <<a href="mailto:openvpn@sf.lists.topphemmelig.net" target="_blank">openvpn@sf.lists.topphemmelig.net</a> <mailto:<a href="mailto:openvpn@sf.lists.topphemmelig.net" target="_blank">openvpn@sf.lists.topphemmelig.net</a>>><br> > wrote:<br> > <br> > On 11/02/2019 21:46, <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a> <mailto:<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>> wrote:<br> > > From: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a> <mailto:<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>>><br> > ><br> > > Currently this raises a warning only. A fatal error is triggered<br> > > later with a confusing message that script failed to execute.<br> > ><br> > > This helps the Windows GUI to show a relevant error message when<br> > > script-security is over-ridden as a security measure.<br> > ><br> > > Signed-off-by: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a><br> > <mailto:<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>>><br> > > ---<br> > > src/openvpn/init.c | 2 +-<br> > > 1 file changed, 1 insertion(+), 1 deletion(-)<br> > ><br> > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c<br> > > index 3c44967..5863828 100644<br> > > --- a/src/openvpn/init.c<br> > > +++ b/src/openvpn/init.c<br> > > @@ -3206,7 +3206,7 @@ do_option_warnings(struct context *c)<br> > > }<br> > > else<br> > > {<br> > > - msg(M_WARN, "NOTE: starting with " PACKAGE_NAME " 2.1,<br> > '--script-security 2' or higher is required to call user-defined scripts<br> > or executables");<br> > > + msg(M_FATAL, "ERROR: starting with " PACKAGE_NAME " 2.1,<br> > '--script-security 2' or higher is required to call user-defined scripts<br> > or executables");<br> > <br> > Generally speaking, I am fine with this (so Feature-ACK).<br> > <br> > What I am struggling with though is that this may break existing<br> > configurations for users who do have an invalid configuration file. In this<br> > case trying to use scripts without --script-security *and* ignoring that their<br> > scripts does not work. The cynical me says "scr** them, they need to fix<br> > their configs".<br> > <br> > I fail to get this.. Users cannot ignore it as currently they do get a FATAL<br> > error in such cases.<br> > I'm only moving the error to happen earlier.<br> > Am I missing something?<br> <br> So another M_FATAL occurs later on? I haven't checked the code yet, but I was<br> quite sure there were scenarios where scripts failed to run - with with some<br> other complaints in the log.<br> <br> If all script hooks results in M_FATAL later on (server and client configs),<br> then this is a non-issue.<br></blockquote><div><br></div><div>That got me worried as I had checked this only with up/down scripts on client</div><div>in which case it causes a FATAL error later in openvpn_run_script.</div><div><br></div><div>But you are right, not all scripts cause FATAL error when not executed or failed..</div><div><br></div><div>In that case, a better option may be to get a proper error msg -- instead of "external program fork failed"</div><div>we want "script not executed because of script-security < xxx " or something like that..</div><div><br></div><div>Selva</div></div></div>
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 3c44967..5863828 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -3206,7 +3206,7 @@ do_option_warnings(struct context *c) } else { - msg(M_WARN, "NOTE: starting with " PACKAGE_NAME " 2.1, '--script-security 2' or higher is required to call user-defined scripts or executables"); + msg(M_FATAL, "ERROR: starting with " PACKAGE_NAME " 2.1, '--script-security 2' or higher is required to call user-defined scripts or executables"); } } }