From e23d5071ec4c7aa6bb2b0f2c3e0fc2182ed7e63f Mon Sep 17 00:00:00 2001 From: David Woodhouse Date: Wed, 12 Oct 2016 23:12:04 +0100 Subject: [PATCH] Fix encrypt-then-mac implementation for DTLS MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit OpenSSL 1.1.0 will negotiate EtM on DTLS but will then not actually *do* it. If we use DTLSv1.2 that will hopefully be harmless since we'll tend to use an AEAD ciphersuite anyway. But if we're using DTLSv1, then we certainly will end up using CBC, so EtM is relevant — and we fail to interoperate with anything that implements EtM correctly. Fixing it in HEAD and 1.1.0c will mean that 1.1.0[ab] are incompatible with 1.1.0c+... for the limited case of non-AEAD ciphers, where they're *already* incompatible with other implementations due to this bug anyway. That seems reasonable enough, so let's do it. The only alternative is just to turn it off for ever... which *still* leaves 1.0.0[ab] failing to communicate with non-OpenSSL implementations anyway. Tested against itself as well as against GnuTLS both with and without EtM. Reviewed-by: Tim Hudson Reviewed-by: Matt Caswell --- ssl/record/rec_layer_d1.c | 10 +++++++++- ssl/record/ssl3_record.c | 22 +++++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c index 1d16319f14..c9fd0669ed 100644 --- a/ssl/record/rec_layer_d1.c +++ b/ssl/record/rec_layer_d1.c @@ -1094,7 +1094,7 @@ int do_dtls1_write(SSL *s, int type, const unsigned char *buf, * wb->buf */ - if (mac_size != 0) { + if (!SSL_USE_ETM(s) && mac_size != 0) { if (s->method->ssl3_enc->mac(s, &wr, &(p[SSL3_RECORD_get_length(&wr) + eivlen]), 1) < 0) @@ -1112,6 +1112,14 @@ int do_dtls1_write(SSL *s, int type, const unsigned char *buf, if (s->method->ssl3_enc->enc(s, &wr, 1, 1) < 1) goto err; + if (SSL_USE_ETM(s) && mac_size != 0) { + if (s->method->ssl3_enc->mac(s, &wr, + &(p[SSL3_RECORD_get_length(&wr)]), + 1) < 0) + goto err; + SSL3_RECORD_add_length(&wr, mac_size); + } + /* record length after mac and block padding */ /* * if (type == SSL3_RT_APPLICATION_DATA || (type == SSL3_RT_ALERT && ! diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index 32a97aff08..32361666b6 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -1314,6 +1314,26 @@ int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap) rr->data = rr->input; rr->orig_len = rr->length; + if (SSL_USE_ETM(s) && s->read_hash) { + unsigned char *mac; + mac_size = EVP_MD_CTX_size(s->read_hash); + OPENSSL_assert(mac_size <= EVP_MAX_MD_SIZE); + if (rr->orig_len < mac_size) { + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_DTLS1_PROCESS_RECORD, SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + rr->length -= mac_size; + mac = rr->data + rr->length; + i = s->method->ssl3_enc->mac(s, rr, md, 0 /* not send */ ); + if (i < 0 || CRYPTO_memcmp(md, mac, (size_t)mac_size) != 0) { + al = SSL_AD_BAD_RECORD_MAC; + SSLerr(SSL_F_DTLS1_PROCESS_RECORD, + SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC); + goto f_err; + } + } + enc_err = s->method->ssl3_enc->enc(s, rr, 1, 0); /*- * enc_err is: @@ -1338,7 +1358,7 @@ int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap) #endif /* r->length is now the compressed data plus mac */ - if ((sess != NULL) && + if ((sess != NULL) && !SSL_USE_ETM(s) && (s->enc_read_ctx != NULL) && (EVP_MD_CTX_md(s->read_hash) != NULL)) { /* s->read_hash != NULL => mac_size != -1 */ unsigned char *mac = NULL; -- 2.25.1