From: Matt Caswell <matt@openssl.org>
Date: Tue, 4 Dec 2018 08:37:04 +0000 (+0000)
Subject: Fix some SSL_export_keying_material() issues
X-Git-Tag: openssl-3.0.0-alpha1~2843
X-Git-Url: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff_plain;h=0fb2815b873304d145ed00283454fc9f3bd35e6b;ds=sidebyside

Fix some SSL_export_keying_material() issues

Fix some issues in tls13_hkdf_expand() which impact the above function
for TLSv1.3. In particular test that we can use the maximum label length
in TLSv1.3.

Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7755)
---

diff --git a/doc/man3/SSL_export_keying_material.pod b/doc/man3/SSL_export_keying_material.pod
index abebf911fc..4c81a60ffb 100644
--- a/doc/man3/SSL_export_keying_material.pod
+++ b/doc/man3/SSL_export_keying_material.pod
@@ -59,7 +59,8 @@ B<label> and should be B<llen> bytes long. Typically this will be a value from
 the IANA Exporter Label Registry
 (L<https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#exporter-labels>).
 Alternatively labels beginning with "EXPERIMENTAL" are permitted by the standard
-to be used without registration.
+to be used without registration. TLSv1.3 imposes a maximum label length of
+249 bytes.
 
 Note that this function is only defined for TLSv1.0 and above, and DTLSv1.0 and
 above. Attempting to use it in SSLv3 will result in an error.
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 70e5a1740f..307131de93 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -2461,7 +2461,7 @@ __owur int tls13_hkdf_expand(SSL *s, const EVP_MD *md,
                              const unsigned char *secret,
                              const unsigned char *label, size_t labellen,
                              const unsigned char *data, size_t datalen,
-                             unsigned char *out, size_t outlen);
+                             unsigned char *out, size_t outlen, int fatal);
 __owur int tls13_derive_key(SSL *s, const EVP_MD *md,
                             const unsigned char *secret, unsigned char *key,
                             size_t keylen);
diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 63e61c6184..716d6d23e0 100644
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -1506,7 +1506,7 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
 
     /* Generate the binder key */
     if (!tls13_hkdf_expand(s, md, early_secret, label, labelsize, hash,
-                           hashsize, binderkey, hashsize)) {
+                           hashsize, binderkey, hashsize, 1)) {
         /* SSLfatal() already called */
         goto err;
     }
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 5a8f1163df..a0e495d8e8 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -2740,7 +2740,7 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt)
                                PACKET_data(&nonce),
                                PACKET_remaining(&nonce),
                                s->session->master_key,
-                               hashlen)) {
+                               hashlen, 1)) {
             /* SSLfatal() already called */
             goto err;
         }
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index e7c11c4bea..a8e862ced5 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -4099,7 +4099,7 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
                                tick_nonce,
                                TICKET_NONCE_SIZE,
                                s->session->master_key,
-                               hashlen)) {
+                               hashlen, 1)) {
             /* SSLfatal() already called */
             goto err;
         }
diff --git a/ssl/tls13_enc.c b/ssl/tls13_enc.c
index f7ab0fa470..c3021d18aa 100644
--- a/ssl/tls13_enc.c
+++ b/ssl/tls13_enc.c
@@ -13,7 +13,7 @@
 #include <openssl/evp.h>
 #include <openssl/kdf.h>
 
-#define TLS13_MAX_LABEL_LEN     246
+#define TLS13_MAX_LABEL_LEN     249
 
 /* Always filled with zeros */
 static const unsigned char default_zeros[EVP_MAX_MD_SIZE];
@@ -22,30 +22,47 @@ static const unsigned char default_zeros[EVP_MAX_MD_SIZE];
  * Given a |secret|; a |label| of length |labellen|; and |data| of length
  * |datalen| (e.g. typically a hash of the handshake messages), derive a new
  * secret |outlen| bytes long and store it in the location pointed to be |out|.
