| From db860ea3dcf56a1993c66da22bd44460d7ac4914 Mon Sep 17 00:00:00 2001 |
| From: Matt Caswell <matt@openssl.org> |
| Date: Tue, 4 Dec 2018 08:37:04 +0000 |
| Subject: [PATCH] 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) |
| |
| (cherry picked from commit 0fb2815b873304d145ed00283454fc9f3bd35e6b) |
| Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de> |
| --- |
| doc/man3/SSL_export_keying_material.pod | 3 +- |
| ssl/ssl_locl.h | 2 +- |
| ssl/statem/extensions.c | 2 +- |
| ssl/statem/statem_clnt.c | 2 +- |
| ssl/statem/statem_srvr.c | 2 +- |
| ssl/tls13_enc.c | 73 +++++++++++++++++-------- |
| test/sslapitest.c | 48 ++++++++++++---- |
| test/tls13secretstest.c | 2 +- |
| 8 files changed, 92 insertions(+), 42 deletions(-) |
| |
| 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 724c170c56..66a0582887 100644 |
| --- a/test/tls13secretstest.c |
| +++ b/test/tls13secretstest.c |
| @@ -236,7 +236,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; |
| } |
| -- |
| 2.20.1 |
| |
