[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) 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 Acked-by: Lev Stipakov Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1307 Message-Id: <[email protected]> URL: https://www.mail-archive.com/[email protected]/msg34336.html Signed-off-by: Gert Doering --- M src/openvpnserv/validate.c 1 file changed, 9 insertions(+), 22 deletions(-) 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 #include -#include #include +#include #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: merged Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 8 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-Reviewer: selvanair Gerrit-Reviewer: stipa Gerrit-CC: openvpn-devel ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
cron2 has uploaded a new patch set (#8) to the change originally created by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) The following approvals got outdated and were removed: Code-Review+1 by cron2, Code-Review+2 by stipa 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 Acked-by: Lev Stipakov Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1307 Message-Id: <[email protected]> URL: https://www.mail-archive.com/[email protected]/msg34336.html Signed-off-by: Gert Doering --- M src/openvpnserv/validate.c 1 file changed, 9 insertions(+), 22 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/07/1307/8 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 #include -#include #include +#include #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: 8 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-Reviewer: selvanair Gerrit-Reviewer: stipa Gerrit-CC: openvpn-devel ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: d12fk, flichtenheld, plaisthos, selvanair. stipa has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 7: Code-Review+2 (1 comment) Patchset: PS7: Built and tested, looks good. -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 7 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-Reviewer: selvanair Gerrit-Reviewer: stipa Gerrit-CC: openvpn-devel Gerrit-Attention: plaisthos Gerrit-Attention: flichtenheld Gerrit-Attention: d12fk Gerrit-Attention: selvanair Gerrit-Comment-Date: Wed, 12 Nov 2025 09:14:02 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: d12fk, flichtenheld, plaisthos, selvanair. cron2 has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 6: Code-Review+1 (1 comment) Patchset: PS6: I withdraw my -2, as that has been addressed by the new version and Selva +2'ing it already. -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 6 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-Reviewer: selvanair Gerrit-CC: openvpn-devel Gerrit-Attention: plaisthos Gerrit-Attention: flichtenheld Gerrit-Attention: d12fk Gerrit-Attention: selvanair Gerrit-Comment-Date: Mon, 10 Nov 2025 22:59:36 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
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
---
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
#include
-#include
#include
+#include
#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
Gerrit-Reviewer: cron2
Gerrit-Reviewer: flichtenheld
Gerrit-Reviewer: plaisthos
Gerrit-Reviewer: selvanair
Gerrit-CC: openvpn-devel
Gerrit-Attention: plaisthos
Gerrit-Attention: cron2
Gerrit-Attention: flichtenheld
Gerrit-Attention: selvanair
___
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, flichtenheld, plaisthos. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 5: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/0bc45645_e107cbf0?usp=email : PS3, Line 85: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > For got to mention that this was reported by: […] I think I'll break it out into its own commit then. -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 5 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-Reviewer: selvanair Gerrit-CC: openvpn-devel Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Comment-Date: Fri, 07 Nov 2025 16:30:34 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk Comment-In-Reply-To: selvanair ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos. selvanair has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 5: Code-Review+2 (1 comment) Patchset: PS5: Please add a reported by line for https://github.com/OpenVPN/openvpn-private-issues/issues/81 (for the character case fix). May be done at commit time. This has to be merged along with 1307 to avoid re-introducing the "trailing slash" bug. -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 5 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-Reviewer: selvanair Gerrit-CC: openvpn-devel Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: d12fk Gerrit-Comment-Date: Fri, 07 Nov 2025 02:48:41 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 4: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/df0586da_23d5964d?usp=email : PS4, Line 78: res = PathCchCombine(config_path, sizeof(config_path), workdir, fname); > size should be in characters, not sizeof() isn't it? Same below in > PathCchCanonicalize. Good catch -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 4 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: selvanair Gerrit-Comment-Date: Fri, 07 Nov 2025 02:16:50 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: selvanair ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos.
Hello cron2, flichtenheld, plaisthos,
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 (#5).
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.
Also compare case insensitive, since Windows filesystems do not make a
difference here.
Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4
Signed-off-by: Heiko Hund
---
M src/openvpnserv/validate.c
1 file changed, 9 insertions(+), 22 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/07/1307/5
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index ddaa381..e1f0b66 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -24,8 +24,8 @@
#include
#include
-#include
#include
+#include
#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 && wcsnicmp(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: 5
Gerrit-Owner: d12fk
Gerrit-Reviewer: cron2
Gerrit-Reviewer: flichtenheld
Gerrit-Reviewer: plaisthos
Gerrit-CC: openvpn-devel
Gerrit-CC: selvanair
Gerrit-Attention: plaisthos
Gerrit-Attention: cron2
Gerrit-Attention: flichtenheld
Gerrit-Attention: d12fk
___
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos. selvanair has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 4: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/4c526f28_d6c55351?usp=email : PS3, Line 85: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > Done For got to mention that this was reported by: https://github.com/OpenVPN/openvpn-private-issues/issues/81 -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 4 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: d12fk Gerrit-Comment-Date: Thu, 06 Nov 2025 22:12:58 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk Comment-In-Reply-To: selvanair ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos. selvanair has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 4: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/121294f6_264c940d?usp=email : PS4, Line 78: res = PathCchCombine(config_path, sizeof(config_path), workdir, fname); size should be in characters, not sizeof() isn't it? Same below in PathCchCanonicalize. -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 4 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: d12fk Gerrit-Comment-Date: Thu, 06 Nov 2025 22:04:44 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 3: (2 comments) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/14df14a0_d3e23416?usp=email : PS3, Line 88: HRESULT res = PathCchCanonicalizeEx(config_dir, _countof(config_dir), s->config_dir, > As you are effectively reverting commit 05a8ba808, also remove > "HAVE_PATHCCH_ENSURE_TRAILING_SLASH" […] Done in #1354 http://gerrit.openvpn.net/c/openvpn/+/1307/comment/2e5dfd92_b85abcb8?usp=email : PS3, Line 85: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > While at it, make this case insensitve. Done -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 3 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: selvanair Gerrit-Comment-Date: Thu, 06 Nov 2025 21:00:07 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: selvanair ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos.
Hello cron2, flichtenheld, plaisthos,
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 (#4).
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.
Also compare case insensitive, since Windows filesystems do not make a
difference here.
Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4
Signed-off-by: Heiko Hund
---
M src/openvpnserv/validate.c
1 file changed, 9 insertions(+), 22 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/07/1307/4
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index ddaa381..4eb3141 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -24,8 +24,8 @@
#include
#include
-#include
#include
+#include
#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, sizeof(config_path), workdir, fname);
}
else
{
-config_file = fname;
+res = PathCchCanonicalize(config_path, sizeof(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 && wcsnicmp(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: 4
Gerrit-Owner: d12fk
Gerrit-Reviewer: cron2
Gerrit-Reviewer: flichtenheld
Gerrit-Reviewer: plaisthos
Gerrit-CC: openvpn-devel
Gerrit-CC: selvanair
Gerrit-Attention: plaisthos
Gerrit-Attention: cron2
Gerrit-Attention: flichtenheld
Gerrit-Attention: d12fk
___
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos. selvanair has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 3: (2 comments) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/68d02a1c_0271f54d?usp=email : PS3, Line 88: HRESULT res = PathCchCanonicalizeEx(config_dir, _countof(config_dir), s->config_dir, As you are effectively reverting commit 05a8ba808, also remove "HAVE_PATHCCH_ENSURE_TRAILING_SLASH" introduced by there and related changes in CMakeFiles.txt. Also the follow up commit d5e6b72460a. http://gerrit.openvpn.net/c/openvpn/+/1307/comment/1a4c657c_0c6a5d1e?usp=email : PS3, Line 85: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; While at it, make this case insensitve. -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 3 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: d12fk Gerrit-Comment-Date: Thu, 06 Nov 2025 20:30:43 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair.
Hello cron2, flichtenheld, plaisthos,
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 (#3).
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
---
M src/openvpnserv/validate.c
1 file changed, 9 insertions(+), 22 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/07/1307/3
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index ddaa381..ea02c8e 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -24,8 +24,8 @@
#include
#include
-#include
#include
+#include
#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, sizeof(config_path), workdir, fname);
}
else
{
-config_file = fname;
+res = PathCchCanonicalize(config_path, sizeof(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: 3
Gerrit-Owner: d12fk
Gerrit-Reviewer: cron2
Gerrit-Reviewer: flichtenheld
Gerrit-Reviewer: plaisthos
Gerrit-CC: openvpn-devel
Gerrit-CC: selvanair
Gerrit-Attention: plaisthos
Gerrit-Attention: cron2
Gerrit-Attention: flichtenheld
Gerrit-Attention: selvanair
___
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/394d39d7_9ef2361a?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > Okay, I think optionally adding a \ when settings_t. […] #1336 -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: selvanair Gerrit-Comment-Date: Fri, 31 Oct 2025 19:36:47 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk Comment-In-Reply-To: selvanair ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/af7d5233_8513dfb0?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > https://github. […] Okay, I think optionally adding a \ when settings_t.config_dir is set makes more sense, as it would apply to any other (future) use as well then. Will add a commit dealing with that. -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: selvanair Gerrit-Comment-Date: Fri, 31 Oct 2025 18:41:15 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk Comment-In-Reply-To: selvanair ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos. selvanair has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/3e51da9a_275be521?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > Which github issue # is this? https://github.com/OpenVPN/openvpn-private-issues/issues/22 -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: d12fk Gerrit-Comment-Date: Fri, 31 Oct 2025 18:27:07 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk Comment-In-Reply-To: selvanair ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/fe993544_9469c998?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > hmm. This misses the purpose of the previous patch -- i.e. […] Which github issue # is this? -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: selvanair Gerrit-Comment-Date: Fri, 31 Oct 2025 18:19:57 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: selvanair ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos. selvanair has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/65edbdf0_59c409fa?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; hmm. This misses the purpose of the previous patch -- i.e., to ensure that config_dir has a trailing slash which was the source of the zeropath report. -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-CC: selvanair Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Attention: d12fk Gerrit-Comment-Date: Fri, 31 Oct 2025 18:11:32 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: d12fk, flichtenheld, plaisthos.
Hello cron2, flichtenheld, plaisthos,
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 (#2).
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
---
M src/openvpnserv/validate.c
1 file changed, 9 insertions(+), 22 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/07/1307/2
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index 2187fb5..80ccb67 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -24,8 +24,8 @@
#include
#include
-#include
#include
+#include
#ifndef HAVE_PATHCCH_ENSURE_TRAILING_SLASH
#define PATHCCH_ENSURE_TRAILING_SLASH 0x20
@@ -58,39 +58,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;
-WCHAR config_dir[MAX_PATH];
+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);
}
-/* 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: 2
Gerrit-Owner: d12fk
Gerrit-Reviewer: cron2
Gerrit-Reviewer: flichtenheld
Gerrit-Reviewer: plaisthos
Gerrit-CC: openvpn-devel
Gerrit-Attention: plaisthos
Gerrit-Attention: flichtenheld
Gerrit-Attention: d12fk
___
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: cron2, flichtenheld, plaisthos. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 1: (1 comment) Patchset: PS1: > This conflicted with Selva working on the same function, and I saw the other > one first :-) - so we h […] Done -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 1 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-Attention: plaisthos Gerrit-Attention: cron2 Gerrit-Attention: flichtenheld Gerrit-Comment-Date: Fri, 31 Oct 2025 17:37:39 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
Attention is currently required from: d12fk, flichtenheld, plaisthos. cron2 has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better .. Patch Set 1: Code-Review-2 (1 comment) Patchset: PS1: This conflicted with Selva working on the same function, and I saw the other one first :-) - so we have commit 05a8ba808 in tree which does the `PathCchCanonicalizeEx()` bit, but not the `PathCchCombine()` part... arguably your patch is nicer, but does not apply anymore. Can you do a new version that does the Combine thing based on master? -- 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: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 1 Gerrit-Owner: d12fk Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-CC: openvpn-devel Gerrit-Attention: plaisthos Gerrit-Attention: flichtenheld Gerrit-Attention: d12fk Gerrit-Comment-Date: Tue, 28 Oct 2025 19:27:03 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes ___ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better
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
---
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
#include
+#include
#include
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
Gerrit-Reviewer: flichtenheld
Gerrit-Reviewer: plaisthos
Gerrit-CC: openvpn-devel
Gerrit-Attention: plaisthos
Gerrit-Attention: flichtenheld
___
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
