Script security #271

selvanair · 2018-07-03T17:16:58Z

Ref: issue #270

See individual commit messages for details. Here is a summary.

Enforce --script-security 1 but allow other legal values (0 to 3) or -1 to mean do not override the value in the config file
Use a sha1 hash of the file to detect changes and revert the setting to the default
A helper patch to easily handle user preferences for password save (needed for next commit)
Add a dialog for setting the preferred script-security value. Also move some config-specific preferences such as save password to this dialog.

Add a checkbox to Settings->General dialog for enabling a global kill-switch for this feature. I was reluctant to add this but it will be missed by users who know what they are doing.

selvanair · 2018-07-03T22:22:35Z

My tests show this could be very annoying if scripts cause a fatal error when script-security is overridden. So we should consider this only if we can merge something like the patch I proposed for OpenVPN core: patch-395

selvanair · 2018-11-22T18:00:30Z

My tests show this could be very annoying if scripts cause a fatal error when script-security is overridden. So we should consider this only if we can merge something like the patch I proposed for OpenVPN core: patch-395

As discussed in openvpn-devel the fatal error seems to be the right thing to do so this could be merged without needing any changes to the core.

Anyone ready to test this patch?

TinCanTech

"Do not override script security (unsafe)" tested on W7 appears to work as expect.
(other tests to follow)

TinCanTech · 2018-11-25T18:19:17Z

With regard to the script security drop down box, this is not intuitive.
I recommend the drop down box list the numeric value of the setting.
eg:

0. No external commands (Very restrictive, may cause config to malfunction)
1. Built in commands only (Recommended)
2. All scripts (Unsafe)
3. All scripts and access to passwords (Very unsafe)
4. Use config file settings (Extremely unsafe)

I understand 4 is technically -1 but users are not going to understand that.

TinCanTech · 2018-11-25T18:28:19Z

I think that an extra manual "Warning" dialogue should also be considered for any setting above 1.
eg:

Warning: This setting is unsafe
Only use it if you understand the risks. See https://...
Please confirm: [Yes] [Cancel]

selvanair · 2018-11-25T18:59:24Z

Not convinced of the value of including a numerical number as these options do not really correspond to the number in the code (eg., there is no "use config file option" in the core).

The reason for the recommended setting at the top is that is the one selected by default and the best option. But agreed that probably we should re-order at least the rest -- whatever users will find convenient is fine with me.

By the way -- "use config file setting" is a valid option and potentially "extremely unsafe" at the same time: in the end, if the setting the GUI imposes does not match what is in config, the user is very likely going to get a FATAL error -- recall the discussion with Steffan which led to keeping the FATAL error when config wants to run a script but we override it in the GUI.

TinCanTech · 2018-11-25T20:03:57Z

Only with regard to the drop down list with a numeric, does the default option have to be the first on the list ? (I mean, is that required by the windows widget behind it or can the list appear numerically ordered but select 1 by default)
Perhaps a Radio selector instead ? (FYI: this is only feedback as a user)

TinCanTech · 2018-11-25T22:06:53Z

Not convinced of the value of including a numerical number as these options do not really correspond to the number in the code (eg., there is no "use config file option" in the core).

They do correspond, other than the "use Config file" option (which can be made clear in the GUI). Also, that they do correspond reinforces the core values ;-)

TinCanTech

Implement hash functions to enable tracking config file changes; Tested on W7, appears to work as expected. If config file is changed then script security is reset to default.

TinCanTech

Force script-security to SSEC_BUILT_IN by default; tested on W7, apppears to work as expect. Config file is locked while in use. If config is changed then script security is reset to default.

TinCanTech · 2018-11-26T14:05:36Z

All my tests completed with expected results.

Other than the list being out of logical order (which I presume can be corrected) and the possible change to a more appropriate gadget for that list (perhaps an up/down slider or something), I could not find any way to subvert script-security.

selvanair · 2019-01-21T17:41:26Z

This implementation needs a fix. If the user has limited privileges and the config is in the system-wide location, adding the --script-security option from the GUI will cause interactive service to wrongly reject the connection attempt as not authorized.

Options: for configs is in system-wide location, do not enforce a safe script security OR white-list "--script-security" as an option that any user can override. I prefer the latter as on Windows admins are often lay users who could unwittingly install insecure configs in the central location.

selvanair · 2019-02-07T15:45:56Z

I'm in the process of updating this PR like this:

Minimize the dialog about script security setting to just two options: (i) built-in commands only (recommended) and (ii) Use config file setting (potentially unsafe).

