[Openvpn-devel,v2] Load OpenSSL config on Windows from trusted location

Message ID 20211118212945.478-1-lstipakov@gmail.com
State Superseded
Headers show
Series
  • [Openvpn-devel,v2] Load OpenSSL config on Windows from trusted location
Related show

Commit Message

Lev Stipakov Nov. 18, 2021, 9:29 p.m.
From: Lev Stipakov <lev@openvpn.net>

Commit 7e33127d5 ("contrib/vcpkg-ports: remove openssl port")
disabled OpenSSL config loading to prevent loading config
from untrusted locations.

Config loading feature might be useful for some users. This
brings it back, and sets OpenSSL enviroment variables

 OPENSSL_CONF, OPENSSL_ENGINES, OPENSSL_MODULES

which are used to load config, engines and modules, to a trusted location.
The location is built based on installation path, read from registry on startup.
If installation path cannot be read, Windows\System32 is used as a fallback.

While on it, remove unused "bool impersonate_as_system();" declaration.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 v2: add missing "static" modifier to set_openssl_env_vars()
declaration noted by gcc

 .../vcpkg-triplets/arm64-windows-ovpn.cmake   |  2 -
 contrib/vcpkg-triplets/x64-windows-ovpn.cmake |  2 -
 contrib/vcpkg-triplets/x86-windows-ovpn.cmake |  2 -
 src/openvpn/buffer.c                          | 23 ------
 src/openvpn/win32.c                           | 76 +++++++++++++++++++
 src/openvpn/win32.h                           |  8 +-
 6 files changed, 83 insertions(+), 30 deletions(-)

Comments

Selva Nair Nov. 23, 2021, 4:38 p.m. | #1
Hi,

+1 for setting these env vars. I will test this but some quick comments

