From: David Woodhouse <David.Woodhouse@intel.com>
Date: Wed, 12 Oct 2016 22:12:04 +0000 (+0100)
Subject: Fix encrypt-then-mac implementation for DTLS
X-Git-Tag: OpenSSL_1_1_1-pre1~3365
X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=e23d5071ec4c7aa6bb2b0f2c3e0fc2182ed7e63f;p=oweals%2Fopenssl.git

Fix encrypt-then-mac implementation for DTLS

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 <tjh@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
---

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;