Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair.
Hello cron2, flichtenheld, plaisthos, selvanair,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email
to look at the new patch set (#6).
The following approvals got outdated and were removed:
Code-Review+2 by selvanair
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 <[email protected]>
---
M src/openvpnserv/validate.c
1 file changed, 9 insertions(+), 22 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/07/1307/6
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index ddaa381..2dcfe1a 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -24,8 +24,8 @@
#include <lmaccess.h>
#include <shlwapi.h>
-#include <lm.h>
#include <pathcch.h>
+#include <lm.h>
#ifndef HAVE_PATHCCH_ENSURE_TRAILING_SLASH
#define PATHCCH_ENSURE_TRAILING_SLASH 0x20
@@ -58,44 +58,31 @@
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;
- WCHAR config_dir[MAX_PATH];
+ HRESULT res;
+ WCHAR config_path[MAX_PATH];
/* fname = stdin is special: do not treat it as a relative path */
if (wcscmp(fname, L"stdin") == 0)
{
return FALSE;
}
- /* 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, _countof(config_path), workdir,
fname);
}
else
{
- config_file = fname;
+ res = PathCchCanonicalize(config_path, _countof(config_path), fname);
}
- /* canonicalize config_dir and add trailing slash before comparison */
- HRESULT res = PathCchCanonicalizeEx(config_dir, _countof(config_dir),
s->config_dir,
- PATHCCH_ENSURE_TRAILING_SLASH);
-
- if (res == S_OK
- && 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: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4
Gerrit-Change-Number: 1307
Gerrit-PatchSet: 6
Gerrit-Owner: d12fk <[email protected]>
Gerrit-Reviewer: cron2 <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: selvanair <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: cron2 <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
Gerrit-Attention: selvanair <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