On Tue, Nov 23, 2021 at 10:08 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> From: Lev Stipakov <lev@openvpn.net>
>
> Commit 7e33127d5 ("contrib/vcpkg-ports: remove openssl port")
> disabled OpenSSL config loading to prevent loading config
> from untrusted locations.
>
> Config loading feature might be useful for some users. This
> brings it back, and sets OpenSSL enviroment variables
>
>  OPENSSL_CONF, OPENSSL_ENGINES, OPENSSL_MODULES
>
> which are used to load config, engines and modules, to a trusted location.
> The location is built based on installation path, read from registry on
> startup.
> If installation path cannot be read, Windows\System32 is used as a
> fallback.
>
> While on it, remove unused "bool impersonate_as_system();" declaration.
>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  v2: add missing "static" modifier to set_openssl_env_vars()
> declaration noted by gcc
>
>  .../vcpkg-triplets/arm64-windows-ovpn.cmake   |  2 -
>  contrib/vcpkg-triplets/x64-windows-ovpn.cmake |  2 -
>  contrib/vcpkg-triplets/x86-windows-ovpn.cmake |  2 -
>  src/openvpn/buffer.c                          | 23 ------
>  src/openvpn/win32.c                           | 76 +++++++++++++++++++
>  src/openvpn/win32.h                           |  8 +-
>  6 files changed, 83 insertions(+), 30 deletions(-)
>
> diff --git a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake
> b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake
> index 89f6a279..dd3c6c0a 100644
> --- a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake
> +++ b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake
> @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic)
>  if(PORT STREQUAL "lz4")
>      set(VCPKG_LIBRARY_LINKAGE static)
>  endif()
> -
> -set(OPENSSL_NO_AUTOLOAD_CONFIG ON)
> diff --git a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake
> b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake
> index d860eed6..7036ed2d 100644
> --- a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake
> +++ b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake
> @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic)
>  if(PORT STREQUAL "lz4")
>      set(VCPKG_LIBRARY_LINKAGE static)
>  endif()
> -
> -set(OPENSSL_NO_AUTOLOAD_CONFIG ON)
> diff --git a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake
> b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake
> index c1ea6ef3..7d3bf340 100644
> --- a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake
> +++ b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake
> @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic)
>  if(PORT STREQUAL "lz4")
>      set(VCPKG_LIBRARY_LINKAGE static)
>  endif()
> -
> -set(OPENSSL_NO_AUTOLOAD_CONFIG ON)
> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
> index c82d3d4d..54e758af 100644
> --- a/src/openvpn/buffer.c
> +++ b/src/openvpn/buffer.c
> @@ -310,29 +310,6 @@ openvpn_snprintf(char *str, size_t size, const char
> *format, ...)
>      return (len >= 0 && len < size);
>  }
>
> -/*
> - * openvpn_swprintf() is currently only used by Windows code paths
> - * and when enabled for all platforms it will currently break older
> - * OpenBSD versions lacking vswprintf(3) support in their libc.
> - */
> -
> -#ifdef _WIN32
> -bool
> -openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t
> *const format, ...)
> -{
> -    va_list arglist;
> -    int len = -1;
> -    if (size > 0)
> -    {
> -        va_start(arglist, format);
> -        len = vswprintf(str, size, format, arglist);
> -        va_end(arglist);
> -        str[size - 1] = L'\0';
> -    }
> -    return (len >= 0 && len < size);
> -}
> -#endif
> -
>  /*
>   * write a string to the end of a buffer that was
>   * truncated by buf_printf
> diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
> index 6cff17b2..ebc7c833 100644
> --- a/src/openvpn/win32.c
> +++ b/src/openvpn/win32.c
> @@ -101,6 +101,12 @@ struct semaphore netcmd_semaphore; /* GLOBAL */
>   */
>  static char *win_sys_path = NULL; /* GLOBAL */
>
> +/**
> + * Set OpenSSL environment variables to a safe directory
> + */
> +static void
> +set_openssl_env_vars();
> +
>  void
>  init_win32(void)
>  {
> @@ -110,6 +116,8 @@ init_win32(void)
>      }
>      window_title_clear(&window_title);
>      win32_signal_clear(&win32_signal);
> +
> +    set_openssl_env_vars();
>  }
>
>  void
> @@ -1509,4 +1517,72 @@ send_msg_iservice(HANDLE pipe, const void *data,
> size_t size,
>      return ret;
>  }
>
> +bool
> +openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t*
> const format, ...)
> +{
> +    va_list arglist;
> +    int len = -1;
> +    if (size > 0)
> +    {
> +        va_start(arglist, format);
> +        len = vswprintf(str, size, format, arglist);
> +        va_end(arglist);
> +        str[size - 1] = L'\0';
> +    }
> +    return (len >= 0 && len < size);
> +}
> +
> +static BOOL
> +get_install_path(WCHAR *path, DWORD size)
> +{
> +    WCHAR reg_path[256];
> +    HKEY key;
> +    BOOL res = FALSE;
> +    openvpn_swprintf(reg_path, _countof(reg_path), L"SOFTWARE\\"
> PACKAGE_NAME);
>

Does this string concatenation like L"foo" "bar" work correctly on MSVC? I
know it works on mingw, but in the past with the GUI resources we had run
into issues with such usage -- iirc, MSVC wanted L"foo" L"bar".


> +
> +    LONG status = RegOpenKeyExW(HKEY_LOCAL_MACHINE, reg_path, 0,
> KEY_READ, &key);
> +    if (status != ERROR_SUCCESS)
> +    {
> +        return res;
> +    }
> +
> +    /* The default value of REG_KEY is the install path */
> +    status = RegGetValueW(key, NULL, NULL, RRF_RT_REG_SZ, NULL,
> (LPBYTE)path, &size);
> +    res = status == ERROR_SUCCESS;
> +
> +    RegCloseKey(key);
> +
> +    return res;
> +}
> +
> +static void
> +set_openssl_env_vars()
> +{
> +    const WCHAR* ssl_fallback_dir = L"C:\\Windows\\System32\\";
> +
> +    WCHAR install_path[MAX_PATH] = { 0 };
> +    if (!get_install_path(install_path, _countof(install_path)))
> +    {
> +        /* if we cannot find installation path from the registry,
> +         * use Windows directory as a fallback
> +         */
> +        openvpn_swprintf(install_path, _countof(install_path), L"%ls",
> ssl_fallback_dir);
> +    }
>

Is this registry value guaranteed to end with the directory separator "\\"?
Should we check it? If so, it may be easier to strip it here and add in the
format with no ending "\\" in the default above.


> +
> +    WCHAR openssl_cnf[MAX_PATH] = {0};
> +    WCHAR openssl_engines[MAX_PATH] = {0};
> +    WCHAR openssl_modules[MAX_PATH] = {0};
>

On linux one needs strings on the heap or const strings for putenv() as it
does not make a copy unlike setenv (with some exceptions).  Does _wputenv
on Windows behave differently? Do local variables work?


> +
> +    openvpn_swprintf(openssl_cnf, _countof(install_path),
> +        L"OPENSSL_CONF=%lsssl\\openssl.cnf", install_path);
> +    openvpn_swprintf(openssl_engines, _countof(openssl_engines),
> +        L"OPENSSL_ENGINES=%lsssl\\engines", install_path);
> +    openvpn_swprintf(openssl_modules, _countof(openssl_modules),
> +        L"OPENSSL_MODULES=%lsssl\\modules", install_path);
> +
> +    _wputenv(openssl_cnf);
> +    _wputenv(openssl_engines);
> +    _wputenv(openssl_modules);
>

I think we should set these only if not already set. Otherwise the user has
no way of overriding these locations which is the original purpose of env
vars.


> +}
> +
>  #endif /* ifdef _WIN32 */
> diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
> index 5d3371a0..4a992d91 100644
> --- a/src/openvpn/win32.h
> +++ b/src/openvpn/win32.h
> @@ -327,7 +327,13 @@ bool send_msg_iservice(HANDLE pipe, const void *data,
> size_t size,
>  int
>  openvpn_execve(const struct argv *a, const struct env_set *es, const
> unsigned int flags);
>
> -bool impersonate_as_system();
> +/*
> + * openvpn_swprintf() is currently only used by Windows code paths
> + * and when enabled for all platforms it will currently break older
> + * OpenBSD versions lacking vswprintf(3) support in their libc.
> + */
> +bool
> +openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t*
> const format, ...);
>
>  #endif /* ifndef OPENVPN_WIN32_H */
>  #endif /* ifdef _WIN32 */
> --


