Attention is currently required from: flichtenheld, plaisthos.
Hello plaisthos, flichtenheld,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1332?usp=email
to review the following change.
Change subject: win32: ensure plugin dir has the trailing slash
......................................................................
win32: ensure plugin dir has the trailing slash
This prevents loading plugins from the directories
which share initial characters with the trusted path.
Reported-by: Joshua Rogers <[email protected]>
Found-by: ZeroPath (https://zeropath.com/)
Change-Id: I5ea90594493ab5cb858f7495f773e080b379e8e8
Signed-off-by: Lev Stipakov <[email protected]>
---
M CMakeLists.txt
M src/openvpn/win32.c
2 files changed, 29 insertions(+), 11 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/32/1332/1
diff --git a/CMakeLists.txt b/CMakeLists.txt
index c9301e6..6888de3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -245,6 +245,9 @@
check_symbol_exists(chsize io.h HAVE_CHSIZE)
check_symbol_exists(getrlimit sys/resource.h HAVE_GETRLIMIT)
+# Some old versions of mingw does not have PATHCCH_OPTIONS enums -- add a check
+check_symbol_exists(PATHCCH_ENSURE_TRAILING_SLASH pathcch.h
HAVE_PATHCCH_ENSURE_TRAILING_SLASH)
+
# Some OS (e.g. FreeBSD) need some basic headers to allow
# including network headers
set(NETEXTRA sys/types.h)
@@ -338,7 +341,7 @@
if (WIN32)
target_link_libraries(${target} PUBLIC
ws2_32.lib crypt32.lib fwpuclnt.lib iphlpapi.lib
- wininet.lib setupapi.lib rpcrt4.lib wtsapi32.lib ncrypt.lib
bcrypt.lib)
+ wininet.lib setupapi.lib rpcrt4.lib wtsapi32.lib ncrypt.lib
bcrypt.lib pathcch.lib)
endif ()
endif ()
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 3c11ec3..6aa9463 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -51,6 +51,12 @@
#include "wfp_block.h"
+#include <pathcch.h>
+
+#ifndef HAVE_PATHCCH_ENSURE_TRAILING_SLASH
+#define PATHCCH_ENSURE_TRAILING_SLASH 0x20
+#endif
+
/*
* WFP handle
*/
@@ -1552,12 +1558,12 @@
return false;
}
- WCHAR plugin_dir[MAX_PATH] = { 0 };
+ WCHAR plugin_dir_reg[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)))
+ if (!get_openvpn_reg_value(L"plugin_dir", plugin_dir_reg,
_countof(plugin_dir_reg))
+ && !get_openvpn_reg_value(NULL, plugin_dir_reg,
_countof(plugin_dir_reg)))
{
msg(M_WARN, "Installation path could not be determined.");
}
@@ -1569,26 +1575,35 @@
msg(M_NONFATAL | M_ERRNO, "Failed to get system directory.");
}
- if ((wcslen(plugin_dir) == 0) && (wcslen(system_dir) == 0))
+ if ((wcslen(plugin_dir_reg) == 0) && (wcslen(system_dir) == 0))
{
return false;
}
- WCHAR normalized_plugin_dir[MAX_PATH] = { 0 };
+ WCHAR plugin_dir[MAX_PATH] = { 0 };
- /* Normalize the plugin dir */
- if (wcslen(plugin_dir) > 0)
+ /* normalize and canonicalize the plugin dir and add trailing slash */
+ if (wcslen(plugin_dir_reg) > 0)
{
- if (!GetFullPathNameW(plugin_dir, MAX_PATH, normalized_plugin_dir,
NULL))
+ WCHAR normalized_plugin_dir[MAX_PATH] = { 0 };
+ if (!GetFullPathNameW(plugin_dir_reg, MAX_PATH, normalized_plugin_dir,
NULL))
{
msg(M_NONFATAL | M_ERRNO, "Failed to normalize plugin dir.");
+ }
+
+ HRESULT res = PathCchCanonicalizeEx(plugin_dir, _countof(plugin_dir),
normalized_plugin_dir,
+ PATHCCH_ENSURE_TRAILING_SLASH);
+ if (res != S_OK)
+ {
+ /* doc says we cannot rely on GetLastError() */
+ msg(M_NONFATAL, "Failed to canonicalize 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))
+ if ((wcslen(plugin_dir) > 0)
+ && (wcsnicmp(plugin_dir, plugin_path, wcslen(plugin_dir)) == 0))
{
return true;
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1332?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I5ea90594493ab5cb858f7495f773e080b379e8e8
Gerrit-Change-Number: 1332
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
