[Openvpn-devel] win32: Enforce loading of plugins from a trusted directory

Message ID 20240319135355.1279-2-lev@openvpn.net
State Accepted
Headers show
Series [Openvpn-devel] win32: Enforce loading of plugins from a trusted directory | expand

Commit Message

Lev Stipakov March 19, 2024, 1:53 p.m. UTC
Currently, there's a risk associated with allowing plugins to be loaded
from any location. This update ensures plugins are only loaded from a
trusted directory, which is either:

    - HKLM\SOFTWARE\OpenVPN\plugin_dir (or if the key is missing,
    then HKLM\SOFTWARE\OpenVPN, which is installation directory)

    - System directory

Loading from UNC paths is disallowed.

Note: This change affects only Windows environments.

CVE: 2024-27903

Change-Id: I154a4aaad9242c9253a64312a14c5fd2ea95f40d
Reported-by: Vladimir Tokarev <vtokarev@microsoft.com>
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/plugin.c | 18 +++++++++--
 src/openvpn/win32.c  | 77 +++++++++++++++++++++++++++++++++++++-------
 src/openvpn/win32.h  | 27 ++++++++++++++++
 3 files changed, 107 insertions(+), 15 deletions(-)

Comments

Gert Doering March 19, 2024, 4:58 p.m. UTC | #1
Thanks for that.

This patch was sent "with ACK included" to the openvpn-devel@ list because 
it was developed under embargo (CVE), and reviewed and ACKed in a closed
group.  I have verified that this patch is identical to the "v4 one" that
Selva and the original reporter saw and ACKed.

This is related to plugin loading on windows only.  We have discussed the
topic of "restricting plugin loading on other platforms" but it's more
complex to tackle (it starts with "there is no central registry to 
put restrictions into", but goes on to "on unix, openvpn runs as root
anyway, so we expect this to be done by admins who spend some thought
on what scripts and plugin they call, and from which paths") - so we
haven't done anything there yet.

I have test built this on MinGW/Ubuntu, just for completeness, and
via GHA.  Haven't tested the result myself (no plugin setup on windows).

(I do have a few gripes, but these are more cosmetical - like "make
get_openvpn_reg_value() static", and "wrap the long if() condition at 
the '&&', not in the middle of the function call" - but these are all
not important for the functionality)

Your patch has been applied to the master, release/2.6 and release/2.5
branch (security relevant bugfix).

commit aaea545d8a940f761898d736b68bcb067d503b1d (master)
commit 05d321ef980734478a86c5241dad7ba26a748a2f (release/2.6)
commit 30bddb1a5426523ef1d61c8a5df2c613ba2a47d3 (release/2.5)
Author: Lev Stipakov
Date:   Tue Mar 19 15:53:45 2024 +0200

     win32: Enforce loading of plugins from a trusted directory

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <20240319135355.1279-2-lev@openvpn.net>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28416.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
index b4d4a986..3d62eced 100644
--- a/src/openvpn/plugin.c
+++ b/src/openvpn/plugin.c
@@ -277,11 +277,23 @@  plugin_init_item(struct plugin *p, const struct plugin_option *o)
 
 #else  /* ifndef _WIN32 */
 
-    rel = !platform_absolute_pathname(p->so_pathname);
-    p->module = LoadLibraryW(wide_string(p->so_pathname, &gc));
+    WCHAR *wpath = wide_string(p->so_pathname, &gc);
+    WCHAR normalized_plugin_path[MAX_PATH] = {0};
+    /* Normalize the plugin path, converting any relative paths to absolute paths. */
+    if (!GetFullPathNameW(wpath, MAX_PATH, normalized_plugin_path, NULL))
+    {
+        msg(M_ERR, "PLUGIN_INIT: could not load plugin DLL: %ls. Failed to normalize plugin path.", wpath);
+    }
+
+    if (!plugin_in_trusted_dir(normalized_plugin_path))
+    {
+        msg(M_FATAL, "PLUGIN_INIT: could not load plugin DLL: %ls. The DLL is not in a trusted directory.", normalized_plugin_path);
+    }
+
+    p->module = LoadLibraryW(normalized_plugin_path);
     if (!p->module)
     {
-        msg(M_ERR, "PLUGIN_INIT: could not load plugin DLL: %s", p->so_pathname);
+        msg(M_ERR, "PLUGIN_INIT: could not load plugin DLL: %ls", normalized_plugin_path);
     }
 
 #define PLUGIN_SYM(var, name, flags) dll_resolve_symbol(p->module, (void *)&p->var, name, p->so_pathname, flags)
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 6b7ba5e4..7ed32811 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1497,27 +1497,24 @@  openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const for
     return (len >= 0 && len < size);
 }
 
