[Openvpn-devel,2/2] Handle EVP_MD_CTX as an opaque struct

Message ID 20220811120722.29168-2-maximilian.fillinger@foxcrypto.com
State Accepted
Headers show
Series [Openvpn-devel,1/2] Update openssl_compat.h for newer LibreSSL | expand

Commit Message

Maximilian Fillinger Aug. 11, 2022, 2:07 a.m. UTC
Building OpenVPN on the latest OpenBSD snapshot failed because EVP_MD_CTX
is an opaque struct in LibreSSL now. Therefore, call md_ctx_new() instead
of declaring them on the stack. When they're not on the stack anymore, we
don't have to call EVP_MD_CTX_init() anymore, but we need to call
EVP_MD_CTX_free() instead of cleanup.

Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
---
 src/openvpn/crypto_openssl.c | 38 ++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

Comments

Arne Schwabe Aug. 11, 2022, 2:21 a.m. UTC | #1
Am 11.08.22 um 14:07 schrieb Max Fillinger:
> Building OpenVPN on the latest OpenBSD snapshot failed because EVP_MD_CTX
> is an opaque struct in LibreSSL now. Therefore, call md_ctx_new() instead
> of declaring them on the stack. When they're not on the stack anymore, we
> don't have to call EVP_MD_CTX_init() anymore, but we need to call
> EVP_MD_CTX_free() instead of cleanup.

Urgh. The whole reason I left this code with the EVP_MD_CTX is that it 
is OpenSSL 1.0.2 only and I expected to be able to remove it sooner or 
later. So LibreSSL doeds not support the alternative API for that?

     EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_TLS1_PRF, NULL);

is what we use for OpenSSL 1.1.0+

I am not happy to soon have LibreSSL specific code in our code but it 
seems like if want to continue that library, we have to.

The change looks good itself.

Acked-By: Arne Schwabe <arne@rfc2549.org>
Maximilian Fillinger Aug. 11, 2022, 2:36 a.m. UTC | #2
> -----Original Message-----
> From: Arne Schwabe [mailto:arne@rfc2549.org]
> Sent: donderdag 11 augustus 2022 14:21
> To: Maximilian Fillinger <maximilian.fillinger@foxcrypto.com>; openvpn-
> devel@lists.sourceforge.net
> Subject: Re: [Openvpn-devel] [PATCH 2/2] Handle EVP_MD_CTX as an opaque
> struct
> 
> Am 11.08.22 um 14:07 schrieb Max Fillinger:
> > Building OpenVPN on the latest OpenBSD snapshot failed because
> EVP_MD_CTX
> > is an opaque struct in LibreSSL now. Therefore, call md_ctx_new()
> instead
> > of declaring them on the stack. When they're not on the stack anymore,
> we
> > don't have to call EVP_MD_CTX_init() anymore, but we need to call
> > EVP_MD_CTX_free() instead of cleanup.
> 
> Urgh. The whole reason I left this code with the EVP_MD_CTX is that it
> is OpenSSL 1.0.2 only and I expected to be able to remove it sooner or
> later. So LibreSSL doeds not support the alternative API for that?
> 
>      EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_TLS1_PRF, NULL);
> 
> is what we use for OpenSSL 1.1.0+
> 
> I am not happy to soon have LibreSSL specific code in our code but it
> seems like if want to continue that library, we have to.
> 
> The change looks good itself.
> 
> Acked-By: Arne Schwabe <arne@rfc2549.org>

LibreSSL now has EVP_PKEY_CTX_new_id(), but it does not define EVP_PKEY_TLS1_PRF.
Gert Doering Aug. 22, 2022, 8:48 a.m. UTC | #3
Same as for the previous patch, I have not tested this on a recent-enough
OpenBSD or OpenSSL 1.0.x (no time to build or find such a system) but I
have tested on FreeBSD with 1.1.1l and 3.0.3 - both OpenSSL builds still
work fine and pass client side tests.

Arne says this does not apply cleanly to 2.5, so if we want it there,
please send a (tested :-) ) backport.

Your patch has been applied to the master branch.

