win32: fix plugin trusted-dir check prefix bypass

plugin_in_trusted_dir() validated the plugin path against the trusted
plugin/install directory (and the system directory fallback) using a raw
string prefix match via wcsnicmp(). When the trusted directory path does
not end in a separator (e.g. the plugin_dir registry value is set to
"C:\openvpn_plugins"), a sibling directory sharing the same prefix
("C:\openvpn_plugins_evil") also passes the check, allowing a plugin to
be loaded from outside the allow-listed directory.

Introduce win_path_in_dir() in win32-util.c which performs the prefix
match but additionally requires the match to end on a path-component
boundary, and use it for both the plugin/install directory and the
system directory checks. Add unit tests in test_misc.c.

Change-Id: Ib7f9c9ce5ed778190445cc4cfaa8f3cd5d1110bc
Reported-By: 章鱼哥 (@AiPy) (www.aipyaipy.com)
Github: OpenVPN/openvpn-private-issues#120
Reported-by: <genco@srlabs.de>
Github: OpenVPN/openvpn-private-issues#109
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1736
Message-Id: <20260629124839.32433-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg37382.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/win32-util.c b/src/openvpn/win32-util.c
index 6fc3be4b20659b335af84028f1248b6310f3f701..209e34ead4ff840b54113818d2196147b99a2d66 100644 (file)
--- a/src/openvpn/win32-util.c
+++ b/src/openvpn/win32-util.c
@@ -175,4 +175,30 @@ win_get_tempdir(void)
     return tmpdir;
 }
 
+bool
+win_path_in_dir(const WCHAR *path, const WCHAR *dir)
+{
+    size_t dir_len = wcslen(dir);
+    /* dir_len <= 1 guards the dir[dir_len - 1] access below and rejects a
+     * degenerate single-character directory (a normalized absolute path is
+     * always longer). */
+    if (dir_len <= 1 || wcsnicmp(dir, path, dir_len) != 0)
+    {
+        return false;
+    }
+
+    /* A plain prefix match is not sufficient: if dir is "C:\foo" then
+     * "C:\foo_evil\bar.dll" shares the prefix but is not inside "C:\foo".
+     * Require that the matched prefix ends on a path separator, i.e. either
+     * dir already ends with a separator or the character following the prefix
+     * in path is one. */
+    if (dir[dir_len - 1] == L'\\' || dir[dir_len - 1] == L'/')
+    {
+        return true;
+    }
+
+    WCHAR next = path[dir_len];
+    return next == L'\\' || next == L'/';
+}
+
 #endif /* _WIN32 */

diff --git a/src/openvpn/win32-util.h b/src/openvpn/win32-util.h
index 6636cf3c47bf62b05681ba19388005f79425f84f..4849ea9f70719ab0a5308a4aa72681cad771b5d8 100644 (file)
--- a/src/openvpn/win32-util.h
+++ b/src/openvpn/win32-util.h
@@ -42,5 +42,17 @@ bool win_safe_filename(const char *fn);
 /* Find temporary directory */
 const char *win_get_tempdir(void);
 
+/**
+ * Check whether @p path resides within directory @p dir.
+ *
+ * Unlike a plain string prefix match, this requires the match to end on a
+ * path-component boundary, so that "C:\\foo_evil\\bar" is not considered to be
+ * inside "C:\\foo". Both arguments are expected to be normalized absolute
+ * paths. The comparison is case-insensitive.
+ *
+ * @return true if @p path is inside @p dir, false otherwise.
+ */
+bool win_path_in_dir(const WCHAR *path, const WCHAR *dir);
+
 #endif /* OPENVPN_WIN32_UTIL_H */
 #endif /* ifdef _WIN32 */

diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index b2a733196b04f204b9d470aac5877e4be22a5240..ca08ead5c5e4b9f5f5dc3d90e0ab1c87e922485f 100644 (file)
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1587,14 +1587,13 @@ plugin_in_trusted_dir(const WCHAR *plugin_path)
     }
 
     /* 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))
+    if (win_path_in_dir(plugin_path, normalized_plugin_dir))
     {
         return true;
     }
 
     /* Fallback to the system directory */