Commit message says this brings back config loading but the config loading
in crypto_openssl.c stays disabled for Windows. An oversight?

Selva
<div dir="ltr"><div dir="ltr">Hi,</div><div><br></div>+1 for setting these env vars. I will test this but some quick comments<div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 23, 2021 at 10:08 AM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
<br>
Commit 7e33127d5 (&quot;contrib/vcpkg-ports: remove openssl port&quot;)<br>
disabled OpenSSL config loading to prevent loading config<br>
from untrusted locations.<br>
<br>
Config loading feature might be useful for some users. This<br>
brings it back, and sets OpenSSL enviroment variables<br>
<br>
 OPENSSL_CONF, OPENSSL_ENGINES, OPENSSL_MODULES<br>
<br>
which are used to load config, engines and modules, to a trusted location.<br>
The location is built based on installation path, read from registry on startup.<br>
If installation path cannot be read, Windows\System32 is used as a fallback.<br>
<br>
While on it, remove unused &quot;bool impersonate_as_system();&quot; declaration.<br>
<br>
Signed-off-by: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
---<br>
 v2: add missing &quot;static&quot; modifier to set_openssl_env_vars()<br>
declaration noted by gcc<br>
<br>
 .../vcpkg-triplets/arm64-windows-ovpn.cmake   |  2 -<br>
 contrib/vcpkg-triplets/x64-windows-ovpn.cmake |  2 -<br>
 contrib/vcpkg-triplets/x86-windows-ovpn.cmake |  2 -<br>
 src/openvpn/buffer.c                          | 23 ------<br>
 src/openvpn/win32.c                           | 76 +++++++++++++++++++<br>
 src/openvpn/win32.h                           |  8 +-<br>
 6 files changed, 83 insertions(+), 30 deletions(-)<br>
<br>
diff --git a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake<br>
index 89f6a279..dd3c6c0a 100644<br>
--- a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake<br>
+++ b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake<br>
@@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic)<br>
 if(PORT STREQUAL &quot;lz4&quot;)<br>
     set(VCPKG_LIBRARY_LINKAGE static)<br>
 endif()<br>
-<br>
-set(OPENSSL_NO_AUTOLOAD_CONFIG ON)<br>
diff --git a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake<br>
index d860eed6..7036ed2d 100644<br>
--- a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake<br>
+++ b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake<br>
@@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic)<br>
 if(PORT STREQUAL &quot;lz4&quot;)<br>
     set(VCPKG_LIBRARY_LINKAGE static)<br>
 endif()<br>
