Menu
▾
▴
openvpn-devel
|
From: d12fk (C. Review) <ge...@op...> - 2025-10-26 22:14:38
|
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...>
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-10-28 19:27:17
|
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 <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> 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...> Gerrit-Attention: d12fk <he...@op...> Gerrit-Comment-Date: Tue, 28 Oct 2025 19:27:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes |
|
From: d12fk (C. Review) <ge...@op...> - 2025-10-31 17:37:53
|
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 <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Fri, 31 Oct 2025 17:37:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> |
|
From: d12fk (C. Review) <ge...@op...> - 2025-10-31 18:03:29
|
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 <he...@is...>
---
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 <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,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 <he...@op...>
Gerrit-Reviewer: cron2 <ge...@gr...>
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...>
Gerrit-Attention: d12fk <he...@op...>
|
|
From: selvanair (C. Review) <ge...@op...> - 2025-10-31 18:11:42
|
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 <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: d12fk <he...@op...> Gerrit-Comment-Date: Fri, 31 Oct 2025 18:11:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No |
|
From: d12fk (C. Review) <ge...@op...> - 2025-10-31 18:20:07
|
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 <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: selvanair <sel...@gm...> Gerrit-Comment-Date: Fri, 31 Oct 2025 18:19:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: selvanair <sel...@gm...> |
|
From: selvanair (C. Review) <ge...@op...> - 2025-10-31 18:27:17
|
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 <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: d12fk <he...@op...> Gerrit-Comment-Date: Fri, 31 Oct 2025 18:27:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk <he...@op...> Comment-In-Reply-To: selvanair <sel...@gm...> |
|
From: d12fk (C. Review) <ge...@op...> - 2025-10-31 18:41:30
|
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 <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: selvanair <sel...@gm...> Gerrit-Comment-Date: Fri, 31 Oct 2025 18:41:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk <he...@op...> Comment-In-Reply-To: selvanair <sel...@gm...> |
|
From: d12fk (C. Review) <ge...@op...> - 2025-10-31 19:37:02
|
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 <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: selvanair <sel...@gm...> Gerrit-Comment-Date: Fri, 31 Oct 2025 19:36:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk <he...@op...> Comment-In-Reply-To: selvanair <sel...@gm...> |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X