-    return wcsnicmp(system_dir, plugin_path, wcslen(system_dir)) == 0;
+    return win_path_in_dir(plugin_path, system_dir);
 }
 
 bool

diff --git a/tests/unit_tests/openvpn/test_misc.c b/tests/unit_tests/openvpn/test_misc.c
index ccdfdeed4500b4ed230f79b243cad758b251e9ca..fc9840a02116569b35e8ef4df79b669099dc2816 100644 (file)
--- a/tests/unit_tests/openvpn/test_misc.c
+++ b/tests/unit_tests/openvpn/test_misc.c
@@ -38,6 +38,9 @@
 #include "test_common.h"
 #include "list.h"
 #include "mock_msg.h"
+#ifdef _WIN32
+#include "win32-util.h"
+#endif
 
 static void
 test_compat_lzo_string(void **state)
@@ -445,12 +448,48 @@ test_atoi_variants(void **state)
     mock_set_debug_level(saved_log_level);
 }
 
-const struct CMUnitTest misc_tests[] = { cmocka_unit_test(test_compat_lzo_string),
-                                         cmocka_unit_test(test_auth_fail_temp_no_flags),
-                                         cmocka_unit_test(test_auth_fail_temp_flags),
-                                         cmocka_unit_test(test_auth_fail_temp_flags_msg),
-                                         cmocka_unit_test(test_list),
-                                         cmocka_unit_test(test_atoi_variants) };
+#ifdef _WIN32
+static void
+test_win_path_in_dir(void **state)
+{
+    /* plugin/install dir without trailing separator */
+    assert_true(win_path_in_dir(L"C:\\openvpn_plugins\\foo.dll", L"C:\\openvpn_plugins"));
+
+    /* the bug being fixed: a sibling dir sharing the prefix must NOT match */
+    assert_false(win_path_in_dir(L"C:\\openvpn_plugins_evil\\foo.dll", L"C:\\openvpn_plugins"));
+
+    /* trusted dir with trailing separator */
+    assert_true(win_path_in_dir(L"C:\\openvpn_plugins\\foo.dll", L"C:\\openvpn_plugins\\"));
+    assert_false(win_path_in_dir(L"C:\\openvpn_plugins_evil\\foo.dll", L"C:\\openvpn_plugins\\"));
+
+    /* forward slash separator in the candidate path is accepted */
+    assert_true(win_path_in_dir(L"C:\\openvpn_plugins/foo.dll", L"C:\\openvpn_plugins"));
+
+    /* comparison is case-insensitive */
+    assert_true(win_path_in_dir(L"c:\\OPENVPN_PLUGINS\\foo.dll", L"C:\\openvpn_plugins"));
+
+    /* the directory itself (no trailing component) is not "in" the directory */
+    assert_false(win_path_in_dir(L"C:\\openvpn_plugins", L"C:\\openvpn_plugins"));
+
+    /* nested subdirectories are still inside */
+    assert_true(win_path_in_dir(L"C:\\openvpn_plugins\\sub\\foo.dll", L"C:\\openvpn_plugins"));
+
+    /* an empty trusted dir never matches */
+    assert_false(win_path_in_dir(L"C:\\openvpn_plugins\\foo.dll", L""));
+}
+#endif /* _WIN32 */
+
+const struct CMUnitTest misc_tests[] = {
+#ifdef _WIN32
+    cmocka_unit_test(test_win_path_in_dir),
+#endif
+    cmocka_unit_test(test_compat_lzo_string),
+    cmocka_unit_test(test_auth_fail_temp_no_flags),
+    cmocka_unit_test(test_auth_fail_temp_flags),
+    cmocka_unit_test(test_auth_fail_temp_flags_msg),
+    cmocka_unit_test(test_list),
+    cmocka_unit_test(test_atoi_variants)
+};
 
 int
 main(void)