-<br>
-set(OPENSSL_NO_AUTOLOAD_CONFIG ON)<br>
diff --git a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake<br>
index c1ea6ef3..7d3bf340 100644<br>
--- a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake<br>
+++ b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake<br>
@@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic)<br>
 if(PORT STREQUAL &quot;lz4&quot;)<br>
     set(VCPKG_LIBRARY_LINKAGE static)<br>
 endif()<br>
-<br>
-set(OPENSSL_NO_AUTOLOAD_CONFIG ON)<br>
diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c<br>
index c82d3d4d..54e758af 100644<br>
--- a/src/openvpn/buffer.c<br>
+++ b/src/openvpn/buffer.c<br>
@@ -310,29 +310,6 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...)<br>
     return (len &gt;= 0 &amp;&amp; len &lt; size);<br>
 }<br>
<br>
-/*<br>
- * openvpn_swprintf() is currently only used by Windows code paths<br>
- * and when enabled for all platforms it will currently break older<br>
- * OpenBSD versions lacking vswprintf(3) support in their libc.<br>
- */<br>
-<br>
-#ifdef _WIN32<br>
-bool<br>
-openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)<br>
-{<br>
-    va_list arglist;<br>
-    int len = -1;<br>
-    if (size &gt; 0)<br>
-    {<br>
-        va_start(arglist, format);<br>
-        len = vswprintf(str, size, format, arglist);<br>
-        va_end(arglist);<br>
-        str[size - 1] = L&#39;\0&#39;;<br>
-    }<br>
-    return (len &gt;= 0 &amp;&amp; len &lt; size);<br>
-}<br>
-#endif<br>
-<br>
 /*<br>
  * write a string to the end of a buffer that was<br>
  * truncated by buf_printf<br>
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c<br>
index 6cff17b2..ebc7c833 100644<br>
--- a/src/openvpn/win32.c<br>
+++ b/src/openvpn/win32.c<br>
@@ -101,6 +101,12 @@ struct semaphore netcmd_semaphore; /* GLOBAL */<br>
  */<br>
 static char *win_sys_path = NULL; /* GLOBAL */<br>
<br>
+/**<br>
+ * Set OpenSSL environment variables to a safe directory<br>
+ */<br>
+static void<br>
+set_openssl_env_vars();<br>
+<br>
 void<br>
 init_win32(void)<br>
 {<br>
@@ -110,6 +116,8 @@ init_win32(void)<br>
     }<br>
     window_title_clear(&amp;window_title);<br>
     win32_signal_clear(&amp;win32_signal);<br>
+<br>
+    set_openssl_env_vars();<br>
 }<br>
<br>
 void<br>
@@ -1509,4 +1517,72 @@ send_msg_iservice(HANDLE pipe, const void *data, size_t size,<br>
     return ret;<br>
 }<br>
