Make up/down script errors not FATAL

PATCH Superseded [email protected]... 6 participants 13 messages Jul 3, 2018 – Nov 11, 2018

lore

Share & tools

lore

[email protected]...

author

2018-07-03 01:24 UTC #0 lore raw

PATCH 1/1 Make up/down script errors not FATAL

From: Selva Nair <[email protected]...>

Instead log only a warning.

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

Signed-off-by Selva Nair <[email protected]...>

---
Note: All other scripts are called with flag = 0 and will only trigger a warning message if openvpn_execve fails.

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

src/openvpn/init.c +1 -1

index b748357..6673734 100644
@@ -174,7 +174,7 @@ run_up_down(const char *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, 0, "--up/--down");
         argv_reset(&argv);
     }

Signature · 1 line

2.1.4

Jonathan K. Bullard

2018-07-03 03:13 UTC #1 lore raw

Hi.

Quoted 7 lines · On Mon, Jul 2, 2018 at 9:24 PM, <[email protected]...> wrote:

From: Selva Nair <[email protected]...>
Instead log only a warning.
This helps user interfaces enforce a safer script-security setting without causing a FATAL error.

Can you expand on that? What "safer script secuity settings' do you have in mind? Tunnelblick (and I think all Linux) use script-security 2 to allow for up/down scripts that implement DNS and other settings.

My initial reaction is that I'd rather a problem in the up/down scripts generates a fatal error, so if there's a problem in the Tunnelblick scripts somebody will report it. In my experience, almost nobody pays attention to warnings, and mostly, those who do are worried about warning that don't matter.

Best regards,

Jon Bullard

Selva Nair

author

2018-07-03 06:49 UTC #2 lore raw

Hi Jon,

Quoted 16 lines · Jonathan K. Bullard

Hi.
Quoted 7 lines · On Mon, Jul 2, 2018 at 9:24 PM, <[email protected]...> wrote:
From: Selva Nair <[email protected]...>
Instead log only a warning.
This helps user interfaces enforce a safer script-security setting without causing a FATAL error.
Can you expand on that? What "safer script secuity settings' do you have in mind? Tunnelblick (and I think all Linux) use script-security 2 to allow for up/down scripts that implement DNS and other settings.

Quoted 5 lines

My initial reaction is that I'd rather a problem in the up/down scripts generates a fatal error, so if there's a problem in the Tunnelblick scripts somebody will report it. In my experience, almost nobody pays attention to warnings, and mostly, those who do are worried about warning that don't matter.

This is in reaction to

https://medium.com/tenable-techblog/reverse-shell-from-an-
openvpn-configuration-file-73fd8b1d38da

In OpenVPN Windows GUI I'm considering to enforce "--script-security 1" (SSEC_BUILT_IN). See the discussion here:

https://github.com/OpenVPN/openvpn-gui/issues/270

Selva

David Sommerseth

2018-07-03 08:23 UTC #3 lore raw

Quoted 26 lines · Selva Nair

Hi Jon,
Quoted 3 lines · On Mon, Jul 2, 2018 at 11:13 PM, Jonathan K. Bullard <[email protected]...…
Hi.
On Mon, Jul 2, 2018 at 9:24 PM, <[email protected]...
Quoted 18 lines · <mailto:[email protected]...>> wrote:
Quoted 7 lines
From: Selva Nair <[email protected]... <mailto:[email protected]...>>
Instead log only a warning.
This helps user interfaces enforce a safer script-security setting without causing a FATAL error.
Can you expand on that? What "safer script secuity settings' do you have in mind? Tunnelblick (and I think all Linux) use script-security 2 to allow for up/down scripts that implement DNS and other settings.
My initial reaction is that I'd rather a problem in the up/down scripts generates a fatal error, so if there's a problem in the Tunnelblick scripts somebody will report it. In my experience, almost nobody pays attention to warnings, and mostly, those who do are worried about warning that don't matter.

Quoted 10 lines

This is in reaction to
https://medium.com/tenable-techblog/reverse-shell-from-an-openvpn-configuration-file-73fd8b1d38da
<https://medium.com/tenable-techblog/reverse-shell-from-an-openvpn-configuration-file-73fd8b1d38da>
In OpenVPN Windows GUI I'm considering to enforce "--script-security 1" (SSEC_BUILT_IN). See the discussion here:
https://github.com/OpenVPN/openvpn-gui/issues/270

This I am much more in favour of. I've already added a longer GitHub comment with a bit different perspective, as well as looking more into the future of what we're doing with OpenVPN 3 - where OpenVPN processes generally will not run any scripts or even support it.

TL;DR: Reduce the possibility to run scripts to an absolute minimum (if at all). If having this possibility run them with as few privileges as possible, and scripts to run is preferred to be configured outside of the OpenVPN configuration file.

The latter argument of configuring scripts outside of the configuration file is simply trying to end up with a single configuration file which would be functional on all devices. A configuration file with Windows scripts won't work on a non-Windows box and vice versa - some configuration files might not even work across Linux distributions even. So let the OpenVPN configuration files be as generic as possible, focusing on getting a connection to a remote server. And configure the rest outside of the OpenVPN configuration profile.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Inc

Antonio Quartulli

2018-07-03 08:32 UTC #4 lore raw

Hi,

Quoted 5 lines · David Sommerseth

TL;DR: Reduce the possibility to run scripts to an absolute minimum (if at all). If having this possibility run them with as few privileges as possible, and scripts to run is preferred to be configured outside of the OpenVPN configuration file.

