diff mbox series

[Openvpn-devel,v2] win32: fix plugin trusted-dir check prefix bypass

Message ID 20260629124839.32433-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v2] win32: fix plugin trusted-dir check prefix bypass |

Commit Message

Gert Doering June 29, 2026, 12:48 p.m. UTC 
  From: Lev Stipakov <lev@openvpn.net>

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
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
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1736
This mail reflects revision 2 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>

Comments

Gert Doering June 29, 2026, 3:58 p.m. UTC | #1 
This is a somewhat long-standing "unpleasantness" - it was reported twice
as a security vulnerability, so really time to fix it (thanks, Lev).

We do not consider it CVE worthy, as you can't actually achieve anything
with this mechanism that you can't do otherwise (like, "if you can create
a directory here, you already have to have Admin privileges" or "if you
can trick a user into running this .ovpn config, it becomes more of a
social engineering attack").  But it's a bug, as the code doesn't do what
we claim it does, so it's good that it is fixed now ;-)

I have not actively tested it.  I have stared a bit at the code, and
rely on Arne's +2, and all the buildbots claim "things still work"
(though we don't currently test "plugins on a windows client" *ahem*).

There is a unit test (test_misc), which is run by GHA -> tested, works.

Your patch has been applied to the master, release/2.7 and release/2.6
branch.  The unit test in release/2.6 looks different enough that this
needs major work, so I decided to skip that.  The rest of the files
involved had only trivial conflicts in context.

2.5 *does* have the "normalized_plugin_dir" comparison, but since we 
do no releases on 2.5.x anymore, there will never be "fixed windows
binaries".  Who wants that needs to backport & build themselves.

commit c553bb511f074b27334d54a1ce2d4d0c03a9d3e0 (master)
commit 7267b93e8ed3f748af69211a8b9b2e1244dcb79f (release/2.7)
commit 0bb1f2c4ae8c6149bf751aebaeb4da02adfc7a75 (release/2.6)
Author: Lev Stipakov
Date:   Mon Jun 29 14:48:34 2026 +0200

     win32: fix plugin trusted-dir check prefix bypass

     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>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/win32-util.c b/src/openvpn/win32-util.c
index 6fc3be4..209e34e 100644
--- a/src/openvpn/win32-util.c
+++ b/src/openvpn/win32-util.c
@@ -175,4 +175,30 @@ 
     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 6636cf3..4849ea9 100644
--- a/src/openvpn/win32-util.h
+++ b/src/openvpn/win32-util.h
@@ -42,5 +42,17 @@ 
 /* 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 b2a7331..ca08ead 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1587,14 +1587,13 @@ 
     }
 
     /* 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 ccdfdee..fc9840a 100644
--- 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 @@ 
     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)