The rationale being the purpose of this patch is to force a safe setting unless the user explicitly authorizes to use the value in the config file. The drop-down menu is not for choosing all possible values of script-security --- config file is the place for that. Choices inconsistent with the config is only going to cause an error at runtime.

cron2 · 2019-02-10T17:16:20Z

Hi,

On Thu, Feb 07, 2019 at 03:45:57PM +0000, Selva Nair wrote: I'm in the process of updating this PR like this: Minimize the dialog about script security setting to just two options: (i) built-in commands only (recommended) and (ii) Use config file setting (potentially unsafe). The rationale being the purpose of this patch is to force a safe setting unless the user explicitly authorizes to use the value in the config file. The drop-down menu is not for choosing all possible values of script-security --- config file is the place for that. Choices inconsistent with the config is only going to cause an error at runtime.

Sounds reasonable to me. gert

…

-- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany [email protected]

cron2 · 2019-02-10T17:18:20Z

Hi,

On Mon, Jan 21, 2019 at 09:41:27AM -0800, Selva Nair wrote: Options: for configs is in system-wide location, do not enforce a safe script security OR white-list "--script-security" as an option that any user can override. I prefer the latter as on Windows admins are often lay users who could unwittingly install insecure configs in the central location.

Just for cross-reference - the "white list --script-security" patch was merged to master and release/2.4, so this particular TODO is covered. gert

…

-- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany [email protected]

The setting is persisted in the registry as HKCU\OpenVPN-GUI\config-name\script_security (DWORD) with possible values 0 to 3 that has the same meaning as described in OpenVPN man page or -1 to mean do not override the option (i.e, any value in the config file will be effective). The setting is reverted to default if the file changes (tracked by a digest of the file). This feature is not effective until the next commit where the digest function is implemented. The config file is locked against writes when a connection is active. This ensures that if/when the file is re-read by OpenVPN for a SIGHUP restart the contents have not changed. If no value is found in the registry the setting defaults to 1 (SSEC_BUILT_IN). TODO: add a dialog to edit the setting Signed-off-by: Selva Nair <[email protected]>

- Use a SHA1 hash of the config file to detect file changes and revert the script-security setting to default. Signed-off-by: Selva Nair <[email protected]>

Currently user preference to save password is implicitly set to true if a saved password is found in the registry. This makes it difficult to toggle the save password flag from a dialog if no previously saved password exists. Instead explicitly save the preference. This change helps implementation of the config-specific preferences dialog (next commit). No user-visible changes. Signed-off-by: Selva Nair <[email protected]>

- Add a connection-specific dialog with - options to override in the config file (currently only script-security) - check boxes for saving credentials or clearing saved ones. - stubs for "save username" and change the config display name are added (not enabled). Accessible via tray->config-name->Options popup menu The clear-saved-passwords menu item is removed as that functionality is moved to this dialog. Signed-off-by: Selva Nair <[email protected]>

- Add a check box to the Settings->General dialog to toggle o.disable_ssec_override Signed-off-by: Selva Nair <[email protected]>

If error is due to script security over-ridden by us, reword the message to a localizable string that points the user to the config specific script-security setting. Signed-off-by: Selva Nair <[email protected]>

- copy from the English rc file as is (need translation). Signed-off-by: Selva Nair <[email protected]>

selvanair · 2019-02-12T17:26:02Z

Rebased to master with some changes:

script-security setting in the menu reduced to two choices: "use config file setting (unsafe)" or "built-in scripts only (recommended)" and defaults to the latter as before.
Added a commit that saves the last fatal error from OpenVPN and appends it to the message box shown when a connection fails. In case of script error if the setting is overridden by the GUI the message is reworded as a localizable string that defaults to:

External scripts are disabled for this connection as a security measure. If you must allow such scripts, open the connection specific "Options" menu and select to use config-file setting for "Allow script execution". Note that this is potentially unsafe unless you trust the origin of this connection profile.

