Ensure strings read from registry are null-terminated

From: Selva Nair <[email protected]...>

- Strings stored in registry are not guaranteed to be null-terminated. So, use RegGetValue() instead of RegQueryValueEx() as the former adds null termination to the returned string if missing. (Needs Windows Vista+)

- While at it also add a default value parameter to GetRegString() to process optional registry values (such as ovpn_admin_group) without causing an otherwise confusing error logged to the eventlog[*].

[*] see Trac: #892

---
 src/openvpnserv/common.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

index e77d7ab..bc99584 100644
@@ -56,14 +56,18 @@ openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
 #define REG_KEY  TEXT("SOFTWARE\\" PACKAGE_NAME)
 
 static DWORD
-GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size)
+GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR default_value)
 {
-    DWORD type;
-    LONG status = RegQueryValueEx(key, value, NULL, &type, (LPBYTE) data, &size);
+    LONG status = RegGetValue(key, NULL, value, RRF_RT_REG_SZ|RRF_RT_REG_EXPAND_SZ,
+                              NULL, (LPBYTE) data, &size);
 
-    if (status == ERROR_SUCCESS && type != REG_SZ)
+    if (status == ERROR_FILE_NOT_FOUND && default_value)
     {
-        status = ERROR_DATATYPE_MISMATCH;
+        size_t len = size/sizeof(data[0]);
+        if (openvpn_sntprintf(data, len, default_value) > 0)
+        {
+            status = ERROR_SUCCESS;
+        }
     }
 
     if (status != ERROR_SUCCESS)

@@ -91,48 +95,48 @@ GetOpenvpnSettings(settings_t *s)
         return MsgToEventLog(M_SYSERR, TEXT("Could not open Registry key HKLM\\%s not found"), REG_KEY);
     }
 
-    error = GetRegString(key, TEXT("exe_path"), s->exe_path, sizeof(s->exe_path));
+    error = GetRegString(key, TEXT("exe_path"), s->exe_path, sizeof(s->exe_path), NULL);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("config_dir"), s->config_dir, sizeof(s->config_dir));
+    error = GetRegString(key, TEXT("config_dir"), s->config_dir, sizeof(s->config_dir), NULL);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("config_ext"), s->ext_string, sizeof(s->ext_string));
+    error = GetRegString(key, TEXT("config_ext"), s->ext_string, sizeof(s->ext_string), NULL);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir));
+    error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir), NULL);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("priority"), priority, sizeof(priority));
+    error = GetRegString(key, TEXT("priority"), priority, sizeof(priority), NULL);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("log_append"), append, sizeof(append));
+    error = GetRegString(key, TEXT("log_append"), append, sizeof(append), NULL);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
     /* read if present, else use default */
-    error = GetRegString(key, TEXT("ovpn_admin_group"), s->ovpn_admin_group, sizeof(s->ovpn_admin_group));
+    error = GetRegString(key, TEXT("ovpn_admin_group"), s->ovpn_admin_group,
+                         sizeof(s->ovpn_admin_group), OVPN_ADMIN_GROUP);
     if (error != ERROR_SUCCESS)
     {
-        openvpn_sntprintf(s->ovpn_admin_group, _countof(s->ovpn_admin_group), OVPN_ADMIN_GROUP);
-        error = 0; /* this error is not fatal */
+        goto out;
     }
     /* set process priority */
     if (!_tcsicmp(priority, TEXT("IDLE_PRIORITY_CLASS")))

2.1.4

From: Selva Nair <[email protected]...>

Not all installations need registry values such as log_dir and config_dir especially if automatic service is not in use. This patch provides reasonable defaults for registry values.

- Read the default value of HKLM\Software\PACKAGE_NAME to get the install path and construct defaults for exe_path, config_dir, log_dir from it. Use "ovpn", "0", NORMAL_PRIORITY as the defaults for config file extension, log-append flag and process priority.

The only remaining required registry entry is the root key (usually HKLM\Software\OpenVPN) whose default value should be set to the installation path.

---
 src/openvpnserv/common.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