<br>
+bool<br>
+openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* const format, ...)<br>
+{<br>
+    va_list arglist;<br>
+    int len = -1;<br>
+    if (size &gt; 0)<br>
+    {<br>
+        va_start(arglist, format);<br>
+        len = vswprintf(str, size, format, arglist);<br>
+        va_end(arglist);<br>
+        str[size - 1] = L&#39;\0&#39;;<br>
+    }<br>
+    return (len &gt;= 0 &amp;&amp; len &lt; size);<br>
+}<br>
+<br>
+static BOOL<br>
+get_install_path(WCHAR *path, DWORD size)<br>
+{<br>
+    WCHAR reg_path[256];<br>
+    HKEY key;<br>
+    BOOL res = FALSE;<br>
+    openvpn_swprintf(reg_path, _countof(reg_path), L&quot;SOFTWARE\\&quot; PACKAGE_NAME);<br></blockquote><div><br></div><div>Does this string concatenation like L&quot;foo&quot; &quot;bar&quot; work correctly on MSVC? I know it works on mingw, but in the past with the GUI resources we had run into issues with such usage -- iirc, MSVC wanted L&quot;foo&quot; L&quot;bar&quot;.</div><div>  </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+    LONG status = RegOpenKeyExW(HKEY_LOCAL_MACHINE, reg_path, 0, KEY_READ, &amp;key);<br>
+    if (status != ERROR_SUCCESS)<br>
+    {<br>
+        return res;<br>
+    }<br>
+<br>
+    /* The default value of REG_KEY is the install path */<br>
+    status = RegGetValueW(key, NULL, NULL, RRF_RT_REG_SZ, NULL, (LPBYTE)path, &amp;size);<br>
+    res = status == ERROR_SUCCESS;<br>
+<br>
+    RegCloseKey(key);<br>
+<br>
+    return res;<br>
+}<br>
+<br>
+static void<br>
+set_openssl_env_vars()<br>
+{<br>
+    const WCHAR* ssl_fallback_dir = L&quot;C:\\Windows\\System32\\&quot;;<br>
+<br>
+    WCHAR install_path[MAX_PATH] = { 0 };<br>
+    if (!get_install_path(install_path, _countof(install_path)))<br>
+    {<br>
+        /* if we cannot find installation path from the registry,<br>
+         * use Windows directory as a fallback<br>
+         */<br>
+        openvpn_swprintf(install_path, _countof(install_path), L&quot;%ls&quot;, ssl_fallback_dir);<br>
+    }<br>
</blockquote><div><br></div><div>Is this registry value guaranteed to end with the directory separator &quot;\\&quot;? Should we check it? If so, it may be easier to strip it here and add in the format with no ending &quot;\\&quot; in the default above.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+<br>
+    WCHAR openssl_cnf[MAX_PATH] = {0};<br>
+    WCHAR openssl_engines[MAX_PATH] = {0};<br>
+    WCHAR openssl_modules[MAX_PATH] = {0};<br></blockquote><div><br></div><div>On linux one needs strings on the heap or const strings for putenv() as it does not make a copy unlike setenv (with some exceptions).  Does _wputenv on Windows behave differently? Do local variables work?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+    openvpn_swprintf(openssl_cnf, _countof(install_path),<br>
+        L&quot;OPENSSL_CONF=%lsssl\\openssl.cnf&quot;, install_path);<br>
+    openvpn_swprintf(openssl_engines, _countof(openssl_engines),<br>
+        L&quot;OPENSSL_ENGINES=%lsssl\\engines&quot;, install_path);<br>
+    openvpn_swprintf(openssl_modules, _countof(openssl_modules),<br>
+        L&quot;OPENSSL_MODULES=%lsssl\\modules&quot;, install_path);<br>
+<br>
+    _wputenv(openssl_cnf);<br>
+    _wputenv(openssl_engines);<br>
+    _wputenv(openssl_modules);<br></blockquote><div><br></div><div>I think we should set these only if not already set. Otherwise the user has no way of overriding these locations which is the original purpose of env vars. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+}<br>
+<br>
 #endif /* ifdef _WIN32 */<br>
diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h<br>
index 5d3371a0..4a992d91 100644<br>
--- a/src/openvpn/win32.h<br>
+++ b/src/openvpn/win32.h<br>
@@ -327,7 +327,13 @@ bool send_msg_iservice(HANDLE pipe, const void *data, size_t size,<br>
 int<br>
 openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned int flags);<br>
<br>
-bool impersonate_as_system();<br>
+/*<br>
+ * openvpn_swprintf() is currently only used by Windows code paths<br>
+ * and when enabled for all platforms it will currently break older<br>
+ * OpenBSD versions lacking vswprintf(3) support in their libc.<br>
+ */<br>
+bool<br>
+openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* const format, ...);<br>
<br>
 #endif /* ifndef OPENVPN_WIN32_H */<br>
 #endif /* ifdef _WIN32 */<br>
--</blockquote><div><br></div><div>Commit message says this brings back config loading but the config loading in crypto_openssl.c stays disabled for Windows. An oversight?</div><div><br></div><div>Selva</div></div></div></div>
Lev Stipakov Nov. 23, 2021, 5:56 p.m. | #2
Hi,

> Does this string concatenation like L"foo" "bar" work correctly on MSVC? I know it works on mingw, but in the past with the GUI resources we had run into issues with such usage -- iirc, MSVC wanted L"foo" L"bar".

I think so, at least result of concatenation looks correct in procmon:

