Do not hard-code windows systemroot in env_block

PATCH In review Selva Nair 2 participants 7 messages Nov 13, 2015

lore

Share & tools

lore

Selva Nair

author

2015-11-13 02:41 UTC #0 lore raw

PATCH 1/1 Do not hard-code windows systemroot in env_block

FWIW, fixes trac #500

Signed-off-by Selva Nair <[email protected]...>

---
 src/openvpn/win32.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

src/openvpn/win32.c +6 -1

index 7c89a5a..d06b41f 100644
@@ -763,7 +763,12 @@ win_safe_filename (const char *fn)
 static char *
 env_block (const struct env_set *es)
 {
-  char * force_path = "PATH=C:\\Windows\\System32;C:\\WINDOWS;C:\\WINDOWS\\System32\\Wbem";
+  char force_path[256];
+  char *sysroot = get_win_sys_path();
+
+  if (!openvpn_snprintf(force_path, sizeof(force_path), "PATH=%s\\System32;%s;%s\\System32\\Wbem",
+                        sysroot, sysroot, sysroot))
+    msg(M_WARN, "env_block: default path truncated to %s", force_path);
 
   if (es)
     {

Signature · 1 line

2.6.2

Gert Doering

2015-11-13 07:38 UTC #1 lore raw

Hi,

Quoted 12 lines · Selva Nair

FWIW, fixes trac #500
Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/win32.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
src/openvpn/win32.c +0 -0
index 7c89a5a..d06b41f 100644

src/openvpn/win32.c win_safe_filename (const char *fn) · line 771

Quoted 10 lines

src/openvpn/win32.c +6 -1

@@ -763,7 +763,12 @@ win_safe_filename (const char *fn)
 static char *
 env_block (const struct env_set *es)
 {
-  char * force_path = "PATH=C:\\Windows\\System32;C:\\WINDOWS;C:\\WINDOWS\\System32\\Wbem";
+  char force_path[256];
+  char *sysroot = get_win_sys_path();
+
+  if (!openvpn_snprintf(force_path, sizeof(force_path), "PATH=%s\\System32;%s;%s\\System32\\Wbem",
+                        sysroot, sysroot, sysroot))
+    msg(M_WARN, "env_block: default path truncated to %s", force_path);

Cool :-)

This bug was nagging me since quite a while, but I did not know how to read the "correct" windows path - and didn't know about get_win_sys_path().

OTOH, I'm not sure if this is going to work - win_sys_path is (unless set from options.c) initialized via

  GetEnvironmentVariable (SYS_PATH_ENV_VAR_NAME, buf, sizeof(buf)
  set_win_sys_path (buf, es);

... and that is likely to contain more than "c:\windows", but "a number of path elements"... so it's not really "sysroot".

What am I missing?

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Selva Nair

author

2015-11-13 15:21 UTC #2 lore raw

Quoted 10 lines · Gert Doering

Hi,
Quoted 7 lines · Selva Nair
FWIW, fixes trac #500
Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/win32.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

...

Quoted 5 lines

Cool :-)
This bug was nagging me since quite a while, but I did not know how to read the "correct" windows path - and didn't know about get_win_sys_path().

Quoted 8 lines

OTOH, I'm not sure if this is going to work - win_sys_path is (unless set from options.c) initialized via
  GetEnvironmentVariable (SYS_PATH_ENV_VAR_NAME, buf, sizeof(buf)
  set_win_sys_path (buf, es);
... and that is likely to contain more than "c:\windows", but "a number of path elements"... so it's not really "sysroot".

SYS_PATH_ENV_VAR_NAME is "SystemRoot" so that will set win_sys_path to %SystemRoot% (aka "C:\\windows" for most people).

Quoted 2 lines

What am I missing?

However, if the user specifies --win-sys some_cruft, win_sys_path will get set to that "some_cruft" (in options.c). User is the king.

Having said that --the man-page description of win-sys is confusing and its value is set as win_sys_path without any sanity checks. If win_sys_path is not a single base directory name, many other things will break; for example in route.c we construct the command line for route.exe as

argv_printf (&argv, "%s%sc ADD %s MASK %s %s",

         get_win_sys_path(),
         WIN_ROUTE_PATH_SUFFIX, <---- defined as "\\system32\route.exe"
                 network,
                 netmask,
                 gateway);

Selva

Gert Doering

2015-11-13 15:27 UTC #3 lore raw

Hi,

Quoted 11 lines · Selva Nair

Quoted 8 lines
OTOH, I'm not sure if this is going to work - win_sys_path is (unless set from options.c) initialized via
  GetEnvironmentVariable (SYS_PATH_ENV_VAR_NAME, buf, sizeof(buf)
  set_win_sys_path (buf, es);
... and that is likely to contain more than "c:\windows", but "a number of path elements"... so it's not really "sysroot".
SYS_PATH_ENV_VAR_NAME is "SystemRoot" so that will set win_sys_path to %SystemRoot% (aka "C:\\windows" for most people).

AAAH! I assumed it is just "PATH". Yes, this should work perfectly fine then. (Obviously I did not test this, sorry)

Quoted 4 lines

Quoted 1 line
What am I missing?
However, if the user specifies --win-sys some_cruft, win_sys_path will get set to that "some_cruft" (in options.c). User is the king.

Yes. In this case, I do not worry at all - "you get what you ask for".

Quoted 6 lines

Having said that --the man-page description of win-sys is confusing and its value is set as win_sys_path without any sanity checks. If win_sys_path is not a single base directory name, many other things will break; for example in route.c we construct the command line for route.exe as

Quoted 6 lines

argv_printf (&argv, "%s%sc ADD %s MASK %s %s",

         get_win_sys_path(),
         WIN_ROUTE_PATH_SUFFIX, <---- defined as "\\system32\route.exe"
                 network,
                 netmask,
                 gateway);

Yeah. "Put garbage in, get interesting problems out".

I should have looked at this example myself - it makes fairly clear that I was looking at the wrong environment variable.

Thanks again. ACK, and merging ASAP.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Selva Nair

author

2015-11-13 15:34 UTC #4 lore raw

Hi,

Quoted 5 lines · Gert Doering

Quoted 3 lines
However, if the user specifies --win-sys some_cruft, win_sys_path will get set to that "some_cruft" (in options.c). User is the king.
Yes. In this case, I do not worry at all - "you get what you ask for".

In the current setup its perfectly fine. But I am adding it to my list of genies when the privilege separation aka iService can is opened...:)

Selva

Gert Doering

2015-11-13 15:38 UTC #5 lore raw

Hi,

Quoted 9 lines · Selva Nair

Quoted 4 lines · Gert Doering
Quoted 2 lines
However, if the user specifies --win-sys some_cruft, win_sys_path will get set to that "some_cruft" (in options.c). User is the king.
Yes. In this case, I do not worry at all - "you get what you ask for".
In the current setup its perfectly fine. But I am adding it to my list of genies when the privilege separation aka iService can is opened...:)

That *should* actually still be a non-issue, as openvpn.exe is run with the privileges of the requesting user (and the route handling is requested via service pipe from the iService). Unlike today, where openvpn.exe has to run with admin privileges.

But of course there will be surprises in the details.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Gert Doering

2015-11-13 15:37 UTC #6 lore raw

In reply to Selva Nair

ACK, and thanks again for cleaning up my sloppy code :-)

Your patch has been applied to the master and release/2.3 branch.

commit 7546cba4761b24f2195034dcd3407aecd43fd3be (master) commit 9f87007d8997d8127b9ea5fb0cb6b6cb9c88a4f3 (release/2.3)

Author: Selva Nair
Date:   Thu Nov 12 21:41:27 2015 -0500

     Do not hard-code windows systemroot in env_block

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10492
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering