[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better

[d12fk (C. Review) <ge...@op...>]

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/+/1307?usp=email

to review the following change.


Change subject: iservice: validate config path better
......................................................................

iservice: validate config path better

Instead of just rejecting any path that contains ".." use some WIN32 API
functions to combine, canonicalize and then check if the resulting
path is located under the config directory. Makes the code prettier
and more correct.

Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4
Signed-off-by: Heiko Hund <he...@is...>
---
M src/openvpnserv/CMakeLists.txt
M src/openvpnserv/validate.c
2 files changed, 10 insertions(+), 19 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/07/1307/1

diff --git a/src/openvpnserv/CMakeLists.txt b/src/openvpnserv/CMakeLists.txt
index 340b904..a30b164 100644
--- a/src/openvpnserv/CMakeLists.txt
+++ b/src/openvpnserv/CMakeLists.txt
@@ -31,7 +31,7 @@
     )
 target_link_libraries(openvpnserv
     advapi32.lib userenv.lib iphlpapi.lib fwpuclnt.lib rpcrt4.lib
-    shlwapi.lib netapi32.lib ws2_32.lib ntdll.lib ole32.lib)
+    shlwapi.lib netapi32.lib ws2_32.lib ntdll.lib ole32.lib pathcch.lib)
 if (MINGW)
     target_compile_options(openvpnserv PRIVATE -municode)
     target_link_options(openvpnserv PRIVATE -municode)
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index 59d5b86..675f8a8 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -24,6 +24,7 @@
 
 #include <lmaccess.h>
 #include <shlwapi.h>
+#include <pathcch.h>
 #include <lm.h>
 
 static const WCHAR *white_list[] = {
@@ -53,36 +54,26 @@
 static PTOKEN_GROUPS GetTokenGroups(const HANDLE token);
 
 /*
- * Check workdir\fname is inside config_dir
- * The logic here is simple: we may reject some valid paths if ..\ is in any of the strings
+ * Check that config path is inside config_dir
+ * The logic here is simple: if the path isn't prefixed with config_dir it's rejected
  */
 static BOOL
 CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s)
 {
-    WCHAR tmp[MAX_PATH];
-    const WCHAR *config_file = NULL;
-    const WCHAR *config_dir = NULL;
+    HRESULT res;
+    WCHAR config_path[MAX_PATH];
 
-    /* convert fname to full path */
+    /* convert fname to full canonical path */
     if (PathIsRelativeW(fname))
     {
-        swprintf(tmp, _countof(tmp), L"%ls\\%ls", workdir, fname);
-        config_file = tmp;
+        res = PathCchCombine(config_path, sizeof(config_path), workdir, fname);
     }
     else
     {
-        config_file = fname;
+        res = PathCchCanonicalize(config_path, sizeof(config_path), fname);
     }
 
-    config_dir = s->config_dir;
-
-    if (wcsncmp(config_dir, config_file, wcslen(config_dir)) == 0
-        && wcsstr(config_file + wcslen(config_dir), L"..") == NULL)
-    {
-        return TRUE;
-    }
-
-    return FALSE;
+    return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0;
 }
 
 

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1307?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: I0e94068f467f2899daf133b032a785d2d7fc05e4
Gerrit-Change-Number: 1307
Gerrit-PatchSet: 1
Gerrit-Owner: d12fk <he...@op...>
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...>