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

From: Lev Stipakov <lev@openvpn.net>

Commits

 - 92535b6 ("contrib/vcpkg-ports: add openssl port with --no-autoload-config option set (CVE-2121-3606)")
 - 447cfb4 ("crypto_openssl.c: disable explicit initialization on Windows (CVE-2121-3606)")

disabled OpenSSL config loading functionality, which could be
exploited by loading config from untrusted locations.

This 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 constructed 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>
---
 v4:
  - make set_openssl_env_vars() code more succint
  - use security-enhanced _wputenv_s/_wgetenv_s

 v3:
  - do not assume that installation path ends with directory separator
  - set enviroment variables only if they're not already set
  - bring back explicit initialization on Windows (might be needed on
    some cases)
  - slightly revamp commit message

 v2:
  - add missing "static" modifier to set_openssl_env_vars() declaration
spotted 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/crypto_openssl.c                  |  2 -
 src/openvpn/win32.c                           | 88 +++++++++++++++++++
 src/openvpn/win32.h                           |  8 +-
 7 files changed, 95 insertions(+), 32 deletions(-)

Hi,

Looks good in my tests using the msvc artifacts from
https://github.com/lstipakov/openvpn/actions/runs/1496339867#artifacts.

Loads config from <install-root>\ssl\openssl.cnf and engines specified with
relative paths load from <install-root>\ssl\engines. So the env vars are
being seen by OpenSSL and being used as expected.

I wasted quite some time using the msvc-built executable with mingw
compiled OpenSSL libraries (I was being lazy copying only openvpn.exe). For
some reason OpenSSL doesn't detect the env vars in this case --- process
explorer shows its all set right, but the library doesn't load openssl.cnf.
Not worth exploring further, I guess.

Testing OPENSSL_MODULES path is left for later when a 3.0 build with MSVC
is available.

Some nits below which may be ignored.

On Tue, Nov 23, 2021 at 2:31 PM Lev Stipakov <lstipakov@gmail.com> wrote:

