The branch OpenSSL_1_1_1-stable has been updated via db860ea3dcf56a1993c66da22bd44460d7ac4914 (commit) via 56e0f123dc17cb99f50efbae4bbbab77f360818f (commit) from 8df98cd98812dff67aa93a3f760860ac91f7f24a (commit)
- Log ----------------------------------------------------------------- commit db860ea3dcf56a1993c66da22bd44460d7ac4914 Author: Matt Caswell <m...@openssl.org> Date: Tue Dec 4 08:37:04 2018 +0000 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 <t...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/7755) (cherry picked from commit 0fb2815b873304d145ed00283454fc9f3bd35e6b) commit 56e0f123dc17cb99f50efbae4bbbab77f360818f Author: Matt Caswell <m...@openssl.org> Date: Mon Dec 3 18:14:57 2018 +0000 Revert "Reduce stack usage in tls13_hkdf_expand" This reverts commit ec0c5f5693e39c5a013f81e6dd9dfd09ec65162d. SSL_export_keying_material() may use longer label lengths. Fixes #7712 Reviewed-by: Tim Hudson <t...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/7755) (cherry picked from commit ed371b8cbac0d0349667558c061c1ae380cf75eb) ----------------------------------------------------------------------- Summary of changes: 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, 88 insertions(+), 46 deletions(-) diff --git a/doc/man3/SSL_export_keying_material.pod b/doc/man3/SSL_export_keying_material.pod index abebf91..4c81a60 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 70e5a17..307131d 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 63e61c6..716d6d2 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 5a8f116..a0e495d 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 e7c11c4..a8e862c 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 b6825d2..c3021d1 100644 --- a/ssl/tls13_enc.c +++ b/ssl/tls13_enc.c @@ -13,14 +13,7 @@ #include <openssl/evp.h> #include <openssl/kdf.h> -/* - * RFC 8446, 7.1 Key Schedule, says: - * Note: With common hash functions, any label longer than 12 characters - * requires an additional iteration of the hash function to compute. - * The labels in this specification have all been chosen to fit within - * this limit. - */ -#define TLS13_MAX_LABEL_LEN 12 +#define TLS13_MAX_LABEL_LEN 249 /* Always filled with zeros */ static const unsigned char default_zeros[EVP_MAX_MD_SIZE]; @@ -29,12 +22,13 @@ 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) { static const unsigned char label_prefix[] = "tls13 "; EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL); @@ -48,12 +42,27 @@ int tls13_hkdf_expand(SSL *s, const EVP_MD *md, const unsigned char *secret, */ 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) @@ -67,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; } @@ -82,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; } @@ -99,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); } /* @@ -112,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, @@ -122,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); } /* @@ -185,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; @@ -345,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; } @@ -525,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; @@ -612,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; } @@ -632,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; } @@ -746,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; @@ -805,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 108d57e..a4bbb4f 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 724c170..66a0582 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; } _____ openssl-commits mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-commits