commit 5a9d5dbf5c2d0fc979a05c84afd05689f2ad99b0
Author: Max Fillinger
Date:   Thu Aug 11 14:07:22 2022 +0200

     Handle EVP_MD_CTX as an opaque struct

     Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20220811120722.29168-2-maximilian.fillinger@foxcrypto.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24873.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 5cd09e33..5c86268d 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -1492,7 +1492,7 @@  tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
 {
     int chunk;
     size_t j;
-    EVP_MD_CTX ctx, ctx_tmp, ctx_init;
+    EVP_MD_CTX *ctx, *ctx_tmp, *ctx_init;
     EVP_PKEY *mac_key;
     unsigned char A1[EVP_MAX_MD_SIZE];
     size_t A1_len = EVP_MAX_MD_SIZE;
@@ -1501,28 +1501,28 @@  tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
     chunk = EVP_MD_size(md);
     OPENSSL_assert(chunk >= 0);
 
-    EVP_MD_CTX_init(&ctx);
-    EVP_MD_CTX_init(&ctx_tmp);
-    EVP_MD_CTX_init(&ctx_init);
-    EVP_MD_CTX_set_flags(&ctx_init, EVP_MD_CTX_FLAG_NON_FIPS_ALLOW);
+    ctx = md_ctx_new();
+    ctx_tmp = md_ctx_new();
+    ctx_init = md_ctx_new();
+    EVP_MD_CTX_set_flags(ctx_init, EVP_MD_CTX_FLAG_NON_FIPS_ALLOW);
     mac_key = EVP_PKEY_new_mac_key(EVP_PKEY_HMAC, NULL, sec, sec_len);
     if (!mac_key)
     {
         goto err;
     }
-    if (!EVP_DigestSignInit(&ctx_init, NULL, md, NULL, mac_key))
+    if (!EVP_DigestSignInit(ctx_init, NULL, md, NULL, mac_key))
     {
         goto err;
     }
-    if (!EVP_MD_CTX_copy_ex(&ctx, &ctx_init))
+    if (!EVP_MD_CTX_copy_ex(ctx, ctx_init))
     {
         goto err;
     }
-    if (!EVP_DigestSignUpdate(&ctx, seed, seed_len))
+    if (!EVP_DigestSignUpdate(ctx, seed, seed_len))
     {
         goto err;
     }
-    if (!EVP_DigestSignFinal(&ctx, A1, &A1_len))
+    if (!EVP_DigestSignFinal(ctx, A1, &A1_len))
     {
         goto err;
     }
@@ -1530,19 +1530,19 @@  tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
     for (;; )
     {
         /* Reinit mac contexts */
-        if (!EVP_MD_CTX_copy_ex(&ctx, &ctx_init))
+        if (!EVP_MD_CTX_copy_ex(ctx, ctx_init))
         {
             goto err;
         }
-        if (!EVP_DigestSignUpdate(&ctx, A1, A1_len))
+        if (!EVP_DigestSignUpdate(ctx, A1, A1_len))
         {
             goto err;
         }
-        if (olen > chunk && !EVP_MD_CTX_copy_ex(&ctx_tmp, &ctx))
+        if (olen > chunk && !EVP_MD_CTX_copy_ex(ctx_tmp, ctx))
         {
             goto err;
         }
-        if (!EVP_DigestSignUpdate(&ctx, seed, seed_len))
+        if (!EVP_DigestSignUpdate(ctx, seed, seed_len))
         {
             goto err;
         }
@@ -1550,14 +1550,14 @@  tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
         if (olen > chunk)
         {
             j = olen;
-            if (!EVP_DigestSignFinal(&ctx, out, &j))
+            if (!EVP_DigestSignFinal(ctx, out, &j))
             {
                 goto err;
             }
             out += j;
             olen -= j;
             /* calc the next A1 value */
-            if (!EVP_DigestSignFinal(&ctx_tmp, A1, &A1_len))
+            if (!EVP_DigestSignFinal(ctx_tmp, A1, &A1_len))
             {
                 goto err;
             }
@@ -1566,7 +1566,7 @@  tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
         {
             A1_len = EVP_MAX_MD_SIZE;
             /* last one */
-            if (!EVP_DigestSignFinal(&ctx, A1, &A1_len))
+            if (!EVP_DigestSignFinal(ctx, A1, &A1_len))
             {
                 goto err;
             }
@@ -1577,9 +1577,9 @@  tls1_P_hash(const EVP_MD *md, const unsigned char *sec,
     ret = true;
 err:
     EVP_PKEY_free(mac_key);
-    EVP_MD_CTX_cleanup(&ctx);
-    EVP_MD_CTX_cleanup(&ctx_tmp);
-    EVP_MD_CTX_cleanup(&ctx_init);
+    EVP_MD_CTX_free(ctx);
+    EVP_MD_CTX_free(ctx_tmp);
+    EVP_MD_CTX_free(ctx_init);
     OPENSSL_cleanse(A1, sizeof(A1));
     return ret;
 }