> From: Lev Stipakov <lev@openvpn.net>
>
> Commits
>
>  - 92535b6 ("contrib/vcpkg-ports: add openssl port with
> --no-autoload-config option set (CVE-2121-3606)")
>  - 447cfb4 ("crypto_openssl.c: disable explicit initialization on Windows
> (CVE-2121-3606)")
>
> disabled OpenSSL config loading functionality, which could be
> exploited by loading config from untrusted locations.
>
> This 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 constructed 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>
> ---
>  v4:
>   - make set_openssl_env_vars() code more succint
>   - use security-enhanced _wputenv_s/_wgetenv_s
>
>  v3:
>   - do not assume that installation path ends with directory separator
>   - set enviroment variables only if they're not already set
>   - bring back explicit initialization on Windows (might be needed on
>     some cases)
>   - slightly revamp commit message
>
>  v2:
>   - add missing "static" modifier to set_openssl_env_vars() declaration
> spotted 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/crypto_openssl.c                  |  2 -
>  src/openvpn/win32.c                           | 88 +++++++++++++++++++
>  src/openvpn/win32.h                           |  8 +-
>  7 files changed, 95 insertions(+), 32 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/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index c9dc9d0a..ef520928 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -154,13 +154,11 @@ crypto_init_lib_engine(const char *engine_name)
>  void
>  crypto_init_lib(void)
>  {
> -#ifndef _WIN32
>  #if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
>      OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL);
>  #else
>      OPENSSL_config(NULL);
>  #endif
> -#endif /* _WIN32 */
>      /*
>       * If you build the OpenSSL library and OpenVPN with
>       * CRYPTO_MDEBUG, you will get a listing of OpenSSL
> diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
> index 6cff17b2..ee4d3f66 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,84 @@ 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);
> +    }
> +
> +    if ((install_path[wcslen(install_path) - 1]) == L'\\')
> +    {
> +        install_path[wcslen(install_path) - 1] = L'\0';
> +    }
> +
> +    static struct {
> +        WCHAR* name;
> +        WCHAR* value;
> +    } ossl_env[] = {
> +        {L"OPENSSL_CONF", L"openssl.cnf"},
> +        {L"OPENSSL_ENGINES", L"engines"},
> +        {L"OPENSSL_MODULES", L"modules"}
> +    };
>

As you defined this inside the function, it doesn't have to be static.
Also, name and value could be const. Our preferred style is "TYPE *val",
not the "TYPE* val" used here and elsewhere in the patch. Neither are worth
a v5, IMO.


> +
> +    for (size_t i = 0; i < SIZE(ossl_env); ++i)
> +    {
> +        size_t size = 0;
> +
> +        _wgetenv_s(&size, NULL, 0, ossl_env[i].name);
> +        if (size == 0)
> +        {
> +            WCHAR val[MAX_PATH] = {0};
> +            openvpn_swprintf(val, _countof(val), L"%ls\\ssl\\%ls",
> install_path, ossl_env[i].value);
> +            _wputenv_s(ossl_env[i].name, val);
> +        }
> +    }
> +}
> +
>  #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, ...);


Now there is a duplicate declaration in buffer.h which could be removed.
Multiple declarations is not an error, so let it be...


>  #endif /* ifndef OPENVPN_WIN32_H */
>  #endif /* ifdef _WIN32 */
>

Acked-by: Selva Nair <selva.nair@gmail.com>
<div dir="ltr"><div dir="ltr">Hi,<div><br></div><div>Looks good in my tests using the msvc artifacts from  <a href="https://github.com/lstipakov/openvpn/actions/runs/1496339867#artifacts" target="_blank">https://github.com/lstipakov/openvpn/actions/runs/1496339867#artifacts</a>. </div><div><br></div><div>Loads config from &lt;install-root&gt;\ssl\openssl.cnf and engines specified with relative paths load from &lt;install-root&gt;\ssl\engines. So the env vars are being seen by OpenSSL and being used as expected.</div><div><br></div><div>I wasted quite some time using the msvc-built executable with mingw compiled OpenSSL libraries (I was being lazy copying only openvpn.exe). For some reason OpenSSL doesn&#39;t detect the env vars in this case --- process explorer shows its all set right, but the library doesn&#39;t load openssl.cnf. Not worth exploring further, I guess.</div><div>  </div></div><div>Testing OPENSSL_MODULES path is left for later when a 3.0 build with MSVC is available.</div><div><br></div><div>Some nits below which may be ignored.<br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr"><br></div><div dir="ltr" class="gmail_attr">On Tue, Nov 23, 2021 at 2:31 PM 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>
Commits<br>
<br>
 - 92535b6 (&quot;contrib/vcpkg-ports: add openssl port with --no-autoload-config option set (CVE-2121-3606)&quot;)<br>
 - 447cfb4 (&quot;crypto_openssl.c: disable explicit initialization on Windows (CVE-2121-3606)&quot;)<br>
<br>
disabled OpenSSL config loading functionality, which could be<br>
exploited by loading config from untrusted locations.<br>
<br>
This feature might be useful for some users. This brings it back<br>
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 constructed 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>
 v4:<br>
  - make set_openssl_env_vars() code more succint<br>
  - use security-enhanced _wputenv_s/_wgetenv_s<br>
<br>
 v3:<br>
  - do not assume that installation path ends with directory separator<br>
  - set enviroment variables only if they&#39;re not already set<br>
  - bring back explicit initialization on Windows (might be needed on<br>
    some cases)<br>
  - slightly revamp commit message<br>
<br>
 v2:<br>
  - add missing &quot;static&quot; modifier to set_openssl_env_vars() declaration<br>
spotted 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/crypto_openssl.c                  |  2 -<br>
 src/openvpn/win32.c                           | 88 +++++++++++++++++++<br>
 src/openvpn/win32.h                           |  8 +-<br>
 7 files changed, 95 insertions(+), 32 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/crypto_openssl.c b/src/openvpn/crypto_openssl.c<br>
index c9dc9d0a..ef520928 100644<br>
--- a/src/openvpn/crypto_openssl.c<br>
+++ b/src/openvpn/crypto_openssl.c<br>
@@ -154,13 +154,11 @@ crypto_init_lib_engine(const char *engine_name)<br>
 void<br>
 crypto_init_lib(void)<br>
 {<br>
-#ifndef _WIN32<br>
 #if (OPENSSL_VERSION_NUMBER &gt;= 0x10100000L)<br>
     OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL);<br>
 #else<br>
     OPENSSL_config(NULL);<br>
 #endif<br>
-#endif /* _WIN32 */<br>
     /*<br>
      * If you build the OpenSSL library and OpenVPN with<br>
      * CRYPTO_MDEBUG, you will get a listing of OpenSSL<br>
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c<br>
index 6cff17b2..ee4d3f66 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,84 @@ 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);</blockquote><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>
+<br>
+    if ((install_path[wcslen(install_path) - 1]) == L&#39;\\&#39;)<br>
+    {<br>
+        install_path[wcslen(install_path) - 1] = L&#39;\0&#39;;<br>
+    }<br>
+<br>
+    static struct {<br>
+        WCHAR* name;<br>
+        WCHAR* value;<br>
+    } ossl_env[] = {<br>
+        {L&quot;OPENSSL_CONF&quot;, L&quot;openssl.cnf&quot;},<br>
+        {L&quot;OPENSSL_ENGINES&quot;, L&quot;engines&quot;},<br>
+        {L&quot;OPENSSL_MODULES&quot;, L&quot;modules&quot;}<br>
+    };<br></blockquote><div><br></div><div>As you defined this inside the function, it doesn&#39;t have to be static. Also, name and value could be const. Our preferred style is &quot;TYPE *val&quot;, not the &quot;TYPE* val&quot; used here and elsewhere in the patch. Neither are worth a v5, IMO.</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>
+    for (size_t i = 0; i &lt; SIZE(ossl_env); ++i)<br>
+    {<br>
+        size_t size = 0;<br>
+<br>
+        _wgetenv_s(&amp;size, NULL, 0, ossl_env[i].name);<br>
+        if (size == 0)<br>
+        {<br>
+            WCHAR val[MAX_PATH] = {0};<br>
+            openvpn_swprintf(val, _countof(val), L&quot;%ls\\ssl\\%ls&quot;, install_path, ossl_env[i].value);<br>
+            _wputenv_s(ossl_env[i].name, val);<br>
+        }<br>
+    }<br>
+}<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, ...); </blockquote><div><br>Now there is a duplicate declaration in buffer.h which could be removed. Multiple declarations is not an error, so let it be...</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
 #endif /* ifndef OPENVPN_WIN32_H */<br>
 #endif /* ifdef _WIN32 */<br></blockquote><div><br></div><div>Acked-by: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;</div></div></div>

Your patch has been applied to the master and release/2.5 branch
(I consider this a bugfix since the "do not load config!" CVE patch
unintendedly broke functionality for people)

As instructed I have changed "* " to " *" according to style :-), and
removed the double declaration in buffer.h - the latter is something
on the edge of "can I do that at commit time?" but in this case it's
"the very same declaration", "tun.c and win32.c do include win32.h",
so easy enough.

I did not change the "static" bit.

I have not tested this "for real", just did a test compile of master
and release/2.5 on MinGW.  But the code change looks good, and it does
not break compilation, even with my changes :-)

