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

Message ID 20211119005306.624-1-lstipakov@gmail.com
State Superseded
Headers show
Series [Openvpn-devel,v3] Load OpenSSL config on Windows from trusted location | expand

Commit Message

Lev Stipakov Nov. 18, 2021, 1:53 p.m. UTC
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.

However, ability to load config might be useful for some users.

This brings config loading 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>
---

 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                           | 92 +++++++++++++++++++
 src/openvpn/win32.h                           |  8 +-
 7 files changed, 99 insertions(+), 32 deletions(-)

Comments

Lev Stipakov Nov. 23, 2021, 7:35 a.m. UTC | #1
I don't have a setup to properly test it, like actually loading the
config - I only checked that the openvpn.exe attempted to access
openssl.cnf at the correct location.

If someone wants to test - binary artifacts could be found here:
https://github.com/lstipakov/openvpn/actions/runs/1496114596

I could also do testing if someone educates me how :)

ti 23. marrask. 2021 klo 20.27 Lev Stipakov (lstipakov@gmail.com) kirjoitti:
>
> 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.
>
> However, ability to load config might be useful for some users.
>
> This brings config loading 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>
> ---
>
>  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                           | 92 +++++++++++++++++++
>  src/openvpn/win32.h                           |  8 +-
>  7 files changed, 99 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..ac7775e4 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,88 @@ 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';
> +    }
> +
> +    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=%ls\\ssl\\openssl.cnf", install_path);
> +    openvpn_swprintf(openssl_engines, _countof(openssl_engines),
> +        L"OPENSSL_ENGINES=%ls\\ssl\\engines", install_path);
> +    openvpn_swprintf(openssl_modules, _countof(openssl_modules),
> +        L"OPENSSL_MODULES=%ls\\ssl\\modules", install_path);
> +
> +    if (_wgetenv(L"OPENSSL_CONF") == NULL)
> +    {
> +        _wputenv(openssl_cnf);
> +    }
> +
> +    if (_wgetenv(L"OPENSSL_ENGINES") == NULL)
> +    {
> +        _wputenv(openssl_engines);
> +    }
> +
> +    if (_wgetenv(L"OPENSSL_MODULES") == NULL)
> +    {
> +        _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 */
> --
> 2.23.0.windows.1
>
Gert Doering Nov. 23, 2021, 7:44 a.m. UTC | #2
Hi,

On Fri, Nov 19, 2021 at 02:53:06AM +0200, Lev Stipakov wrote:
> +    if ((install_path[wcslen(install_path) - 1]) == L'\\')
> +    {
> +        install_path[wcslen(install_path) - 1] = L'\0';
> +    }
> +
> +    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=%ls\\ssl\\openssl.cnf", install_path);

This needs to be _countof(openssl_cnf) - even if they are the same
today, they might not be tomorrow.

While at it, I wonder if it is more orderly to move the swprintf()
calls in the "if NULL" clause now...  like this:

   if (_wgetenv(L"OPENSSL_CONF") == NULL)
   {
       WCHAR openssl_cnf[MAX_PATH] = {0};

       openvpn_swprintf(openssl_cnf, _countof(openssl_cnf),
           L"OPENSSL_CONF=%ls\\ssl\\openssl.cnf", install_path);
       _wputenv(openssl_cnf);
   }

(I would not have brought this up for a v4, but the _countof() needs
to be fixed anyway)

Regarding Selva's comment on the scope of the memory passed to _wputenv(),
I checked MS documentation, and it does not say anything.

OTOH, it points to _wputenv_s(varname,string) which might be worth
considering...

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/putenv-s-wputenv-s?view=msvc-170

gert
Selva Nair Nov. 23, 2021, 7:55 a.m. UTC | #3
On Tue, Nov 23, 2021 at 1:46 PM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Fri, Nov 19, 2021 at 02:53:06AM +0200, Lev Stipakov wrote:
> > +    if ((install_path[wcslen(install_path) - 1]) == L'\\')
> > +    {
> > +        install_path[wcslen(install_path) - 1] = L'\0';
> > +    }
> > +
> > +    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=%ls\\ssl\\openssl.cnf", install_path);
>
> This needs to be _countof(openssl_cnf) - even if they are the same
> today, they might not be tomorrow.
>
> While at it, I wonder if it is more orderly to move the swprintf()
> calls in the "if NULL" clause now...  like this:
>
>    if (_wgetenv(L"OPENSSL_CONF") == NULL)
>    {
>        WCHAR openssl_cnf[MAX_PATH] = {0};
>
>        openvpn_swprintf(openssl_cnf, _countof(openssl_cnf),
>            L"OPENSSL_CONF=%ls\\ssl\\openssl.cnf", install_path);
>        _wputenv(openssl_cnf);
>    }
>
> (I would not have brought this up for a v4, but the _countof() needs
> to be fixed anyway)
>

If you are doing a v4 you may want to consider:

static struct {
   wchar_t *name;
   wchar_t *value;
} ossl_env[] = {{L"OPENSSL_CNF", L"openssl.cnf"},
                        {"OPENSSL_ENGINES", L"engines"},
                        {..}};

and use a loop. Less local arrays, easy to add more to the env zoo later
etc..

Just saying --- I'm okay with the current style too.


> Regarding Selva's comment on the scope of the memory passed to _wputenv(),
> I checked MS documentation, and it does not say anything.
>

MS docs say _putenv is their implementation of POSIX putenv but the latter
does imply the pointer supplied by the user should be used as is. Anyway,
if it works with automatics, good for us.


>
> OTOH, it points to _wputenv_s(varname,string) which might be worth
> considering...
>

+1 to that especially if MSVC is planning to deprecate _wputenv as they
have already done for a  host of other such functions.

Selva
<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 23, 2021 at 1:46 PM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">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">Hi,<br>
<br>
On Fri, Nov 19, 2021 at 02:53:06AM +0200, Lev Stipakov wrote:<br>
&gt; +    if ((install_path[wcslen(install_path) - 1]) == L&#39;\\&#39;)<br>
&gt; +    {<br>
&gt; +        install_path[wcslen(install_path) - 1] = L&#39;\0&#39;;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    WCHAR openssl_cnf[MAX_PATH] = {0};<br>
&gt; +    WCHAR openssl_engines[MAX_PATH] = {0};<br>
&gt; +    WCHAR openssl_modules[MAX_PATH] = {0};<br>
&gt; +<br>
&gt; +    openvpn_swprintf(openssl_cnf, _countof(install_path),<br>
&gt; +        L&quot;OPENSSL_CONF=%ls\\ssl\\openssl.cnf&quot;, install_path);<br>
<br>
This needs to be _countof(openssl_cnf) - even if they are the same<br>
today, they might not be tomorrow.<br>
<br>
While at it, I wonder if it is more orderly to move the swprintf()<br>
calls in the &quot;if NULL&quot; clause now...  like this:<br>
<br>
   if (_wgetenv(L&quot;OPENSSL_CONF&quot;) == NULL)<br>
   {<br>
       WCHAR openssl_cnf[MAX_PATH] = {0};<br>
<br>
       openvpn_swprintf(openssl_cnf, _countof(openssl_cnf),<br>
           L&quot;OPENSSL_CONF=%ls\\ssl\\openssl.cnf&quot;, install_path);<br>
       _wputenv(openssl_cnf);<br>
   }<br>
<br>
(I would not have brought this up for a v4, but the _countof() needs<br>
to be fixed anyway)<br></blockquote><div><br></div><div>If you are doing a v4 you may want to consider:</div><div><br></div><div>static struct {</div><div>   wchar_t *name;</div><div>   wchar_t *value;</div><div>} ossl_env[] = {{L&quot;OPENSSL_CNF&quot;, L&quot;openssl.cnf&quot;},</div><div>                        {&quot;OPENSSL_ENGINES&quot;, L&quot;engines&quot;},</div><div>                        {..}};</div><div><br></div><div>and use a loop. Less local arrays, easy to add more to the env zoo later etc..</div><div><br></div><div>Just saying --- I&#39;m okay with the current style too.  </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>
Regarding Selva&#39;s comment on the scope of the memory passed to _wputenv(),<br>
I checked MS documentation, and it does not say anything.<br></blockquote><div><br></div><div>MS docs say _putenv is their implementation of POSIX putenv but the latter does imply the pointer supplied by the user should be used as is. Anyway, if it works with automatics, good for us.</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>
OTOH, it points to _wputenv_s(varname,string) which might be worth<br>
considering...<br></blockquote><div><br></div><div>+1 to that especially if MSVC is planning to deprecate _wputenv as they have already done for a  host of other such functions.</div><div><br></div><div>Selva</div></div></div>
Selva Nair Nov. 23, 2021, 7:57 a.m. UTC | #4
Hi,

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

> I don't have a setup to properly test it, like actually loading the
> config - I only checked that the openvpn.exe attempted to access
> openssl.cnf at the correct location.
>
> If someone wants to test - binary artifacts could be found here:
> https://github.com/lstipakov/openvpn/actions/runs/1496114596


>
> I could also do testing if someone educates me how :)
>

Try using an openssl.cnf like the one below which restricts signature
algorithms to a some non-PSS schemes. Change that line to restrict them
further or comment out to use defaults: not including PSS will force
non-PSS signature with TLS 1.2 even with OpenSSL 1.1.1 server. And will
break TLS 1.3 negotiation. Removing ECC signatures and using an EC key
certificate will break the connection etc..

#
# OpenSSL configuration file to restrict siglags during handshake
#
openssl_conf = default_conf

[default_conf]
ssl_conf = ssl_sect

[ssl_sect]
system_default = system_default_sect

[system_default_sect]
#MinProtocol = TLSv1.2
#CipherString = DEFAULT@SECLEVEL=0
SignatureAlgorithms =
RSA+SHA256:RSA+SHA384:RSA+SHA512:ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:RSA+SHA1:ECDSA+SHA1
# possible values
# PKCS1:  rsa_pkcs1_sha256:rsa_pkcs1_sha384:rsa_pkcs1_sha512
# ECDSA:
ecdsa_secp256r1_sha256:ecdsa_secp384r1_sha384:ecdsa_secp521r1_sha512
# PSS with rsa encryption public key
rsa_pss_rsae_sha256:rsa_pss_rsae_sha384:rsa_pss_rsae_sha512
# EdDSA :ed25519:ed448
# PSS with PSS public key:
rsa_pss_pss_sha256:rsa_pss_pss_sha384:rsa_pss_pss_sha512
# Legacy rsa_pkcs1_sha1:ecdsa_sha1
<div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 23, 2021 at 1:37 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">I don&#39;t have a setup to properly test it, like actually loading the<br>
config - I only checked that the openvpn.exe attempted to access<br>
openssl.cnf at the correct location.<br>
<br>
If someone wants to test - binary artifacts could be found here:<br>
<a href="https://github.com/lstipakov/openvpn/actions/runs/1496114596" rel="noreferrer" target="_blank">https://github.com/lstipakov/openvpn/actions/runs/1496114596</a> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
I could also do testing if someone educates me how :)<br></blockquote><div><br></div><div>Try using an openssl.cnf like the one below which restricts signature algorithms to a some non-PSS schemes. Change that line to restrict them further or comment out to use defaults: not including PSS will force non-PSS signature with TLS 1.2 even with OpenSSL 1.1.1 server. And will break TLS 1.3 negotiation. Removing ECC signatures and using an EC key certificate will break the connection etc..</div><div><br></div><div>#<br># OpenSSL configuration file to restrict siglags during handshake<br>#<br>openssl_conf = default_conf<br><br>[default_conf]<br>ssl_conf = ssl_sect<br><br>[ssl_sect]<br>system_default = system_default_sect<br><br>[system_default_sect]<br>#MinProtocol = TLSv1.2<br>#CipherString = DEFAULT@SECLEVEL=0<br></div><div>SignatureAlgorithms = RSA+SHA256:RSA+SHA384:RSA+SHA512:ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:RSA+SHA1:ECDSA+SHA1</div><div># possible values<br># PKCS1:  rsa_pkcs1_sha256:rsa_pkcs1_sha384:rsa_pkcs1_sha512<br># ECDSA: ecdsa_secp256r1_sha256:ecdsa_secp384r1_sha384:ecdsa_secp521r1_sha512<br># PSS with rsa encryption public key rsa_pss_rsae_sha256:rsa_pss_rsae_sha384:rsa_pss_rsae_sha512<br># EdDSA :ed25519:ed448<br># PSS with PSS public key: rsa_pss_pss_sha256:rsa_pss_pss_sha384:rsa_pss_pss_sha512<br># Legacy rsa_pkcs1_sha1:ecdsa_sha1</div></div></div>
Lev Stipakov Nov. 23, 2021, 8:33 a.m. UTC | #5
Thanks,

I tried this one and client wasn't able to connect:

    OpenSSL: error:14201076:SSL routines:tls_choose_sigalg:no suitable
signature algorithm

So it looks like config loading works.

The binaries for V4 could be found here:
https://github.com/lstipakov/openvpn/actions/runs/1496339867

ti 23. marrask. 2021 klo 20.58 Selva Nair (selva.nair@gmail.com) kirjoitti:
>
> Hi,
>
> On Tue, Nov 23, 2021 at 1:37 PM Lev Stipakov <lstipakov@gmail.com> wrote:
>>
>> I don't have a setup to properly test it, like actually loading the
>> config - I only checked that the openvpn.exe attempted to access
>> openssl.cnf at the correct location.
>>
>> If someone wants to test - binary artifacts could be found here:
>> https://github.com/lstipakov/openvpn/actions/runs/1496114596
>>
>>
>>
>> I could also do testing if someone educates me how :)
>
>
> Try using an openssl.cnf like the one below which restricts signature algorithms to a some non-PSS schemes. Change that line to restrict them further or comment out to use defaults: not including PSS will force non-PSS signature with TLS 1.2 even with OpenSSL 1.1.1 server. And will break TLS 1.3 negotiation. Removing ECC signatures and using an EC key certificate will break the connection etc..
>
> #
> # OpenSSL configuration file to restrict siglags during handshake
> #
> openssl_conf = default_conf
>
> [default_conf]
> ssl_conf = ssl_sect
>
> [ssl_sect]
> system_default = system_default_sect
>
> [system_default_sect]
> #MinProtocol = TLSv1.2
> #CipherString = DEFAULT@SECLEVEL=0
> SignatureAlgorithms = RSA+SHA256:RSA+SHA384:RSA+SHA512:ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:RSA+SHA1:ECDSA+SHA1
> # possible values
> # PKCS1:  rsa_pkcs1_sha256:rsa_pkcs1_sha384:rsa_pkcs1_sha512
> # ECDSA: ecdsa_secp256r1_sha256:ecdsa_secp384r1_sha384:ecdsa_secp521r1_sha512
> # PSS with rsa encryption public key rsa_pss_rsae_sha256:rsa_pss_rsae_sha384:rsa_pss_rsae_sha512
> # EdDSA :ed25519:ed448
> # PSS with PSS public key: rsa_pss_pss_sha256:rsa_pss_pss_sha384:rsa_pss_pss_sha512
> # Legacy rsa_pkcs1_sha1:ecdsa_sha1

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/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..ac7775e4 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,88 @@  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';
+    }
+
+    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=%ls\\ssl\\openssl.cnf", install_path);
+    openvpn_swprintf(openssl_engines, _countof(openssl_engines),
+        L"OPENSSL_ENGINES=%ls\\ssl\\engines", install_path);
+    openvpn_swprintf(openssl_modules, _countof(openssl_modules),
+        L"OPENSSL_MODULES=%ls\\ssl\\modules", install_path);
+
+    if (_wgetenv(L"OPENSSL_CONF") == NULL)
+    {
+        _wputenv(openssl_cnf);
+    }
+
+    if (_wgetenv(L"OPENSSL_ENGINES") == NULL)
+    {
+        _wputenv(openssl_engines);
+    }
+
+    if (_wgetenv(L"OPENSSL_MODULES") == NULL)
+    {
+        _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 */