From: Matt Caswell Date: Mon, 26 Feb 2018 12:26:14 +0000 (+0000) Subject: Use the TLSv1.3 record header as AAD X-Git-Tag: OpenSSL_1_1_1-pre3~103 X-Git-Url: https://git.openssl.org/?p=openssl.git;a=commitdiff_plain;h=3295d2423889496e0933b3f9af6dc692c9f9a8f2;hp=95ea8da1768bf457b021f07cde9a6330827dc8a1 Use the TLSv1.3 record header as AAD As of TLSv1.3 draft-25 the record header data must be used as AAD Reviewed-by: Ben Kaduk (Merged from https://github.com/openssl/openssl/pull/5604) --- diff --git a/engines/e_ossltest.c b/engines/e_ossltest.c index 2bda610f46..64376247c3 100644 --- a/engines/e_ossltest.c +++ b/engines/e_ossltest.c @@ -637,7 +637,7 @@ int ossltest_aes128_gcm_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out, EVP_CIPHER_meth_get_do_cipher(EVP_aes_128_gcm())(ctx, out, in, inl); /* Throw it all away and just use the plaintext as the output */ - if (tmpbuf != NULL) + if (tmpbuf != NULL && out != NULL) memcpy(out, tmpbuf, inl); OPENSSL_free(tmpbuf); diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 0953d2b01d..61010f4e72 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -825,7 +825,6 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf, thispkt = &pkt[j]; thiswr = &wr[j]; - SSL3_RECORD_set_type(thiswr, type); /* * In TLSv1.3, once encrypting, we always use application data for the * record type @@ -834,6 +833,8 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf, rectype = SSL3_RT_APPLICATION_DATA; else rectype = type; + SSL3_RECORD_set_type(thiswr, rectype); + /* * Some servers hang if initial client hello is larger than 256 bytes * and record version number > TLS 1.0 @@ -843,6 +844,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf, && TLS1_get_version(s) > TLS1_VERSION && s->hello_retry_request == SSL_HRR_NONE) version = TLS1_VERSION; + SSL3_RECORD_set_rec_version(thiswr, version); maxcomplen = pipelens[j]; if (s->compress != NULL) diff --git a/ssl/record/record_locl.h b/ssl/record/record_locl.h index c20f5fec15..1782a4fa5b 100644 --- a/ssl/record/record_locl.h +++ b/ssl/record/record_locl.h @@ -80,6 +80,7 @@ int ssl3_release_write_buffer(SSL *s); #define SSL3_RECORD_get_type(r) ((r)->type) #define SSL3_RECORD_set_type(r, t) ((r)->type = (t)) +#define SSL3_RECORD_set_rec_version(r, v) ((r)->rec_version = (v)) #define SSL3_RECORD_get_length(r) ((r)->length) #define SSL3_RECORD_set_length(r, l) ((r)->length = (l)) #define SSL3_RECORD_add_length(r, l) ((r)->length += (l)) diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index fda918a9d8..5bfbaf982e 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -270,7 +270,8 @@ int ssl3_get_record(SSL *s) thisrr->rec_version = version; /* - * Lets check version. In TLSv1.3 we ignore this field. For the + * Lets check version. In TLSv1.3 we only check this field + * when encryption is occurring (see later check). For the * ServerHello after an HRR we haven't actually selected TLSv1.3 * yet, but we still treat it as TLSv1.3, so we must check for * that explicitly @@ -333,14 +334,19 @@ int ssl3_get_record(SSL *s) } } - if (SSL_IS_TLS13(s) - && s->enc_read_ctx != NULL - && thisrr->type != SSL3_RT_APPLICATION_DATA - && (thisrr->type != SSL3_RT_CHANGE_CIPHER_SPEC - || !SSL_IS_FIRST_HANDSHAKE(s))) { - SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, - SSL_F_SSL3_GET_RECORD, SSL_R_BAD_RECORD_TYPE); - return -1; + if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) { + if (thisrr->type != SSL3_RT_APPLICATION_DATA + && (thisrr->type != SSL3_RT_CHANGE_CIPHER_SPEC + || !SSL_IS_FIRST_HANDSHAKE(s))) { + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, + SSL_F_SSL3_GET_RECORD, SSL_R_BAD_RECORD_TYPE); + return -1; + } + if (thisrr->rec_version != TLS1_2_VERSION) { + SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_SSL3_GET_RECORD, + SSL_R_WRONG_VERSION_NUMBER); + return -1; + } } if (thisrr->length > diff --git a/ssl/record/ssl3_record_tls13.c b/ssl/record/ssl3_record_tls13.c index f1e1667b9d..21073b637d 100644 --- a/ssl/record/ssl3_record_tls13.c +++ b/ssl/record/ssl3_record_tls13.c @@ -25,13 +25,14 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending) { EVP_CIPHER_CTX *ctx; - unsigned char iv[EVP_MAX_IV_LENGTH]; - size_t ivlen, taglen, offset, loop; + unsigned char iv[EVP_MAX_IV_LENGTH], recheader[SSL3_RT_HEADER_LENGTH]; + size_t ivlen, taglen, offset, loop, hdrlen; unsigned char *staticiv; unsigned char *seq; int lenu, lenf; SSL3_RECORD *rec = &recs[0]; uint32_t alg_enc; + WPACKET wpkt; if (n_recs != 1) { /* Should not happen */ @@ -143,7 +144,31 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending) if (EVP_CipherInit_ex(ctx, NULL, NULL, NULL, iv, sending) <= 0 || (!sending && EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, taglen, - rec->data + rec->length) <= 0) + rec->data + rec->length) <= 0)) { + return -1; + } + + /* Set up the AAD */ + if (!WPACKET_init_static_len(&wpkt, recheader, sizeof(recheader), 0) + || !WPACKET_put_bytes_u8(&wpkt, rec->type) + || !WPACKET_put_bytes_u16(&wpkt, rec->rec_version) + || !WPACKET_put_bytes_u16(&wpkt, rec->length + taglen) + || !WPACKET_get_total_written(&wpkt, &hdrlen) + || hdrlen != SSL3_RT_HEADER_LENGTH + || !WPACKET_finish(&wpkt)) { + WPACKET_cleanup(&wpkt); + return -1; + } + + /* + * For CCM we must explicitly set the total plaintext length before we add + * any AAD. + */ + if (((alg_enc & SSL_AESCCM) != 0 + && EVP_CipherUpdate(ctx, NULL, &lenu, NULL, + (unsigned int)rec->length) <= 0) + || EVP_CipherUpdate(ctx, NULL, &lenu, recheader, + sizeof(recheader)) <= 0 || EVP_CipherUpdate(ctx, rec->data, &lenu, rec->input, (unsigned int)rec->length) <= 0 || EVP_CipherFinal_ex(ctx, rec->data + lenu, &lenf) <= 0 diff --git a/test/recipes/70-test_sslrecords.t b/test/recipes/70-test_sslrecords.t index 10c559e6ba..88cb0223b6 100644 --- a/test/recipes/70-test_sslrecords.t +++ b/test/recipes/70-test_sslrecords.t @@ -149,11 +149,11 @@ ok(TLSProxy::Message->fail(), "Changed record version in TLS1.2"); SKIP: { skip "TLSv1.3 disabled", 6 if disabled("tls1_3"); - #Test 13: Sending a different record version in TLS1.3 should succeed + #Test 13: Sending a different record version in TLS1.3 should fail $proxy->clear(); $proxy->filter(\&change_version); $proxy->start(); - ok(TLSProxy::Message->success(), "Changed record version in TLS1.3"); + ok(TLSProxy::Message->fail(), "Changed record version in TLS1.3"); #Test 14: Sending an unrecognised record type in TLS1.3 should fail $proxy->clear(); @@ -464,7 +464,7 @@ sub change_version my $proxy = shift; # We'll change a version after the initial version neg has taken place - if ($proxy->flight != 2) { + if ($proxy->flight != 1) { return; } diff --git a/test/recordlentest.c b/test/recordlentest.c index d0941ca416..824c09fc34 100644 --- a/test/recordlentest.c +++ b/test/recordlentest.c @@ -157,11 +157,7 @@ static int test_record_overflow(int idx) overf_expected = 0; } - if (idx == TEST_ENCRYPTED_OVERFLOW_TLS1_3_OK - || idx == TEST_ENCRYPTED_OVERFLOW_TLS1_3_NOT_OK) - recversion = TLS1_VERSION; - else - recversion = TLS1_2_VERSION; + recversion = TLS1_2_VERSION; if (!TEST_true(write_record(serverbio, len, SSL3_RT_APPLICATION_DATA, recversion))) diff --git a/test/tls13encryptiontest.c b/test/tls13encryptiontest.c index 6e4b88985f..4d6595a38a 100644 --- a/test/tls13encryptiontest.c +++ b/test/tls13encryptiontest.c @@ -92,7 +92,7 @@ static RECORD_DATA refdata[] = { "83dd29f64508b2ec3e635a2134fc0e1a39d3ecb51dcddfcf8382c88ffe2a7378" "42ad1de7fe505b6c4d1673870f6fc2a0f2f7972acaee368a1599d64ba18798f1" "0333f9779bd5b05f9b084d03dab2f3d80c2eb74ec70c9866ea31c18b491cd597" - "aae3e941205fcc38a3a10ce8c0269f02ccc9c51278e25f1a0f0731a9" + "aae3e941205fcc38a3a10ce8f2e230d97e3406b77ee53d84d89ca548" }, "d2dd45f87ad87801a85ac38187f9023b", "f0a14f808692cef87a3daf70", @@ -106,7 +106,7 @@ static RECORD_DATA refdata[] = { }, { "fa15e92daa21cd05d8f9c3152a61748d9aaf049da559718e583f95aacecad657" - "b52a6562da09a5819e864d86ac2989360a1eb22795","","" + "b52a6562da66864fd14969acc30dc04a78c38283c5","","" }, "40e1201d75d419627f04c88530a15c9d", "a0f073f3b35e18f96969696b", @@ -128,7 +128,7 @@ static RECORD_DATA refdata[] = { "836905229eac811c4ef8b2faa89867e9ffc586f7f03c216591aa5e620eac3c62" "dfe60f846036bd7ecc4464b584af184e9644e94ee1d7834dba408a51cbe42480" "04796ed9c558e0f5f96115a6f6ba487e17d16a2e20a3d3a650a9a070fb53d9da" - "82864b5621d77650bd0c7947e9889917b53d0515627c72b0ded521","","" + "82864b5621d77650bd0c7972f592aa8546de09b8e46921fab4d876","","" }, "3381f6b3f94500f16226de440193e858", "4f1d73cc1d465eb30021c41f", @@ -142,8 +142,8 @@ static RECORD_DATA refdata[] = { }, { "e306178ad97f74bb64f35eaf3c39846b83aef8472cbc9046749b81a949dfb12c" - "fbc65cbabd20ade92c1f944605892ceeb12fdee8a927bce77c83036ac5a794a8" - "f54a69","","" + "fbc65cbabd20ade92c1f944605892ceeb12fde5781d40e2ca080fc921b750b8c" + "21bd8d","","" }, "eb23a804904b80ba4fe8399e09b1ce42", "efa8c50c06b9c9b8c483e174", @@ -157,8 +157,8 @@ static RECORD_DATA refdata[] = { }, { "467d99a807dbf778e6ffd8be52456c70665f890811ef2f3c495d5bbe983feeda" - "b0c251dde596bc7e2b135909ec9f9166fb0152e8c16a84e4b1039256467f9538" - "be4463","","" + "b0c251dde596bc7e2b135909ec9f9166fb01526c70c7e42b6df52d63b0000222" + "cb2047","","" }, "3381f6b3f94500f16226de440193e858", "4f1d73cc1d465eb30021c41f", @@ -170,7 +170,7 @@ static RECORD_DATA refdata[] = { "010015","","" }, { - "6bdf60847ba6fb650da36e872adc684a4af2e8","","" + "6bdf609107610cff95d70387a67b89e2494f0d","","" }, "eb23a804904b80ba4fe8399e09b1ce42", "efa8c50c06b9c9b8c483e174", @@ -182,7 +182,7 @@ static RECORD_DATA refdata[] = { "010015","","" }, { - "621b7cc1962cd8a70109fee68a52efedf87d2e","","" + "621b7c60d32528b149b36a78c8891a8d2f65ad","","" }, "3381f6b3f94500f16226de440193e858", "4f1d73cc1d465eb30021c41f", @@ -306,7 +306,13 @@ static int test_tls13_encryption(void) int ret = 0; size_t ivlen, ctr; + /* + * Encrypted TLSv1.3 records always have an outer content type of + * application data, and a record version of TLSv1.2. + */ rec.data = NULL; + rec.type = SSL3_RT_APPLICATION_DATA; + rec.rec_version = TLS1_2_VERSION; ctx = SSL_CTX_new(TLS_method()); if (!TEST_ptr(ctx)) {