Bug #15758
open
openVPN client exporting for another user and fails to work with ldap.
0%
Description
When a second user database is configured for the VPN, for example ldap, then the client export utility will no longer list the normal users. This is due to the 'authmode' becoming a comma-separated list of values, and it will not exactly match 'Local Database' anymore. Changing the strict comparison to a str_contains fixes this issue.
Furthermore, if a user is given access to the openVPN client export page, they can impersonate any other user on the VPN by downloading their certificate if certificate login only is used. If privileged data is present in a client config (for example, via a client override) then this too can be accessed. This is unexpected functionality.
Besides cluttering up the page, this could be a security issue if no password is used to authenticate to the VPN.
The diff below will make the following changes to client-export:
1. Forbid access to non 'edit-advanced' users for user certificates and client configs other than their own.
2. Forbid access to non 'edit-advanced' users to certificates and client configs from external databases.
3. Fix the bug that prevents vpn export from working when the authmode contains more than one value.
diff --git a/vpn_openvpn_export_old.php b/vpn_openvpn_export.php
index 541ad3e..9cd578e 100644
--- a/vpn_openvpn_export_old.php
+++ b/vpn_openvpn_export.php
@@ -58,6 +58,7 @@ if (!is_array($config['cert'])) {
$a_cert = $config['cert'];
$ras_server = array();
+$user_can_edit_advanced = (isAdminUID($_SESSION['Username']) || userHasPrivilege($user_entry, "page-openvpn-server-advanced") || userHasPrivilege($user_entry, "page-all"));
foreach ($a_server as $server) {
if (isset($server['disable'])) {
continue;
@@ -78,9 +79,11 @@ foreach ($a_server as $server) {
$ecdsagood[] = $cert['refid'];
}
}
-
- if (($server['mode'] == "server_tls_user") && ($server['authmode'] == "Local Database")) {
+ if (($server['mode'] == "server_tls_user") && str_contains($server['authmode'],"Local Database")) {
foreach ($a_user as $uindex => $user) {
+ if(!$user_can_edit_advanced && $user['name'] != $_SESSION['Username']) {
+ continue;
+ }
if (!is_array($user['cert'])) {
continue;
}
@@ -773,6 +776,7 @@ servers[<?=$sindex?>][1][<?=$c?>][3] = '<?=str_replace("'", "\\'", $user['certna
endif;
endforeach;
$c=0;
+ if($user_can_edit_advanced):
foreach ($server['certs'] as $cert): ?>
<?php
if (!$server['crlref'] || !is_cert_revoked($config['cert'][$cert['cindex']], $server['crlref'])): ?>
@@ -783,6 +787,7 @@ servers[<?=$sindex?>][3][<?=$c?>][1] = '<?=str_replace("'", "\\'", $cert['certna
$c++;
endif;
endforeach;
+ endif;
endforeach;
?>
Updated by npr . 23 days ago
This only solves the display issue: there's still another issue where, in this scenario (both local database and another backend) the client export won't correctly put the correct cert in the config file for local database users.
Add the following diff as well to the same file;
@@ -269,7 +272,7 @@ if (!empty($act)) {
}
$want_cert = false;
- if (($srvcfg['mode'] == "server_tls_user") && ($srvcfg['authmode'] == "Local Database")) {
+ if (($srvcfg['mode'] == "server_tls_user") && str_contains($srvcfg['authmode'], "Local Database")) {
if (array_key_exists($usrid, $a_user) &&
array_key_exists('cert', $a_user[$usrid]) &&
array_key_exists($crtid, $a_user[$usrid]['cert'])) {
Updated by npr . 23 days ago
Finally, there's one more file that should be changed; /usr/local/pkg/
// lookup user certificate info
if ($settings['mode'] == "server_tls_user") {
- if ($settings['authmode'] == "Local Database") {
+ if (str_contains($settings['authmode'], "Local Database")) {
$cert = $user['cert'][$crtid];
} else {
$cert = $config['cert'][$crtid];
However, all this only fixes the export for the local database only. It breaks the certs associated with any other method.
For a real solution, the 'authmode' should not be set globally for the entire VPN but different per config file (because a VPN can have multiple authmodes).
A possible workaround is to change the auth settings of the VPN server before exporting the certificates: Set it to auth via local database only, export the local database users, then change to the second mode, export those, etc., then finally set it to the real value.
Of course, that is rather cumbersome and unworkable when letting the users export their own certificate (for example, because they're working remotely on their own devices).