diff mbox series

[Openvpn-devel,1/2] crypto_openssl.c: disable explicit initialization on Windows

Message ID 20210617061226.244-1-lstipakov@gmail.com
State Accepted
Headers show
Series Disable OpenSSL config autoload in Windows | expand

Commit Message

Lev Stipakov June 16, 2021, 8:12 p.m. UTC 
From: Lev Stipakov <lev@openvpn.net>

Commit a4071b ("crypto_openssl: add initialization to pick up local configuration")
added openssl initialization to load configuration file. However on Windows
this file is loaded from user-writable directory, such as c.\etc\ssl for mingw builds
and (for example) c:\vcpkg\packages\openssl_x64-windows\openvpn.cnf for vcpkg
builds. This could be a security risk.

Since aforementioned commit implements a niche feature which anyway
should use CryptoAPI on Windows, make this code conditional.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/crypto_openssl.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Gert Doering June 17, 2021, 4:01 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

As discussed in the meeting yesterday - this might break people's use
of OpenSSL engines on windows, but we assume that the amount of users
of that will be "close to zero".  For these cases we can find a better
approach after 2.5.3 release.

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

commit abd5ee9b7dc4ba85438da5d16bb7dfb31714dac7 (master)
commit 447cfb4f30fd96126f7d2945cd14ef39cc13a08a (release/2.5)
Author: Lev Stipakov
Date:   Thu Jun 17 09:12:26 2021 +0300

     crypto_openssl.c: disable explicit initialization on Windows (CVE-2121-3606)

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


--
kind regards,

Gert Doering
Selva Nair June 17, 2021, 5:25 a.m. UTC | #2 
Hi

On Thu, Jun 17, 2021 at 2:13 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> From: Lev Stipakov <lev@openvpn.net>
>
> Commit a4071b ("crypto_openssl: add initialization to pick up local
> configuration")
> added openssl initialization to load configuration file. However on Windows
> this file is loaded from user-writable directory, such as c.\etc\ssl for
> mingw builds
> and (for example) c:\vcpkg\packages\openssl_x64-windows\openvpn.cnf for
> vcpkg
> builds. This could be a security risk.
>
> Since aforementioned commit implements a niche feature which anyway
> should use CryptoAPI on Windows, make this code conditional.
>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  src/openvpn/crypto_openssl.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index c571030b..603c67b0 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -154,11 +154,13 @@ 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
> --
> 2.23.0.windows.1
>

This would make it impossible to use openssl.cnf on Windows, wouldn't it? I
use configuraton file to restrict signature algorithms, for example. There
are other uses like configuring engines.

Instead of disabling, why not make the default path a restricted location
within, say,  C:\Windows. The user can then override it using env variables.

What path does OpenSSL Windows binaries described in OpensSL wiki use?   (
https://wiki.openssl.org/index.php/Binaries)

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jun 17, 2021 at 2:13 AM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com">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 a4071b (&quot;crypto_openssl: add initialization to pick up local configuration&quot;)<br>
added openssl initialization to load configuration file. However on Windows<br>
this file is loaded from user-writable directory, such as c.\etc\ssl for mingw builds<br>
and (for example) c:\vcpkg\packages\openssl_x64-windows\openvpn.cnf for vcpkg<br>
builds. This could be a security risk.<br>
<br>
Since aforementioned commit implements a niche feature which anyway<br>
should use CryptoAPI on Windows, make this code conditional.<br>
<br>
Signed-off-by: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
---<br>
 src/openvpn/crypto_openssl.c | 2 ++<br>
 1 file changed, 2 insertions(+)<br>
<br>
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c<br>
index c571030b..603c67b0 100644<br>
--- a/src/openvpn/crypto_openssl.c<br>
+++ b/src/openvpn/crypto_openssl.c<br>
@@ -154,11 +154,13 @@ 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>
-- <br>
2.23.0.windows.1<br></blockquote><div><br></div><div>This would make it impossible to use openssl.cnf on Windows, wouldn&#39;t it? I use configuraton file to restrict signature algorithms, for example. There are other uses like configuring engines.</div><div><br></div><div>Instead of disabling, why not make the default path a restricted location within, say,  C:\Windows. The user can then override it using env variables.</div><div><br></div><div>What path does OpenSSL Windows binaries described in OpensSL wiki use?   (<a href="https://wiki.openssl.org/index.php/Binaries">https://wiki.openssl.org/index.php/Binaries</a>)</div><div><br></div><div>Selva</div></div></div>
Gert Doering June 17, 2021, 5:45 a.m. UTC | #3 
Hi,

On Thu, Jun 17, 2021 at 11:25:02AM -0400, Selva Nair wrote:
> This would make it impossible to use openssl.cnf on Windows, wouldn't it? I
> use configuraton file to restrict signature algorithms, for example. There
> are other uses like configuring engines.

Right.

> Instead of disabling, why not make the default path a restricted location
> within, say,  C:\Windows. The user can then override it using env variables.

This is what we tried to achieve, but Lev could not find a way to make
the MSVC build behave wrt "new ETCDIR".  I have not personally investigated.

So we decided to release 2.5.3 soonish, with "disable openssl.cnf" to 
cover the problem reported, and then see if we can get to a better fix 
incrementally (while a "secured" binary is out for users to run).

gert
diff mbox series

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index c571030b..603c67b0 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -154,11 +154,13 @@  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