For this to work properly OpenVPN requires patch 684 (https://patchwork.openvpn.net/patch/684/).

With this, when script security is set by the GUI failure shows up as

Else if the config file itself has script security < 2 the error shows as

The reason line is appended by this commit.

selvanair · 2019-02-20T17:06:13Z

The more I test this, the more I dislike it. Running a config with a script is no longer easy:

TLDR: there is a question at the end.

Start with a config that has "--up xxx.bat", but no --script-security. Connection fails and GUI shows a message that points the user to the config-specific options menu
User goes to options menu and chooses "Use config file setting (unsafe)" and tries to connect again
Connection fails with an error saying --script-security >= 2 is required
A few who manage to figure out the reason for that may edit the config, add --script-security 2 and try to connect again
Connection fails with the GUI showing a message about safe script-security enforced and points the user to the Options menu
Most give up at that point. A few patient ones go to the Options menu and once again revert the allow script execution to "use config file setting" and try to connect again.
This time the connection succeeds.

Eventually everyone discovers the global "do not override script-security" option and enables that and happily tunnels ever after, oblivious of scripts infiltrating their configs.

How can we make this less frustrating and still safe?

and parse the error msg from OpenVPN proposed in patch OpenVPN#688 (patchwork) (to be squashed with previous commit c7462ab) Signed-off-by: Selva Nair <[email protected]>

selvanair · 2019-02-26T21:07:46Z

Amended the last commit to match the proposed error message from OpenVPN in patch #688

cron2 · 2019-02-28T18:23:29Z

Hi,

On Wed, Feb 20, 2019 at 09:06:15AM -0800, Selva Nair wrote: The more I test this, the more I dislike it. Running a config with a script is no longer easy: TLDR: there is a question at the end. 1. Start with a config that has "--up xxx.bat", but no --script-security. Connection fails and GUI shows a message that points the user to the config-specific options menu 2. User goes to options menu and chooses "Use config file setting (unsafe)" and tries to connect again 3. Connection fails with an error saying --script-security >= 2 is required

Maybe we can make this easier by pointing both at the config file and the gui setting? Like "Your config tries to run external scripts. This is disallowed for security reasons. To allow it, the config file must contain the option 'script-security' with a value >= 2 AND it must be allowed by permitting scripts in the GUI - which should never be done for configs received from an untrusted source, as malicious scripts run from an openvpn config can seriously harm your computer. If the config file is changed, the GUI setting will revert to its default setting and must be re-enabled" Besides this, I couldn't come up with a more user-friendly way that does not involve "parsing the config file" from the GUI. Maybe we need to do this, at least for trivial cases (no nested configs)? Mmmmh. What if we extend openvpn to have a "--disallow-all-scripts" setting that will override --script-security but retain the original value, so the error message (from your new patch :-) ) can print out "--script-security in config has sufficient level, but scripts are disallowed by the GUI" vs. our current "disallowed by script-security setting" error message for "--script-security in the config is not sufficient"? (Yes, a new option. Maybe we need to return to David's original idea about "--security-profile <dummy|user|expert>" which can affect a number of possibly-dangerous config settings at the same time?) gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany [email protected]

selvanair · 2019-02-28T19:12:58Z

Parsing the config is in my TODO as its more generally useful.

I guess the fundamental problem is requiring settings in two different places to match one of which automatically changes in some non-intuitive ways. So, how about always set script-security from the GUI? That is, allow the user to choose all valid script-security settings from the GUI (as I had in a previous version, but no option to use the setting in the config file). And always pass --script-security on the command line. Then the user has to change this only in one place (in the GUI) and whatever is in the config file is never used. Unless globally disabled using the disable-script-security-override checkbox in the main settings.

Whenever the config file is edited we revert the script-security value back to 1. We could show that detail as a tool-tip for the dialog item and also could show a message when its reverted so that the user can edit the setting and then connect or go ahead as is.

Would such an improvement be worth the trouble..?

cron2 · 2019-02-28T21:40:43Z

Hi,

On Thu, Feb 28, 2019 at 11:12:59AM -0800, Selva Nair wrote: Parsing the config is in my TODO as its more generally useful. I guess the fundamental problem is requiring settings in two different places to match one of which automatically changes in some non-intuitive ways. So, how about always set script-security from the GUI? That is, allow the user to choose all valid script-security settings from the GUI (as I had in a previous version, but no option to use the setting in the config file). And always pass --script-security on the command line. Then the user has to change this only in one place (in the GUI) and whatever is in the config file is never used. Unless globally disabled using the disable-script-security-override checkbox in the main settings.

I hear what you're saying, but I'm not sure this is helpful - that way, a GUI user needs to decide, on an .ovpn file given to him by his admin, which value exactly is the right one. "Do I need to select 2 or 3 here? why?"... Maybe with more explanation what the impact is ("no scripts", "scripts", "scripts with full access to password data")... so users have an idea what the effect is. Yes, this might work :-) gert

…

-- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany [email protected]

selvanair · 2019-02-28T23:25:47Z

Yeah, I assumed users have already graduated from OpenVPN U :) But anyway, overriding OpenVPN options can work effectively only if we expect some level of knowledge on part of the users. Unless we just want to ensure that we don't run any scripts without explicit consent from the user but do not care much whether that was informed consent or not..

Back to the drawing board, it seems.

selvanair · 2020-02-27T17:39:56Z

Based all the discussion this needs re-work and I haven't found time for it. For now withdrawing the PR to reduce clutter..