- * The |data| value may be zero length. Returns 1 on success  0 on failure.
+ * The |data| value may be zero length. Any errors will be treated as fatal if
+ * |fatal| is set. Returns 1 on success  0 on failure.
  */
 int tls13_hkdf_expand(SSL *s, const EVP_MD *md, const unsigned char *secret,
                              const unsigned char *label, size_t labellen,
                              const unsigned char *data, size_t datalen,
-                             unsigned char *out, size_t outlen)
+                             unsigned char *out, size_t outlen, int fatal)
 {
-    const unsigned char label_prefix[] = "tls13 ";
+    static const unsigned char label_prefix[] = "tls13 ";
     EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL);
     int ret;
     size_t hkdflabellen;
     size_t hashlen;
     /*
-     * 2 bytes for length of whole HkdfLabel + 1 byte for length of combined
-     * prefix and label + bytes for the label itself + bytes for the hash
+     * 2 bytes for length of derived secret + 1 byte for length of combined
+     * prefix and label + bytes for the label itself + 1 byte length of hash
+     * + bytes for the hash itself
      */
     unsigned char hkdflabel[sizeof(uint16_t) + sizeof(uint8_t) +
                             + sizeof(label_prefix) + TLS13_MAX_LABEL_LEN
-                            + EVP_MAX_MD_SIZE];
+                            + 1 + EVP_MAX_MD_SIZE];
     WPACKET pkt;
 
     if (pctx == NULL)
         return 0;
 
+    if (labellen > TLS13_MAX_LABEL_LEN) {
+        if (fatal) {
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_HKDF_EXPAND,
+                     ERR_R_INTERNAL_ERROR);
+        } else {
+            /*
+             * Probably we have been called from SSL_export_keying_material(),
+             * or SSL_export_keying_material_early().
+             */
+            SSLerr(SSL_F_TLS13_HKDF_EXPAND, SSL_R_TLS_ILLEGAL_EXPORTER_LABEL);
+        }
+        EVP_PKEY_CTX_free(pctx);
+        return 0;
+    }
+
     hashlen = EVP_MD_size(md);
 
     if (!WPACKET_init_static_len(&pkt, hkdflabel, sizeof(hkdflabel), 0)
@@ -59,8 +76,11 @@ int tls13_hkdf_expand(SSL *s, const EVP_MD *md, const unsigned char *secret,
             || !WPACKET_finish(&pkt)) {
         EVP_PKEY_CTX_free(pctx);
         WPACKET_cleanup(&pkt);
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_HKDF_EXPAND,
-                 ERR_R_INTERNAL_ERROR);
+        if (fatal)
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_HKDF_EXPAND,
+                     ERR_R_INTERNAL_ERROR);
+        else
+            SSLerr(SSL_F_TLS13_HKDF_EXPAND, ERR_R_INTERNAL_ERROR);
         return 0;
     }
 
@@ -74,9 +94,13 @@ int tls13_hkdf_expand(SSL *s, const EVP_MD *md, const unsigned char *secret,
 
     EVP_PKEY_CTX_free(pctx);
 
-    if (ret != 0)
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_HKDF_EXPAND,
-                 ERR_R_INTERNAL_ERROR);
+    if (ret != 0) {
+        if (fatal)
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_HKDF_EXPAND,
+                     ERR_R_INTERNAL_ERROR);
+        else
+            SSLerr(SSL_F_TLS13_HKDF_EXPAND, ERR_R_INTERNAL_ERROR);
+    }
 
     return ret == 0;
 }
@@ -91,7 +115,7 @@ int tls13_derive_key(SSL *s, const EVP_MD *md, const unsigned char *secret,
     static const unsigned char keylabel[] = "key";
 
     return tls13_hkdf_expand(s, md, secret, keylabel, sizeof(keylabel) - 1,
-                             NULL, 0, key, keylen);
+                             NULL, 0, key, keylen, 1);
 }
 
 /*
@@ -104,7 +128,7 @@ int tls13_derive_iv(SSL *s, const EVP_MD *md, const unsigned char *secret,
     static const unsigned char ivlabel[] = "iv";
 
     return tls13_hkdf_expand(s, md, secret, ivlabel, sizeof(ivlabel) - 1,
-                             NULL, 0, iv, ivlen);
+                             NULL, 0, iv, ivlen, 1);
 }
 
 int tls13_derive_finishedkey(SSL *s, const EVP_MD *md,
@@ -114,7 +138,7 @@ int tls13_derive_finishedkey(SSL *s, const EVP_MD *md,
     static const unsigned char finishedlabel[] = "finished";
 
     return tls13_hkdf_expand(s, md, secret, finishedlabel,
-                             sizeof(finishedlabel) - 1, NULL, 0, fin, finlen);
+                             sizeof(finishedlabel) - 1, NULL, 0, fin, finlen, 1);
 }
 
 /*
@@ -177,7 +201,7 @@ int tls13_generate_secret(SSL *s, const EVP_MD *md,
         if (!tls13_hkdf_expand(s, md, prevsecret,
                                (unsigned char *)derived_secret_label,
                                sizeof(derived_secret_label) - 1, hash, mdlen,
-                               preextractsec, mdlen)) {
+                               preextractsec, mdlen, 1)) {
             /* SSLfatal() already called */
             EVP_PKEY_CTX_free(pctx);
             return 0;