Added the trac reference to #1296 (thanks Lev for digging it up).

commit 23e6aaef149bd31a7e80af28ee1e3658d2810d4f (master)
commit f911b3f69b0a8296918a06d02eb5144bb4cd8a06 (release/2.5)
Author: Lev Stipakov
Date:   Fri Nov 19 03:55:48 2021 +0200

     Load OpenSSL config on Windows from trusted location

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <20211119015548.687-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23248.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Do we need this fix in openvpn-gui? It only (?) uses openssl to change
private key password, could this functionality be affected by config?

ke 24. marrask. 2021 klo 12.06 Gert Doering (gert@greenie.muc.de) kirjoitti:
>
> Your patch has been applied to the master and release/2.5 branch
> (I consider this a bugfix since the "do not load config!" CVE patch
> unintendedly broke functionality for people)
>
> As instructed I have changed "* " to " *" according to style :-), and
> removed the double declaration in buffer.h - the latter is something
> on the edge of "can I do that at commit time?" but in this case it's
> "the very same declaration", "tun.c and win32.c do include win32.h",
> so easy enough.
>
> I did not change the "static" bit.
>
> I have not tested this "for real", just did a test compile of master
> and release/2.5 on MinGW.  But the code change looks good, and it does
> not break compilation, even with my changes :-)
>
> Added the trac reference to #1296 (thanks Lev for digging it up).
>
> commit 23e6aaef149bd31a7e80af28ee1e3658d2810d4f (master)
> commit f911b3f69b0a8296918a06d02eb5144bb4cd8a06 (release/2.5)
> Author: Lev Stipakov
> Date:   Fri Nov 19 03:55:48 2021 +0200
>
>      Load OpenSSL config on Windows from trusted location
>
>      Signed-off-by: Lev Stipakov <lev@openvpn.net>
>      Acked-by: Selva Nair <selva.nair@gmail.com>
>      Message-Id: <20211119015548.687-1-lstipakov@gmail.com>
>      URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23248.html
>      Signed-off-by: Gert Doering <gert@greenie.muc.de>
>
>
> --
> kind regards,
>
> Gert Doering
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel



