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 1d16319f1485cf2756897001535821f74d697ac6..c9fd0669edfb76851e3f95761ec4642b4134c758 100644 (file)
--- 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 32a97aff0826b00fba3d82c14f12c63df4469213..32361666b6d618ab4bc0143fc5966ec4367f7afc 100644 (file)
--- 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;