@@ -337,7 +361,7 @@ static int derive_secret_key_and_iv(SSL *s, int sending, const EVP_MD *md,
     hashlen = (size_t)hashleni;
 
     if (!tls13_hkdf_expand(s, md, insecret, label, labellen, hash, hashlen,
-                           secret, hashlen)) {
+                           secret, hashlen, 1)) {
         /* SSLfatal() already called */
         goto err;
     }
@@ -517,7 +541,8 @@ int tls13_change_cipher_state(SSL *s, int which)
                                    early_exporter_master_secret,
                                    sizeof(early_exporter_master_secret) - 1,
                                    hashval, hashlen,
-                                   s->early_exporter_master_secret, hashlen)) {
+                                   s->early_exporter_master_secret, hashlen,
+                                   1)) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR,
                          SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);
                 goto err;
@@ -604,7 +629,7 @@ int tls13_change_cipher_state(SSL *s, int which)
                                resumption_master_secret,
                                sizeof(resumption_master_secret) - 1,
                                hashval, hashlen, s->resumption_master_secret,
-                               hashlen)) {
+                               hashlen, 1)) {
             /* SSLfatal() already called */
             goto err;
         }
@@ -624,7 +649,7 @@ int tls13_change_cipher_state(SSL *s, int which)
                                exporter_master_secret,
                                sizeof(exporter_master_secret) - 1,
                                hash, hashlen, s->exporter_master_secret,
-                               hashlen)) {
+                               hashlen, 1)) {
             /* SSLfatal() already called */
             goto err;
         }
@@ -738,10 +763,10 @@ int tls13_export_keying_material(SSL *s, unsigned char *out, size_t olen,
             || EVP_DigestFinal_ex(ctx, data, &datalen) <= 0
             || !tls13_hkdf_expand(s, md, s->exporter_master_secret,
                                   (const unsigned char *)label, llen,
-                                  data, datalen, exportsecret, hashsize)
+                                  data, datalen, exportsecret, hashsize, 0)
             || !tls13_hkdf_expand(s, md, exportsecret, exporterlabel,
                                   sizeof(exporterlabel) - 1, hash, hashsize,
-                                  out, olen))
+                                  out, olen, 0))
         goto err;
 
     ret = 1;
@@ -797,10 +822,10 @@ int tls13_export_keying_material_early(SSL *s, unsigned char *out, size_t olen,
             || EVP_DigestFinal_ex(ctx, data, &datalen) <= 0
             || !tls13_hkdf_expand(s, md, s->early_exporter_master_secret,
                                   (const unsigned char *)label, llen,
-                                  data, datalen, exportsecret, hashsize)
+                                  data, datalen, exportsecret, hashsize, 0)
             || !tls13_hkdf_expand(s, md, exportsecret, exporterlabel,
                                   sizeof(exporterlabel) - 1, hash, hashsize,
-                                  out, olen))
+                                  out, olen, 0))
         goto err;
 
     ret = 1;
diff --git a/test/sslapitest.c b/test/sslapitest.c
index 108d57e478..a4bbb4fead 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -4028,20 +4028,25 @@ static int test_serverinfo(int tst)
  * no test vectors so all we do is test that both sides of the communication
  * produce the same results for different protocol versions.
  */
+#define SMALL_LABEL_LEN 10
+#define LONG_LABEL_LEN  249
 static int test_export_key_mat(int tst)
 {
     int testresult = 0;
     SSL_CTX *cctx = NULL, *sctx = NULL, *sctx2 = NULL;
     SSL *clientssl = NULL, *serverssl = NULL;
-    const char label[] = "test label";
+    const char label[LONG_LABEL_LEN + 1] = "test label";
     const unsigned char context[] = "context";
     const unsigned char *emptycontext = NULL;
     unsigned char ckeymat1[80], ckeymat2[80], ckeymat3[80];
     unsigned char skeymat1[80], skeymat2[80], skeymat3[80];
+    size_t labellen;
     const int protocols[] = {
         TLS1_VERSION,
         TLS1_1_VERSION,
         TLS1_2_VERSION,
+        TLS1_3_VERSION,
+        TLS1_3_VERSION,
         TLS1_3_VERSION
     };
 
@@ -4058,7 +4063,7 @@ static int test_export_key_mat(int tst)
         return 1;
 #endif
 #ifdef OPENSSL_NO_TLS1_3
-    if (tst == 3)
+    if (tst >= 3)
         return 1;
 #endif
     if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(), TLS_client_method(),
@@ -4076,33 +4081,52 @@ static int test_export_key_mat(int tst)
                                                 SSL_ERROR_NONE)))
         goto end;
 