-static BOOL
-get_install_path(WCHAR *path, DWORD size)
+bool
+get_openvpn_reg_value(const WCHAR *key, WCHAR *value, DWORD size)
 {
     WCHAR reg_path[256];
-    HKEY key;
-    BOOL res = FALSE;
+    HKEY hkey;
     openvpn_swprintf(reg_path, _countof(reg_path), L"SOFTWARE\\" PACKAGE_NAME);
 
-    LONG status = RegOpenKeyExW(HKEY_LOCAL_MACHINE, reg_path, 0, KEY_READ, &key);
+    LONG status = RegOpenKeyExW(HKEY_LOCAL_MACHINE, reg_path, 0, KEY_READ, &hkey);
     if (status != ERROR_SUCCESS)
     {
-        return res;
+        return false;
     }
 
-    /* The default value of REG_KEY is the install path */
-    status = RegGetValueW(key, NULL, NULL, RRF_RT_REG_SZ, NULL, (LPBYTE)path, &size);
-    res = status == ERROR_SUCCESS;
+    status = RegGetValueW(hkey, NULL, key, RRF_RT_REG_SZ, NULL, (LPBYTE)value, &size);
 
-    RegCloseKey(key);
+    RegCloseKey(hkey);
 
-    return res;
+    return status == ERROR_SUCCESS;
 }
 
 static void
@@ -1526,7 +1523,7 @@  set_openssl_env_vars()
     const WCHAR *ssl_fallback_dir = L"C:\\Windows\\System32";
 
     WCHAR install_path[MAX_PATH] = { 0 };
-    if (!get_install_path(install_path, _countof(install_path)))
+    if (!get_openvpn_reg_value(NULL, install_path, _countof(install_path)))
     {
         /* if we cannot find installation path from the registry,
          * use Windows directory as a fallback
@@ -1605,4 +1602,60 @@  win32_sleep(const int n)
         }
     }
 }
+
+bool
+plugin_in_trusted_dir(const WCHAR *plugin_path)
+{
+    /* UNC paths are not allowed */
+    if (wcsncmp(plugin_path, L"\\\\", 2) == 0)
+    {
+        msg(M_WARN, "UNC paths for plugins are not allowed.");
+        return false;
+    }
+
+    WCHAR plugin_dir[MAX_PATH] = { 0 };
+
+    /* Attempt to retrieve the trusted plugin directory path from the registry,
+     * using installation path as a fallback */
+    if (!get_openvpn_reg_value(L"plugin_dir", plugin_dir, _countof(plugin_dir))
+        && !get_openvpn_reg_value(NULL, plugin_dir, _countof(plugin_dir)))
+    {
+        msg(M_WARN, "Installation path could not be determined.");
+    }
+
+    /* Get the system directory */
+    WCHAR system_dir[MAX_PATH] = { 0 };
+    if (GetSystemDirectoryW(system_dir, _countof(system_dir)) == 0)
+    {
+        msg(M_NONFATAL | M_ERRNO, "Failed to get system directory.");
+    }
+
+    if ((wcslen(plugin_dir) == 0) && (wcslen(system_dir) == 0))
+    {
+        return false;
+    }
+
+    WCHAR normalized_plugin_dir[MAX_PATH] = { 0 };
+
+    /* Normalize the plugin dir */
+    if (wcslen(plugin_dir) > 0)
+    {
+        if (!GetFullPathNameW(plugin_dir, MAX_PATH, normalized_plugin_dir, NULL))
+        {
+            msg(M_NONFATAL | M_ERRNO, "Failed to normalize plugin dir.");
+            return false;
+        }
+    }
+
+    /* Check if the plugin path resides within the plugin/install directory */
+    if ((wcslen(normalized_plugin_dir) > 0) && (wcsnicmp(normalized_plugin_dir,
+                                                         plugin_path, wcslen(normalized_plugin_dir)) == 0))
+    {
+        return true;
+    }
+
+    /* Fallback to the system directory */
+    return wcsnicmp(system_dir, plugin_path, wcslen(system_dir)) == 0;
+}
+
 #endif /* ifdef _WIN32 */
diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
index aa8513b2..2f147c09 100644
--- a/src/openvpn/win32.h
+++ b/src/openvpn/win32.h
@@ -330,5 +330,32 @@  openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const for
 /* Sleep that can be interrupted by signals and exit event */
 void win32_sleep(const int n);
 
+/**
+ * @brief Fetches a registry value for OpenVPN registry key.
+ *
+ * @param key Registry value name to fetch.
+ * @param value Buffer to store the fetched string value.
+ * @param size Size of `value` buffer in bytes.
+ * @return `true` if successful, `false` otherwise.
+ */
+bool
+get_openvpn_reg_value(const WCHAR *key, WCHAR *value, DWORD size);
+
+/**
+ * @brief Checks if a plugin is located in a trusted directory.
+ *
+ * Verifies the plugin's path against a trusted directory, which is:
+ *
+ * - "plugin_dir" registry value or installation path, if the registry key is missing
+ * - system directory
+ *
+ * UNC paths are explicitly disallowed.
+ *
+ * @param plugin_path Normalized path to the plugin.
+ * @return \c true if the plugin is in a trusted directory and not a UNC path; \c false otherwise.
+ */
+bool
+plugin_in_trusted_dir(const WCHAR *plugin_path);
+
 #endif /* ifndef OPENVPN_WIN32_H */
 #endif /* ifdef _WIN32 */