index bc99584..759e6a0 100644
@@ -87,6 +87,8 @@ GetOpenvpnSettings(settings_t *s)
     TCHAR append[2];
     DWORD error;
     HKEY key;
+    TCHAR install_path[MAX_PATH];
+    TCHAR default_value[MAX_PATH];
 
     LONG status = RegOpenKeyEx(HKEY_LOCAL_MACHINE, REG_KEY, 0, KEY_READ, &key);
     if (status != ERROR_SUCCESS)

@@ -95,37 +97,50 @@ GetOpenvpnSettings(settings_t *s)
         return MsgToEventLog(M_SYSERR, TEXT("Could not open Registry key HKLM\\%s not found"), REG_KEY);
     }
 
-    error = GetRegString(key, TEXT("exe_path"), s->exe_path, sizeof(s->exe_path), NULL);
+    /* The default value of REG_KEY is the install path */
+    if (GetRegString(key, NULL, install_path, sizeof(install_path), NULL) != ERROR_SUCCESS)
+    {
+        goto out;
+    }
+
+    openvpn_sntprintf(default_value, _countof(default_value), TEXT("%s\\bin\\openvpn.exe"),
+                      install_path);
+    error = GetRegString(key, TEXT("exe_path"), s->exe_path, sizeof(s->exe_path), default_value);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("config_dir"), s->config_dir, sizeof(s->config_dir), NULL);
+    openvpn_sntprintf(default_value, _countof(default_value), TEXT("%s\\config"), install_path);
+    error = GetRegString(key, TEXT("config_dir"), s->config_dir, sizeof(s->config_dir),
+                         default_value);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("config_ext"), s->ext_string, sizeof(s->ext_string), NULL);
+    error = GetRegString(key, TEXT("config_ext"), s->ext_string, sizeof(s->ext_string),
+                         TEXT(".ovpn"));
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir), NULL);
+    openvpn_sntprintf(default_value, _countof(default_value), TEXT("%s\\log"), install_path);
+    error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir), default_value);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("priority"), priority, sizeof(priority), NULL);
+    error = GetRegString(key, TEXT("priority"), priority, sizeof(priority),
+                         TEXT("NORMAL_PRIORITY_CLASS"));
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("log_append"), append, sizeof(append), NULL);
+    error = GetRegString(key, TEXT("log_append"), append, sizeof(append), TEXT("0"));
     if (error != ERROR_SUCCESS)
     {
         goto out;

2.1.4

"Because it makes sense". Stared at the code, compile-tested on 16.04.

Same issue as in 1/2 with the "reg_path" change... but again, only conflicts in context, not actual code, and context that is not affected by the code changes.

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

commit db04bca6729e9fe1ea60f0b3bd0329244a6ed611 (master) commit c07bdace820753ab29def5edc3a6cadbf233cd03 (release/2.4)

Author: Selva Nair
Date:   Sat Nov 18 12:40:58 2017 -0500

     Make most registry values optional

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg15892.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

well, today is the day that I get to break things, it seems...

	openvpnserv error: Falscher Parameter. (0x57)
	Error querying registry value: HKLM\SOFTWARE\OpenVPN\(null)

now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...

Hi,

Hmm.. I thought I had tested the patch.. Looking at it right now -- hold on :)

Selva

Hi,

Missed you on IRC...

18:21 < selvanair> cron2: iservives in 2.4 local build and snapshot build works
on Windows 10. Have to go now, but will test Win7 later today when I can get hold of a one.
18:22 < selvanair> s/iservives/iservice

... seems this is different on Win7...?

To be sure it's not something messed up on my system, I have checked the HKLM\Software\OpenVPN\(null) path that it's trying to access - and all is there

(standard)  REG_SZ  c:\Program Files\OpenVPN
config_dir  REG_SZ  c:\Program Files\OpenVPN\config

...

and according to the documentation in

https://msdn.microsoft.com/en-us/library/windows/desktop/ms724868(v=vs.85).aspx