TinCanTech approved these changes Nov 25, 2018

View reviewed changes

TinCanTech approved these changes Nov 26, 2018

View reviewed changes

selvanair mentioned this pull request Jan 21, 2019

Only 50 configurations allowed - need unlimited! #235

Closed

selvanair added 7 commits February 11, 2019 12:41

Implement hash functions to enable tracking config file changes

0138326

- Use a SHA1 hash of the config file to detect file changes and revert the script-security setting to default. Signed-off-by: Selva Nair <[email protected]>

Make the option to disable script security override editable

dd1068e

- Add a check box to the Settings->General dialog to toggle o.disable_ssec_override Signed-off-by: Selva Nair <[email protected]>

Parse fatal errors from OpenVPN and display them when connection fails

c7462ab

If error is due to script security over-ridden by us, reword the message to a localizable string that points the user to the config specific script-security setting. Signed-off-by: Selva Nair <[email protected]>

Copy the "disable_ssec_override" check box to all languages

5177cca

- copy from the English rc file as is (need translation). Signed-off-by: Selva Nair <[email protected]>

selvanair force-pushed the script-security branch from 224403c to 5177cca Compare February 12, 2019 17:17

A fixup for script-security error message

df0f4f1

and parse the error msg from OpenVPN proposed in patch OpenVPN#688 (patchwork) (to be squashed with previous commit c7462ab) Signed-off-by: Selva Nair <[email protected]>

selvanair force-pushed the script-security branch from 88ee3bc to df0f4f1 Compare February 26, 2019 20:56

selvanair closed this Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Script security #271

Script security #271

selvanair commented Jul 3, 2018 •

edited

Loading

selvanair commented Jul 3, 2018

selvanair commented Nov 22, 2018

TinCanTech left a comment •

edited

Loading

TinCanTech commented Nov 25, 2018 •

edited

Loading

TinCanTech commented Nov 25, 2018 •

edited

Loading

selvanair commented Nov 25, 2018

TinCanTech commented Nov 25, 2018 •

edited

Loading

TinCanTech commented Nov 25, 2018 •

edited

Loading

TinCanTech left a comment •

edited

Loading

TinCanTech left a comment

TinCanTech commented Nov 26, 2018

selvanair commented Jan 21, 2019

selvanair commented Feb 7, 2019

cron2 commented Feb 10, 2019 via email

cron2 commented Feb 10, 2019 via email

selvanair commented Feb 12, 2019 •

edited

Loading

selvanair commented Feb 20, 2019 •

edited

Loading

selvanair commented Feb 26, 2019

cron2 commented Feb 28, 2019 via email

selvanair commented Feb 28, 2019

cron2 commented Feb 28, 2019 via email

selvanair commented Feb 28, 2019

selvanair commented Feb 27, 2020

Script security #271

Script security #271

Conversation

selvanair commented Jul 3, 2018 • edited Loading

selvanair commented Jul 3, 2018

selvanair commented Nov 22, 2018

TinCanTech left a comment • edited Loading

Choose a reason for hiding this comment

TinCanTech commented Nov 25, 2018 • edited Loading

TinCanTech commented Nov 25, 2018 • edited Loading

selvanair commented Nov 25, 2018

TinCanTech commented Nov 25, 2018 • edited Loading

TinCanTech commented Nov 25, 2018 • edited Loading

TinCanTech left a comment • edited Loading

Choose a reason for hiding this comment

TinCanTech left a comment

Choose a reason for hiding this comment

TinCanTech commented Nov 26, 2018

selvanair commented Jan 21, 2019

selvanair commented Feb 7, 2019

cron2 commented Feb 10, 2019 via email

cron2 commented Feb 10, 2019 via email

selvanair commented Feb 12, 2019 • edited Loading

selvanair commented Feb 20, 2019 • edited Loading

selvanair commented Feb 26, 2019

cron2 commented Feb 28, 2019 via email

selvanair commented Feb 28, 2019

cron2 commented Feb 28, 2019 via email

selvanair commented Feb 28, 2019

selvanair commented Feb 27, 2020

selvanair commented Jul 3, 2018 •

edited

Loading

TinCanTech left a comment •

edited

Loading

TinCanTech commented Nov 25, 2018 •

edited

Loading

TinCanTech commented Nov 25, 2018 •

edited

Loading

TinCanTech commented Nov 25, 2018 •

edited

Loading

TinCanTech commented Nov 25, 2018 •

edited

Loading

TinCanTech left a comment •

edited

Loading

selvanair commented Feb 12, 2019 •

edited

Loading

selvanair commented Feb 20, 2019 •

edited

Loading