+    if (tst == 5) {
+        /*
+         * TLSv1.3 imposes a maximum label len of 249 bytes. Check we fail if we
+         * go over that.
+         */
+        if (!TEST_int_le(SSL_export_keying_material(clientssl, ckeymat1,
+                                                    sizeof(ckeymat1), label,
+                                                    LONG_LABEL_LEN + 1, context,
+                                                    sizeof(context) - 1, 1), 0))
+            goto end;
+
+        testresult = 1;
+        goto end;
+    } else if (tst == 4) {
+        labellen = LONG_LABEL_LEN;
+    } else {
+        labellen = SMALL_LABEL_LEN;
+    }
+
     if (!TEST_int_eq(SSL_export_keying_material(clientssl, ckeymat1,
                                                 sizeof(ckeymat1), label,
-                                                sizeof(label) - 1, context,
+                                                labellen, context,
                                                 sizeof(context) - 1, 1), 1)
             || !TEST_int_eq(SSL_export_keying_material(clientssl, ckeymat2,
                                                        sizeof(ckeymat2), label,
-                                                       sizeof(label) - 1,
+                                                       labellen,
                                                        emptycontext,
                                                        0, 1), 1)
             || !TEST_int_eq(SSL_export_keying_material(clientssl, ckeymat3,
                                                        sizeof(ckeymat3), label,
-                                                       sizeof(label) - 1,
+                                                       labellen,
                                                        NULL, 0, 0), 1)
             || !TEST_int_eq(SSL_export_keying_material(serverssl, skeymat1,
                                                        sizeof(skeymat1), label,
-                                                       sizeof(label) - 1,
+                                                       labellen,
                                                        context,
                                                        sizeof(context) -1, 1),
                             1)
             || !TEST_int_eq(SSL_export_keying_material(serverssl, skeymat2,
                                                        sizeof(skeymat2), label,
-                                                       sizeof(label) - 1,
+                                                       labellen,
                                                        emptycontext,
                                                        0, 1), 1)
             || !TEST_int_eq(SSL_export_keying_material(serverssl, skeymat3,
                                                        sizeof(skeymat3), label,
-                                                       sizeof(label) - 1,
+                                                       labellen,
                                                        NULL, 0, 0), 1)
                /*
                 * Check that both sides created the same key material with the
@@ -4131,10 +4155,10 @@ static int test_export_key_mat(int tst)
      * Check that an empty context and no context produce different results in
      * protocols less than TLSv1.3. In TLSv1.3 they should be the same.
      */
-    if ((tst != 3 && !TEST_mem_ne(ckeymat2, sizeof(ckeymat2), ckeymat3,
+    if ((tst < 3 && !TEST_mem_ne(ckeymat2, sizeof(ckeymat2), ckeymat3,
                                   sizeof(ckeymat3)))
-            || (tst ==3 && !TEST_mem_eq(ckeymat2, sizeof(ckeymat2), ckeymat3,
-                                        sizeof(ckeymat3))))
+            || (tst >= 3 && !TEST_mem_eq(ckeymat2, sizeof(ckeymat2), ckeymat3,
+                                         sizeof(ckeymat3))))
         goto end;
 
     testresult = 1;
@@ -5909,7 +5933,7 @@ int setup_tests(void)
     ADD_ALL_TESTS(test_custom_exts, 3);
 #endif
     ADD_ALL_TESTS(test_serverinfo, 8);
-    ADD_ALL_TESTS(test_export_key_mat, 4);
+    ADD_ALL_TESTS(test_export_key_mat, 6);
 #ifndef OPENSSL_NO_TLS1_3
     ADD_ALL_TESTS(test_export_key_mat_early, 3);
 #endif
diff --git a/test/tls13secretstest.c b/test/tls13secretstest.c
index 319df17bab..de318df02b 100644
--- a/test/tls13secretstest.c
+++ b/test/tls13secretstest.c
@@ -226,7 +226,7 @@ static int test_secret(SSL *s, unsigned char *prk,
     }
 
     if (!tls13_hkdf_expand(s, md, prk, label, labellen, hash, hashsize,
-                           gensecret, hashsize)) {
+                           gensecret, hashsize, 1)) {
         TEST_error("Secret generation failed");
         return 0;
     }