passing "lpValue = NULL" really should work... the error code 0x57 OTOH is documented as

	ERROR_INVALID_PARAMETER
	87 (0x57)
	The parameter is incorrect.

... which isn't exactly helpful to see *which* argument it does not like
- everything *should* be according to specification...

(It does happen for my own binary and for the installers Samuli builds, so it's not "my build environment" - theoretically it could be part 1/2 of that patch set, which I haven't run yet)

gert

now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...

Hi,

Yeah, I could reproduce it on Windows 7. And, at last, after several painful Windows builds, it turns out:

RegGetValue with RRF_RT_REG_EXPAND_SZ|RRF_RT_REG_SZ is broken on Windows 7 (and most likely Vista too).

But once you know what to look for, just google:

https://stackoverflow.com/questions/47096940/reggetvalue-for-value-that-may-be-either-reg-sz-or-reg-expand-sz

Will send a patch later.

Selva

Hi,

Thanks for investigating and finding the root cause. "This is not what the documentation says...!" -> I need to actually *run* these things, not just test compile.

Thanks!

PS: when you say "several painful Windows builds" - I found that things are actually working quite nicely using "build-snapshot --use-depcache", pointing this to a private git repo which has the code I want to test. Still a bit annoying because the machinery insists on "fetch all, clean all, rebuild all" instead of "there's the source directory, just run make!"... shall we try to spend a bit time on this, to make interactively working on source changes nicer?

gert

now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...

Hi,

When I say painful builds, I mean edit build[*], copy to Windows, run in a few configurations, look through the event viewer, rinse rather repeat. And finally when it seems to start working connect to a my work VPN, remove the registry keys and try to break it etc..

FYI, I don't just "test compile", and leave it at that.

Selva

[*] Yes, that fetch-all and rebuild from scratch part of openvpn-build is very time consuming. Making it work the usual "remake only what changed" way would save a lot of time and frustration.

Hi,

On re-reading, it sounds different from what I wanted to say. I was just cursing stupid Windows that's no fun to debug on... In this particular case it was my fault that I never tested the patch on Windows 7 in the first place. I used to run a Win7 VM, but now all my desktop memory is eaten by those 50 chrome tabs..

Anyway, sorry to all (and to myself) for causing a "must-fix" bug close to a release.

Selva

Hi,

Indeed, that takes quite a bit of time.

I did not want to imply that you did - sorry if it came across that way.

*I* only test compiled, and was then happy merging your patch ("I'm sure Selva tested this, it is according to documentation and the code looks good, and compiles fine") - but as I learned, there are more caveats on Windows. So, when we do more basic changes (new API, etc), I need to do actual test installations on my windows zoo, like, win7/32, win7/64, win10/64... I usually do this already today if I suspect breakage, but in this case, it came as a surprise.

gert

now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...

"Because it makes sense" (checked with the Windows API documentation, and compile-tested on ubuntu 16.04).

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

There was one issue with your patch that you might want to look into - it seems to be based on a private branch that has a "#define REG_KEY" which does not exist in my tree, and which I could not find any references to on the mailing list either. It broke applying the patch as it appeared in patch context - but since none of the actual code line reference this, I decided to apply the patch nonetheless (it has been lingering for way too long already).

commit b1263b06db40f21a8fd20e0efd0c12e37ce89a2c (master) commit b00dab5422669fca7304f4af111b7c259b8a0a55 (release/2.4)

Author: Selva Nair
Date:   Sat Nov 18 12:40:57 2017 -0500

     Ensure strings read from registry are null-terminated

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg15893.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

That line got removed by the following commit (multiple service instances):

commit f3fec49b1c916a701058ef2445b4c07005c30673
Author: Simon Rozman <[email protected]...>
Date: Sun Dec 3 22:16:54 2017 +0100
openvpnserv: Add support for multi-instances

So it was a rebase conflict. I scanned through the patch as applied and all look ok.

Thanks,

Selva

Hi,

[..]

Ah. I thought it looked like Simon, but searched in the wrong place :-)

Thanks for double-checking.

gert

now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...