From: Lev Stipakov <[email protected]> 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 <[email protected]> Acked-by: Arne Schwabe <[email protected]> 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 <[email protected]> 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) _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