--
-Lev

Hi

On Wed, Nov 24, 2021 at 5:06 AM Gert Doering <gert@greenie.muc.de> wrote:

> Your patch has been applied to the master and release/2.5 branch
> (I consider this a bugfix since the "do not load config!" CVE patch
> unintendedly broke functionality for people)
>

What would be a good location in the man page where we can document this.
These are not env vars we natively support so putting it under a section
named "ENVIRONMENT VARIABLES" does not seem right. Also we already have a
section with that name which refer to env vars we export to scripts.

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 24, 2021 at 5:06 AM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</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">Your patch has been applied to the master and release/2.5 branch<br>
(I consider this a bugfix since the &quot;do not load config!&quot; CVE patch<br>
unintendedly broke functionality for people)<br></blockquote><div><br></div><div>What would be a good location in the man page where we can document this. These are not env vars we natively support so putting it under a section named &quot;ENVIRONMENT VARIABLES&quot; does not seem right. Also we already have a section with that name which refer to env vars we export to scripts.</div><div><br></div><div>Selva</div></div></div>

Hi,

On Wed, Nov 24, 2021 at 9:28 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> Do we need this fix in openvpn-gui? It only (?) uses openssl to change
> private key password, could this functionality be affected by config?
>

I do not know.. We do not call any functions that would lead to a config
loading, so probably not required. Automatic crypto initialization does not
load the config, nor does an explicit call to OPENSSL_init_crypto() unless
instructed to. OPENSSL_init_ssl() loads the config unless explicitly
disabled, but we do not use it in the GUI.

However, to be on the safe side, we could set these env vars if not already
set by the user.

Selva
<div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 24, 2021 at 9:28 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">Do we need this fix in openvpn-gui? It only (?) uses openssl to change<br>
private key password, could this functionality be affected by config?<br></blockquote><div><br></div><div>I do not know.. We do not call any functions that would lead to a config loading, so probably not required. Automatic crypto initialization does not load the config, nor does an explicit call to OPENSSL_init_crypto() unless instructed to. OPENSSL_init_ssl() loads the config unless explicitly disabled, but we do not use it in the GUI. </div><div><br></div><div>However, to be on the safe side, we could set these env vars if not already set by the user. </div><div><br></div><div>Selva</div></div></div>