> openvpn.exe 23312 CreateFile C:\Program Files\OpenVPN\ssl\openssl.cnf PATH NOT FOUND

> Is this registry value guaranteed to end with the directory separator "\\"? Should we check it? If so, it may be easier to strip it here and add in the format with no ending "\\" in the default above.

Those are set by our MSI script. But let's be on the safe side.

> On linux one needs strings on the heap or const strings for putenv() as it does not make a copy unlike setenv (with some exceptions).  Does _wputenv on Windows behave differently? Do local variables work?

At least they work in my tests. This
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/putenv-wputenv?view=msvc-170
doesn't mention anything about automatic variables cannot be used,
contrary to "man putenv".

>> +    _wputenv(openssl_cnf);
>> +    _wputenv(openssl_engines);
>> +    _wputenv(openssl_modules);
>
>
> I think we should set these only if not already set. Otherwise the user has no way of overriding these locations which is the original purpose of env vars.

Will do.

> Commit message says this brings back config loading but the config loading in crypto_openssl.c stays disabled for Windows. An oversight?

Good catch. Interesting that it works for me even with that code in
crypto_openssl.c being disabled. It looks like our SSL context
initialization does the right thing:

> libcrypto-1_1-x64.dll!OPENSSL_init_crypto(unsigned __int64 opts, const ossl_init_settings_st * settings) Line 699 C
  libssl-1_1-x64.dll!OPENSSL_init_ssl(unsigned __int64 opts, const
ossl_init_settings_st * settings) Line 205 C
  libssl-1_1-x64.dll!SSL_CTX_new(const ssl_method_st * meth) Line 3023 C
  openvpn.exe!tls_ctx_client_new(tls_root_ctx * ctx) Line 134 C
  openvpn.exe!init_ssl(const options * options, tls_root_ctx *
new_ctx, bool in_chroot) Line 622 C
  openvpn.exe!do_init_crypto_tls_c1(context * c) Line 2741 C
  openvpn.exe!do_init_crypto_tls(context * c, const unsigned int
flags) Line 2821 C
  openvpn.exe!do_init_crypto(context * c, const unsigned int flags) Line 3084 C
  openvpn.exe!init_instance(context * c, const env_set * env, const
unsigned int flags) Line 4311 C
  openvpn.exe!init_instance_handle_signals(context * c, const env_set
* env, const unsigned int flags) Line 4124 C
  openvpn.exe!tunnel_point_to_point(context * c) Line 68 C

and opts got the correct bitflag:

#ifndef OPENSSL_NO_AUTOLOAD_CONFIG
    if ((opts & OPENSSL_INIT_NO_LOAD_CONFIG) == 0)
        opts |= OPENSSL_INIT_LOAD_CONFIG;
#endif

Does that code in crypto_openssl.c cover some unusual use case? Anyway
I can bring it back.

Patch

diff --git a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake
index 89f6a279..dd3c6c0a 100644
--- a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake
+++ b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake
@@ -5,5 +5,3 @@  set(VCPKG_LIBRARY_LINKAGE dynamic)
 if(PORT STREQUAL "lz4")
     set(VCPKG_LIBRARY_LINKAGE static)
 endif()
-
-set(OPENSSL_NO_AUTOLOAD_CONFIG ON)
diff --git a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake
index d860eed6..7036ed2d 100644
--- a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake
+++ b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake
@@ -5,5 +5,3 @@  set(VCPKG_LIBRARY_LINKAGE dynamic)
 if(PORT STREQUAL "lz4")
     set(VCPKG_LIBRARY_LINKAGE static)
 endif()
-
-set(OPENSSL_NO_AUTOLOAD_CONFIG ON)
diff --git a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake
index c1ea6ef3..7d3bf340 100644
--- a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake
+++ b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake
@@ -5,5 +5,3 @@  set(VCPKG_LIBRARY_LINKAGE dynamic)
 if(PORT STREQUAL "lz4")
     set(VCPKG_LIBRARY_LINKAGE static)
 endif()
-
-set(OPENSSL_NO_AUTOLOAD_CONFIG ON)
diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index c82d3d4d..54e758af 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -310,29 +310,6 @@  openvpn_snprintf(char *str, size_t size, const char *format, ...)
     return (len >= 0 && len < size);
 }
 
