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

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

Commit Message

Arne Schwabe Feb. 1, 2021, 5:43 p.m.
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(-)

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,