diff mbox series

[Openvpn-devel,v3,3/3] Handle the unlikely case that PRF generation fails

Message ID 20210201174310.22153-4-arne@rfc2549.org
State Superseded
Headers show
Series [Openvpn-devel,v3,1/3] Check return values in md_ctx_init and hmac_ctx_init | expand

Commit Message

Arne Schwabe Feb. 1, 2021, 6:43 a.m. UTC 
We never had handling of this failure condition. But should it happen
we can now handle it.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto_backend.h |  4 +-
 src/openvpn/crypto_mbedtls.c | 17 ++++----
 src/openvpn/crypto_openssl.c | 50 ++++++++++++++++++-----
 src/openvpn/ssl.c            | 78 ++++++++++++++++++++++--------------
 4 files changed, 99 insertions(+), 50 deletions(-)

Comments

Gert Doering March 18, 2021, 12:39 a.m. UTC | #1 
Hi,

On Mon, Feb 01, 2021 at 06:43:10PM +0100, Arne Schwabe wrote:
> We never had handling of this failure condition. But should it happen
> we can now handle it.

For the sake of the list archives: this patch was folded into the
larger "Prefer TLS libraries TLS PRF function, fix OpenVPN in FIPS mode"
patch.  So it is already in master via "commit 06f6cf3ff850f2".

Tagging in patchwork as "Superseded".

gert
diff mbox series

Patch

diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index d86d9a34..384ffc80 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -710,7 +710,9 @@  const char *translate_cipher_name_to_openvpn(const char *cipher_name);
  * @param secret_len    length of the secret
  * @param output        output destination
  * @param output_len    length of output/number of bytes to generate
+ *
+ * @return              true if successful, false on any error
  */
-void ssl_tls1_PRF(const uint8_t *seed, int seed_len, const uint8_t *secret,
+bool ssl_tls1_PRF(const uint8_t *seed, int seed_len, const uint8_t *secret,
                   int secret_len, uint8_t *output, int output_len);
 #endif /* CRYPTO_BACKEND_H_ */
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 5ed88d6d..3b28ba26 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -987,11 +987,12 @@  memcmp_constant_time(const void *a, const void *b, size_t size)
 }
 /* mbedtls-2.18.0 or newer */
 #ifdef HAVE_MBEDTLS_SSL_TLS_PRF