-/*
- * openvpn_swprintf() is currently only used by Windows code paths
- * and when enabled for all platforms it will currently break older
- * OpenBSD versions lacking vswprintf(3) support in their libc.
- */
-
-#ifdef _WIN32
-bool
-openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
-{
-    va_list arglist;
-    int len = -1;
-    if (size > 0)
-    {
-        va_start(arglist, format);
-        len = vswprintf(str, size, format, arglist);
-        va_end(arglist);
-        str[size - 1] = L'\0';
-    }
-    return (len >= 0 && len < size);
-}
-#endif
-
 /*
  * write a string to the end of a buffer that was
  * truncated by buf_printf
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 6cff17b2..ebc7c833 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -101,6 +101,12 @@  struct semaphore netcmd_semaphore; /* GLOBAL */
  */
 static char *win_sys_path = NULL; /* GLOBAL */
 
+/**
+ * Set OpenSSL environment variables to a safe directory
+ */
+static void
+set_openssl_env_vars();
+
 void
 init_win32(void)
 {
@@ -110,6 +116,8 @@  init_win32(void)
     }
     window_title_clear(&window_title);
     win32_signal_clear(&win32_signal);
+
+    set_openssl_env_vars();
 }
 
 void
@@ -1509,4 +1517,72 @@  send_msg_iservice(HANDLE pipe, const void *data, size_t size,
     return ret;
 }
 
+bool
+openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
+static BOOL
+get_install_path(WCHAR *path, DWORD size)
+{
+    WCHAR reg_path[256];
+    HKEY key;
+    BOOL res = FALSE;
+    openvpn_swprintf(reg_path, _countof(reg_path), L"SOFTWARE\\" PACKAGE_NAME);
+
+    LONG status = RegOpenKeyExW(HKEY_LOCAL_MACHINE, reg_path, 0, KEY_READ, &key);
+    if (status != ERROR_SUCCESS)
+    {
+        return res;
+    }
+
+    /* The default value of REG_KEY is the install path */
+    status = RegGetValueW(key, NULL, NULL, RRF_RT_REG_SZ, NULL, (LPBYTE)path, &size);
+    res = status == ERROR_SUCCESS;
+
+    RegCloseKey(key);
+
+    return res;
+}
+
+static void
+set_openssl_env_vars()
+{
+    const WCHAR* ssl_fallback_dir = L"C:\\Windows\\System32\\";
+
+    WCHAR install_path[MAX_PATH] = { 0 };
+    if (!get_install_path(install_path, _countof(install_path)))
+    {
+        /* if we cannot find installation path from the registry,
+         * use Windows directory as a fallback
+         */
+        openvpn_swprintf(install_path, _countof(install_path), L"%ls", ssl_fallback_dir);
+    }
+
+    WCHAR openssl_cnf[MAX_PATH] = {0};
+    WCHAR openssl_engines[MAX_PATH] = {0};
+    WCHAR openssl_modules[MAX_PATH] = {0};
+
+    openvpn_swprintf(openssl_cnf, _countof(install_path),
+        L"OPENSSL_CONF=%lsssl\\openssl.cnf", install_path);
+    openvpn_swprintf(openssl_engines, _countof(openssl_engines),
+        L"OPENSSL_ENGINES=%lsssl\\engines", install_path);
+    openvpn_swprintf(openssl_modules, _countof(openssl_modules),
+        L"OPENSSL_MODULES=%lsssl\\modules", install_path);
+
+    _wputenv(openssl_cnf);
+    _wputenv(openssl_engines);
+    _wputenv(openssl_modules);
+}
+
 #endif /* ifdef _WIN32 */
diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
index 5d3371a0..4a992d91 100644
--- a/src/openvpn/win32.h
+++ b/src/openvpn/win32.h
@@ -327,7 +327,13 @@  bool send_msg_iservice(HANDLE pipe, const void *data, size_t size,
 int
 openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned int flags);
 
-bool impersonate_as_system();
+/*
+ * openvpn_swprintf() is currently only used by Windows code paths
+ * and when enabled for all platforms it will currently break older
+ * OpenBSD versions lacking vswprintf(3) support in their libc.
+ */
+bool
+openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* const format, ...);
 
 #endif /* ifndef OPENVPN_WIN32_H */
 #endif /* ifdef _WIN32 */