Todo #9511
closedOpenVPN server/client/override advanced settings privilege separation
100%
Description
This issue needs some additional thought and debate.
Due to advanced directives in OpenVPN it is possible for users to gain elevated privileges since they can run commands or scripts (#9510). Many users rely on these commands to make OpenVPN behave as they want, so we can't just filter them out.
We could create a new privilege to (dis)allow editing the advanced settings on clients, servers, and overrides. Though splitting the advanced editing privilege might be OK, it poses its own set of tricky questions:
- If a user without privileges to edit the box edits an entry with advanced settings, are they preserved or cleared? (preserved seems best)
- If a user with access to delete OpenVPN instances doesn't also have access to edit the advanced settings, should they be allowed to delete an instance with an advanced configuration set? If so, they could remove advanced settings by deleting the instances and creating a new one without advanced settings. While not technically a path to elevated privileges, it could lead to unintended network access or behavior if they were able to remove custom routing commands.
- Do we add that new privilege to all users with access to edit those pages on upgrade, or do leave it up to the user to discover that they can no longer edit the advanced section? Either way is bound to cause lots of support questions. Using a negative form such as "deny advanced edit access" is bound to be even more trouble, given the issues users have understanding "Deny config write".
A new privilege would look like this, in src/etc/inc/priv/user.priv.inc
$priv_list['page-openvpn-server-advanced'] = array(); $priv_list['page-openvpn-server-advanced']['name'] = gettext("WebCfg - OpenVPN: Edit Clients Advanced"); $priv_list['page-openvpn-server-advanced']['descr'] = gettext("Allow edit access to the 'OpenVPN: Clients' Advanced settings field."); $priv_list['page-openvpn-server-advanced']['warn'] = "standard-warning-root"; $priv_list['page-openvpn-csc-advanced'] = array(); $priv_list['page-openvpn-csc-advanced']['name'] = gettext("WebCfg - OpenVPN: Client Specific Override Advanced"); $priv_list['page-openvpn-csc-advanced']['descr'] = gettext("Allow edit access to the 'OpenVPN: Client Specific Override' advanced settings field."); $priv_list['page-openvpn-csc-advanced']['warn'] = "standard-warning-root"; $priv_list['page-openvpn-client-advanced'] = array(); $priv_list['page-openvpn-client-advanced']['name'] = gettext("WebCfg - OpenVPN: Edit Servers Advanced"); $priv_list['page-openvpn-client-advanced']['descr'] = gettext("Allow edit access to the 'OpenVPN: Servers' Advanced settings field."); $priv_list['page-openvpn-client-advanced']['warn'] = "standard-warning-root";
And then on vpn_openvpn_client.php, vpn_openvpn_csc.php, and vpn_openvpn_server.php the code would need to test for this privilege before allowing changes to Advanced settings.
I'd like to see this in 2.5.0 but if we can't reach a viable consensus then it can be nudged forward to the next release.
Updated by Jim Pingle over 5 years ago
If or when this is implemented, the warnings added for #9510 can be removed.
Updated by Jim Pingle over 5 years ago
- Target version changed from 2.5.0 to 2.4.4-p3
Updated by Jim Pingle over 5 years ago
- Removed Advanced options from the OpenVPN wizard. If a user has privileges for it, they can add the settings later.
- Added new privileges for OpenVPN servers, clients, and overrides which allows for editing advanced options
- These new privileges are NOT added to users automatically -- erring on the side of security
- Admin users (the admin user or users with page-all) and users with the new privilege(s) can always alter advanced options
- Added input validation to warn the user they can't change advanced options if they try, which means:
- A user without the new privilege can edit existing entries so long as they do not alter the advanced options. The form field is disabled, and if the user submits a form with the value changed, input validation rejects the change
- A user without the new privilege may create new entries so long as they do not contain advanced options
- A user without the new privilege may NOT delete an instance if it contains advanced options which they are not allowed to change
Updated by Jim Pingle over 5 years ago
- Status changed from New to Feedback
- % Done changed from 0 to 100
Applied in changeset 4a1841a1fabcba0100f6a4f505fc1e132c29da20.
Updated by Jim Pingle over 5 years ago
- Status changed from Feedback to Resolved
- Private changed from Yes to No