Thread: [Openvpn-devel] [S] Change in openvpn[master]: win32: ensure plugin dir has the trailing slash

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

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 <co...@jo...>
Found-by: ZeroPath (https://zeropath.com/)

Change-Id: I5ea90594493ab5cb858f7495f773e080b379e8e8
Signed-off-by: Lev Stipakov <le...@op...>
---
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 <lst...@gm...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>


Thread: [Openvpn-devel] [S] Change in openvpn[master]: win32: ensure plugin dir has the trailing slash

Robust and flexible VPN network tunnelling

openvpn-devel