Quoted 8 lines

The latter argument of configuring scripts outside of the configuration file is simply trying to end up with a single configuration file which would be functional on all devices. A configuration file with Windows scripts won't work on a non-Windows box and vice versa - some configuration files might not even work across Linux distributions even. So let the OpenVPN configuration files be as generic as possible, focusing on getting a connection to a remote server. And configure the rest outside of the OpenVPN configuration profile.

I have previously proposed to use an udev-compatible mechanism to run scripts.
In this scenario OpenVPN only needs to trigger "signals" and then whoever is listening (i.e. udev/hotplug) will take care of handling them. This could even be DBus driven.

However, this can work on Linux. Anybody knows of a similar mechanism for Windows and macOS?

Cheers,

Signature · 1 line

Antonio Quartulli

Gert Doering

2018-07-03 07:09 UTC #5 lore raw

In reply to Jonathan K. Bullard

Hi,

Quoted 5 lines · Jonathan K. Bullard

My initial reaction is that I'd rather a problem in the up/down scripts generates a fatal error, so if there's a problem in the Tunnelblick scripts somebody will report it. In my experience, almost nobody pays attention to warnings, and mostly, those who do are worried about warning that don't matter.

Selva Nair

author

2018-07-03 14:31 UTC #6 lore raw

Hi,

Quoted 9 lines · Gert Doering

Hi,
Quoted 5 lines · Jonathan K. Bullard
My initial reaction is that I'd rather a problem in the up/down scripts generates a fatal error, so if there's a problem in the Tunnelblick scripts somebody will report it. In my experience, almost nobody pays attention to warnings, and mostly, those who do are worried about warning that don't matter.

Quoted 9 lines

From how I read Selva's mail, an error in the script will still create a fatal error.
The difference is that today, if you have --script-security 1 and a --up config, that combination will cause an error, while after the change, this will only cause a warning.
Selva, did I read that correctly?

Unfortunately no. This patch will trigger only a warning for both a script error
and inability execute the script due to script-security setting.

If actual errors in up/down scripts should trigger M_FATAL, we can change the
patch to just bypass the script execution if script security is < 2. It would be a
bit ugly like this:

- openvpn_run_script(&argv, es, 0, "--up/--down");
+ openvpn_run_script(&argv, es, (script_security >= SSEC_SCRIPTS)?
S_FATAL : 0, "--up/--down");

For some reason the code path involved is somewhat convoluted:

First we log a warning that external scripts require script_security >= 2. But fully knowing its going to fail we still call openvpn_run_script(). The flag
that say error out or warn is set in this call and script permission is checked just before executing:

openvpn_run_script() --> openvpn_execve_check() --> openvpn_execve_allowed()

When the latter returns an error due to script-security, openvpn_execve_check()
fails with a slightly misleading message.

Selva

[email protected]...

author

2018-07-03 15:58 UTC #7 lore raw

PATCH v2 1/1 Make up/down script errors not FATAL

In reply to [email protected]...

From: Selva Nair <[email protected]...>

Treat the error as not FATAL 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 <[email protected]...>

---
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(-)

src/openvpn/init.c +2 -1

index b748357..074a2d3 100644
@@ -168,13 +168,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);
     }

Signature · 1 line

2.1.4

[email protected]...

author

2018-07-13 14:16 UTC #8 lore raw

PATCH v3 1/1 Make up/down script errors not FATAL

From: Selva Nair <[email protected]...>

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 <[email protected]...>

---
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(-)

src/openvpn/init.c +2 -1

index d28d1fd..4969565 100644
@@ -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);
     }

Signature · 1 line

2.1.4

Steffan Karger

2018-10-05 09:43 UTC #9 lore raw

Hi,

Quoted 7 lines · On 13-07-18 16:16, [email protected]... wrote:

From: Selva Nair <[email protected]...>
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

author

2018-10-05 15:30 UTC #10 lore raw

Hi,

Quoted 5 lines · Steffan Karger

Hi,
Quoted 2 lines · On 13-07-18 16:16, [email protected]... wrote:
From: Selva Nair <[email protected]...>

Quoted 9 lines

Quoted 5 lines
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

Steffan Karger

2018-11-11 13:43 UTC #11 lore raw

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:

Quoted 30 lines · Selva Nair

On Fri, Oct 5, 2018 at 5:44 AM Steffan Karger <[email protected]... <mailto:[email protected]...>> wrote:
Hi,
On 13-07-18 16:16, [email protected]... <mailto:[email protected]...> wrote:
> From: Selva Nair <[email protected]... <mailto:[email protected]...>> >
> 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.

Quoted 5 lines

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

author

2018-11-11 17:03 UTC #12 lore raw

Hi Steffan,

Quoted 6 lines · Steffan Karger

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.

Quoted 54 lines · Selva Nair

Quoted 8 lines
On Fri, Oct 5, 2018 at 5:44 AM Steffan Karger <[email protected]... <mailto:[email protected]...>> wrote:
    Hi,

    On 13-07-18 16:16, [email protected]...
    <mailto:[email protected]...> wrote:
    > From: Selva Nair <[email protected]... <mailto:
[email protected]...>>
Quoted 8 lines
Quoted 6 lines
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
Quoted 7 lines
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.
Quoted 8 lines
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
Quoted 4 lines
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.

Quoted 2 lines

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