-void ssl_tls1_PRF(const uint8_t *seed, int seed_len, const uint8_t *secret,
+bool ssl_tls1_PRF(const uint8_t *seed, int seed_len, const uint8_t *secret,
                   int secret_len, uint8_t *output, int output_len)
 {
-    mbedtls_ssl_tls_prf(MBEDTLS_SSL_TLS_PRF_TLS1, secret, secret_len, "", seed,
-                        seed_len, output, output_len);
+    return mbed_ok(mbedtls_ssl_tls_prf(MBEDTLS_SSL_TLS_PRF_TLS1, secret,
+                                       secret_len, "", seed, seed_len, output,
+                                       output_len));
 }
 #else
 /*
@@ -1088,9 +1089,8 @@  tls1_P_hash(const md_kt_t *md_kt,
  * (1) key_block contains a full set of 4 keys.
  * (2) The pre-master secret is generated by the client.
  */
-void
-ssl_tls1_PRF(const uint8_t *label, int label_len, const uint8_t *sec, int slen,
-         uint8_t *out1, int olen)
+bool ssl_tls1_PRF(const uint8_t *label, int label_len, const uint8_t *sec,
+                  int slen, uint8_t *out1, int olen)
 {
     struct gc_arena gc = gc_new();
     const md_kt_t *md5 = md_kt_get("MD5");
@@ -1103,8 +1103,8 @@  ssl_tls1_PRF(const uint8_t *label, int label_len, const uint8_t *sec, int slen,
     const uint8_t *S2 = &(sec[len]);
     len += (slen&1); /* add for odd, make longer */
 
-    tls1_P_hash(md5,S1,len,label,label_len,out1,olen);
-    tls1_P_hash(sha1,S2,len,label,label_len,out2,olen);
+    tls1_P_hash(md5, S1, len, label, label_len, out1, olen);
+    tls1_P_hash(sha1, S2, len, label, label_len, out2, olen);
 
     for (int i = 0; i<olen; i++)
     {
@@ -1116,6 +1116,7 @@  ssl_tls1_PRF(const uint8_t *label, int label_len, const uint8_t *sec, int slen,
     dmsg(D_SHOW_KEY_SOURCE, "tls1_PRF out[%d]: %s", olen, format_hex(out1, olen, 0, &gc));
 
     gc_free(&gc);
+    return true;
 }
 #endif
 #endif /* ENABLE_CRYPTO_MBEDTLS */
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index b42dc044..c5ae2835 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -1128,21 +1128,41 @@  engine_load_key(const char *file, SSL_CTX *ctx)
 }
 
 #if OPENSSL_VERSION_NUMBER >= 0x10100000L
-void ssl_tls1_PRF(const uint8_t *seed, int seed_len, const uint8_t *secret,
+bool ssl_tls1_PRF(const uint8_t *seed, int seed_len, const uint8_t *secret,
                   int secret_len, uint8_t *output, int output_len)
 {
     EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_TLS1_PRF, NULL);
-    ASSERT(EVP_PKEY_derive_init(pctx) == 1);
+    if(!EVP_PKEY_derive_init(pctx))
+    {
+        return false;
+    }
 
-    ASSERT(EVP_PKEY_CTX_set_tls1_prf_md(pctx, EVP_md5_sha1()) == 1);
 
-    ASSERT(EVP_PKEY_CTX_set1_tls1_prf_secret(pctx, secret, secret_len) == 1);
+    if(!EVP_PKEY_CTX_set_tls1_prf_md(pctx, EVP_md5_sha1()))
+    {
+        return false;
+    }
 
-    ASSERT(EVP_PKEY_CTX_add1_tls1_prf_seed(pctx, seed, seed_len) == 1);
+    if(!EVP_PKEY_CTX_set1_tls1_prf_secret(pctx, secret, secret_len))
+    {
+        return false;
+    }
+
+    if(!EVP_PKEY_CTX_add1_tls1_prf_seed(pctx, seed, seed_len))
+    {
+        return false;
+    }
 
     size_t out_len = output_len;
-    ASSERT (EVP_PKEY_derive(pctx, output, &out_len) == 1);
-    ASSERT (out_len == output_len);
+    if(!EVP_PKEY_derive(pctx, output, &out_len))
+    {
+        return false;
+    }
+    if (out_len != output_len)
+    {
+        return false;
+    }
+    return true;
 }
 #else
 /*
@@ -1258,9 +1278,10 @@  err:
  * (1) key_block contains a full set of 4 keys.
  * (2) The pre-master secret is generated by the client.
  */
-void ssl_tls1_PRF(const uint8_t *label, int label_len, const uint8_t *sec,
+bool ssl_tls1_PRF(const uint8_t *label, int label_len, const uint8_t *sec,
                   int slen, uint8_t *out1, int olen)
 {
+    bool ret = true;
     struct gc_arena gc = gc_new();
     /* For some reason our md_kt_get("MD5") fails otherwise in the unit test */
     const md_kt_t *md5 = EVP_md5();
@@ -1274,8 +1295,16 @@  void ssl_tls1_PRF(const uint8_t *label, int label_len, const uint8_t *sec,
     len += (slen&1); /* add for odd, make longer */
 
     if(!tls1_P_hash(md5,S1,len,label,label_len,out1,olen))
+    {
+        ret = false;
+        goto done;
+    }
 
-    ASSERT(tls1_P_hash(sha1,S2,len,label,label_len,out2,olen));
+    if(!tls1_P_hash(sha1,S2,len,label,label_len,out2,olen))
+    {
+        ret = false;
+        goto done;
+    }
 
     for (int i = 0; i<olen; i++)
     {
@@ -1285,8 +1314,9 @@  void ssl_tls1_PRF(const uint8_t *label, int label_len, const uint8_t *sec,
     secure_memzero(out2, olen);
 
     dmsg(D_SHOW_KEY_SOURCE, "tls1_PRF out[%d]: %s", olen, format_hex(out1, olen, 0, &gc));
-
+done:
     gc_free(&gc);
+    return ret;
 }
 #endif
 #endif /* ENABLE_CRYPTO_OPENSSL */
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index c0de76b5..ab9639e6 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1580,7 +1580,7 @@  key_source2_print(const struct key_source2 *k)
     key_source_print(&k->server, "Server");
 }
 
-static void
+static bool
 openvpn_PRF(const uint8_t *secret,
             int secret_len,
             const char *label,
@@ -1593,6 +1593,7 @@  openvpn_PRF(const uint8_t *secret,
             uint8_t *output,
             int output_len)
 {
+    bool ret = true;
     /* concatenate seed components */
 
     struct buffer seed = alloc_buf(strlen(label)
@@ -1614,12 +1615,16 @@  openvpn_PRF(const uint8_t *secret,
     }
 
     /* compute PRF */
-    ssl_tls1_PRF(BPTR(&seed), BLEN(&seed), secret, secret_len, output, output_len);
+    if (!ssl_tls1_PRF(BPTR(&seed), BLEN(&seed), secret, secret_len, output, output_len))
+    {
+        ret = false;
+    }
 
     buf_clear(&seed);
     free_buf(&seed);
 
     VALGRIND_MAKE_READABLE((void *)output, output_len);
+    return ret;
 }
 
 static void
@@ -1654,8 +1659,8 @@  generate_key_expansion_tls_export(struct tls_session *session, struct key2 *key2
     return true;
 }
 
-static struct key2
-generate_key_expansion_openvpn_prf(const struct tls_session *session)
+static bool
+generate_key_expansion_openvpn_prf(const struct tls_session *session, struct key2 *key2)
 {
     uint8_t master[48] = { 0 };
 
@@ -1671,33 +1676,40 @@  generate_key_expansion_openvpn_prf(const struct tls_session *session)
     key_source2_print(key_src);
 
     /* compute master secret */
-    openvpn_PRF(key_src->client.pre_master,
-                sizeof(key_src->client.pre_master),
-                KEY_EXPANSION_ID " master secret",
-                key_src->client.random1,
-                sizeof(key_src->client.random1),
-                key_src->server.random1,
-                sizeof(key_src->server.random1),
-                NULL,
-                NULL,
-                master,
-                sizeof(master));
+    if(!openvpn_PRF(key_src->client.pre_master,
+                    sizeof(key_src->client.pre_master),
+                    KEY_EXPANSION_ID " master secret",
+                    key_src->client.random1,
+                    sizeof(key_src->client.random1),
+                    key_src->server.random1,
+                    sizeof(key_src->server.random1),
+                    NULL,
+                    NULL,
+                    master,
+                    sizeof(master)))
+    {
+        return false;
+    }
 
-    struct key2 key2;
     /* compute key expansion */
-    openvpn_PRF(master,
-                sizeof(master),
-                KEY_EXPANSION_ID " key expansion",
-                key_src->client.random2,
-                sizeof(key_src->client.random2),
-                key_src->server.random2,
-                sizeof(key_src->server.random2),
-                client_sid,
-                server_sid,
-                (uint8_t *)key2.keys,
-                sizeof(key2.keys));
+    if (!openvpn_PRF(master,
+                    sizeof(master),
+                    KEY_EXPANSION_ID " key expansion",
+                    key_src->client.random2,
+                    sizeof(key_src->client.random2),
+                    key_src->server.random2,
+                    sizeof(key_src->server.random2),
+                    client_sid,
+                    server_sid,
+                    (uint8_t *)key2->keys,
+                    sizeof(key2->keys)))
+    {
+        return false;
+    }
     secure_memzero(&master, sizeof(master));
 
+
+
     /*
      * fixup_key only correctly sets DES parity bits if the cipher is a
      * DES variant.
@@ -1712,11 +1724,11 @@  generate_key_expansion_openvpn_prf(const struct tls_session *session)
      */
     for (int i = 0; i < 2; ++i)
     {
-        fixup_key(&key2.keys[i], &session->opt->key_type);
+        fixup_key(&key2->keys[i], &session->opt->key_type);
     }
-    key2.n = 2;
+    key2->n = 2;
 
-    return key2;
+    return true;
 }
 
 /*
@@ -1748,7 +1760,11 @@  generate_key_expansion(struct key_ctx_bi *key,
     }
     else
     {
-        key2 = generate_key_expansion_openvpn_prf(session);
+        if (!generate_key_expansion_openvpn_prf(session, &key2))
+        {
+            msg(D_TLS_ERRORS, "TLS Error: PRF calcuation failed");
+            goto exit;
+        }
     }
 
     key2_print(&key2, &session->opt->key_type,