Don't change the state of the ETM flags until CCS processing

In 1.1.0 changing the ciphersuite during a renegotiation can result in
a crash leading to a DoS attack. In master this does not occur with TLS
(instead you get an internal error, which is still wrong but not a security
issue) - but the problem still exists in the DTLS code.

The problem is caused by changing the flag indicating whether to use ETM
or not immediately on negotiation of ETM, rather than at CCS. Therefore,
during a renegotiation, if the ETM state is changing (usually due to a
change of ciphersuite), then an error/crash will occur.

Due to the fact that there are separate CCS messages for read and write
we actually now need two flags to determine whether to use ETM or not.

CVE-2017-3733

Reviewed-by: Richard Levitte <levitte@openssl.org>

diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index d76236ab8dbd526783fd2c77de6366a71384f4f8..f2f62b4f1abc39bafbc0588206236460e6a1cded 100644 (file)
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -265,11 +265,14 @@ extern "C" {
 # define TLS1_FLAGS_SKIP_CERT_VERIFY             0x0010
 
 /* Set if we encrypt then mac instead of usual mac then encrypt */
-# define TLS1_FLAGS_ENCRYPT_THEN_MAC             0x0100
+# define TLS1_FLAGS_ENCRYPT_THEN_MAC_READ        0x0100
+# define TLS1_FLAGS_ENCRYPT_THEN_MAC             TLS1_FLAGS_ENCRYPT_THEN_MAC_READ
 
 /* Set if extended master secret extension received from peer */
 # define TLS1_FLAGS_RECEIVED_EXTMS               0x0200
 
+# define TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE       0x0400
+
 # define SSL3_MT_HELLO_REQUEST                   0
 # define SSL3_MT_CLIENT_HELLO                    1
 # define SSL3_MT_SERVER_HELLO                    2

diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index c1d160ecd4306997ac41e45534e9bcb87742c8b1..213fad5a8ded074730b8be42ca6da5180b83fee0 100644 (file)
--- a/ssl/d1_lib.c
+++ b/ssl/d1_lib.c
@@ -937,7 +937,7 @@ size_t DTLS_get_data_mtu(const SSL *s)
                                  &blocksize, &ext_overhead))
         return 0;
 
-    if (SSL_USE_ETM(s))
+    if (SSL_READ_ETM(s))
         ext_overhead += mac_overhead;
     else
         int_overhead += mac_overhead;

diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c
index 67846bd19f62ed833358c8756d4b1af1e9422367..dc106732f5417c5f13abfd669d482a14540b2752 100644 (file)
--- a/ssl/record/rec_layer_d1.c
+++ b/ssl/record/rec_layer_d1.c
@@ -1029,7 +1029,7 @@ int do_dtls1_write(SSL *s, int type, const unsigned char *buf,
      * wb->buf
      */
 
-    if (!SSL_USE_ETM(s) && mac_size != 0) {
+    if (!SSL_WRITE_ETM(s) && mac_size != 0) {
         if (!s->method->ssl3_enc->mac(s, &wr,
                                       &(p[SSL3_RECORD_get_length(&wr) + eivlen]),
                                       1))
@@ -1047,7 +1047,7 @@ 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 (SSL_WRITE_ETM(s) && mac_size != 0) {
         if (!s->method->ssl3_enc->mac(s, &wr,
                                       &(p[SSL3_RECORD_get_length(&wr)]), 1))
             goto err;

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 58a4716ffe8105ba377345a4749fe36428c2625f..0da91b1c629f5937da933985b1b557dcb561989f 100644 (file)
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -401,7 +401,7 @@ int ssl3_write_bytes(SSL *s, int type, const void *buf_, size_t len,
     if (type == SSL3_RT_APPLICATION_DATA &&
         len >= 4 * (max_send_fragment = s->max_send_fragment) &&
         s->compress == NULL && s->msg_callback == NULL &&
-        !SSL_USE_ETM(s) && SSL_USE_EXPLICIT_IV(s) &&
+        !SSL_WRITE_ETM(s) && SSL_USE_EXPLICIT_IV(s) &&
         EVP_CIPHER_flags(EVP_CIPHER_CTX_cipher(s->enc_write_ctx)) &
         EVP_CIPH_FLAG_TLS1_1_MULTIBLOCK) {
         unsigned char aad[13];
@@ -870,7 +870,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
          * in the wb->buf
          */
 
-        if (!SSL_USE_ETM(s) && mac_size != 0) {
+        if (!SSL_WRITE_ETM(s) && mac_size != 0) {
             unsigned char *mac;
 
             if (!WPACKET_allocate_bytes(thispkt, mac_size, &mac)
@@ -923,7 +923,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
             SSLerr(SSL_F_DO_SSL3_WRITE, ERR_R_INTERNAL_ERROR);
             goto err;
         }
-        if (SSL_USE_ETM(s) && mac_size != 0) {
+        if (SSL_WRITE_ETM(s) && mac_size != 0) {
             unsigned char *mac;
 
             if (!WPACKET_allocate_bytes(thispkt, mac_size, &mac)

diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c
index fc4723685c8b05aa55414cf8879fa9da5899706a..9e99210d899f2eb3cbbfaca5ea477c04d8bdcc5e 100644 (file)
--- a/ssl/record/ssl3_record.c
+++ b/ssl/record/ssl3_record.c
@@ -383,7 +383,7 @@ int ssl3_get_record(SSL *s)
      * If in encrypt-then-mac mode calculate mac from encrypted record. All
      * the details below are public so no timing details can leak.
      */
-    if (SSL_USE_ETM(s) && s->read_hash) {
+    if (SSL_READ_ETM(s) && s->read_hash) {
         unsigned char *mac;
         /* TODO(size_t): convert this to do size_t properly */
         imac_size = EVP_MD_CTX_size(s->read_hash);
@@ -440,7 +440,7 @@ int ssl3_get_record(SSL *s)
     /* r->length is now the compressed data plus mac */
     if ((sess != NULL) &&
         (s->enc_read_ctx != NULL) &&
-        (EVP_MD_CTX_md(s->read_hash) != NULL) && !SSL_USE_ETM(s)) {
+        (!SSL_READ_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL)) {
         /* s->read_hash != NULL => mac_size != -1 */
         unsigned char *mac = NULL;
         unsigned char mac_tmp[EVP_MAX_MD_SIZE];
@@ -915,7 +915,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int send)
         }
 
         ret = 1;
-        if (!SSL_USE_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL) {
+        if (!SSL_READ_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL) {
             imac_size = EVP_MD_CTX_size(s->read_hash);
             if (imac_size < 0)
                 return -1;
@@ -1092,7 +1092,7 @@ int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int send)
     header[11] = (unsigned char)(rec->length >> 8);
     header[12] = (unsigned char)(rec->length & 0xff);
 
-    if (!send && !SSL_USE_ETM(ssl) &&
+    if (!send && !SSL_READ_ETM(ssl) &&
         EVP_CIPHER_CTX_mode(ssl->enc_read_ctx) == EVP_CIPH_CBC_MODE &&
         ssl3_cbc_record_digest_supported(mac_ctx)) {
         /*
@@ -1118,7 +1118,7 @@ int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int send)
             EVP_MD_CTX_free(hmac);
             return 0;
         }
-        if (!send && !SSL_USE_ETM(ssl) && FIPS_mode())
+        if (!send && !SSL_READ_ETM(ssl) && FIPS_mode())
             if (!tls_fips_digest_extra(ssl->enc_read_ctx,
                                        mac_ctx, rec->input,
                                        rec->length, rec->orig_len)) {
@@ -1408,7 +1408,7 @@ 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) {
+    if (SSL_READ_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);
@@ -1452,7 +1452,7 @@ int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap)
 #endif
 
     /* r->length is now the compressed data plus mac */
-    if ((sess != NULL) && !SSL_USE_ETM(s) &&
+    if ((sess != NULL) && !SSL_READ_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;

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 106ff69492b43f7f9a4f09d0890b061d40f8e2cb..e1bce30a20de1267232aeb0e99bb60eb4c817710 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -395,7 +395,8 @@
 # define SSL_CLIENT_USE_SIGALGS(s)        \
     SSL_CLIENT_USE_TLS1_2_CIPHERS(s)
 
-# define SSL_USE_ETM(s) (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC)
+# define SSL_READ_ETM(s) (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC_READ)
+# define SSL_WRITE_ETM(s) (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE)
 
 /* Mostly for SSLv3 */
 # define SSL_PKEY_RSA            0
@@ -1132,6 +1133,9 @@ struct ssl_st {
 
         /* The available PSK key exchange modes */
         int psk_kex_mode;
+
+        /* Set to one if we have negotiated ETM */
+        int use_etm;
     } ext;
 
     /*-

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index edb674d7a83e8db6af3fcf464ba8f6bf439c349b..8e1b502083f9627f7436ab0c0702a54ac42a35fa 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -207,7 +207,7 @@ static const EXTENSION_DEFINITION ext_defs[] = {
 #endif
     {
         TLSEXT_TYPE_encrypt_then_mac,
-        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_2_AND_BELOW_ONLY,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_2_AND_BELOW_ONLY | EXT_SSL3_ALLOWED,
         init_etm, tls_parse_ctos_etm, tls_parse_stoc_etm,
         tls_construct_stoc_etm, tls_construct_ctos_etm, NULL
     },
@@ -912,7 +912,7 @@ static int init_srp(SSL *s, unsigned int context)
 
 static int init_etm(SSL *s, unsigned int context)
 {
-    s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
+    s->ext.use_etm = 0;
 
     return 1;
 }

diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index ea379199e04019a46c938c4a52ea2c4236b7ed83..09780a949548136ff58ac861cbcaf676a20aa6b5 100644 (file)
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -1172,7 +1172,7 @@ int tls_parse_stoc_etm(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
     if (!(s->options & SSL_OP_NO_ENCRYPT_THEN_MAC)
             && s->s3->tmp.new_cipher->algorithm_mac != SSL_AEAD
             && s->s3->tmp.new_cipher->algorithm_enc != SSL_RC4)
-        s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC;
+        s->ext.use_etm = 1;
 
     return 1;
 }

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index b555d68d8d6607426078bcdb20a73adeec90bef5..ecfd00b098da489e27342620447565a833c69e97 100644 (file)
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -451,7 +451,7 @@ int tls_parse_ctos_etm(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
                        size_t chainidx, int *al)
 {
     if (!(s->options & SSL_OP_NO_ENCRYPT_THEN_MAC))
-        s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC;
+        s->ext.use_etm = 1;
 
     return 1;
 }
@@ -953,7 +953,7 @@ int tls_construct_stoc_use_srtp(SSL *s, WPACKET *pkt, unsigned int context,
 int tls_construct_stoc_etm(SSL *s, WPACKET *pkt, unsigned int context, X509 *x,
                            size_t chainidx, int *al)
 {
-    if ((s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC) == 0)
+    if (!s->ext.use_etm)
         return 1;
 
     /*
@@ -964,7 +964,7 @@ int tls_construct_stoc_etm(SSL *s, WPACKET *pkt, unsigned int context, X509 *x,
         || s->s3->tmp.new_cipher->algorithm_enc == SSL_RC4
         || s->s3->tmp.new_cipher->algorithm_enc == SSL_eGOST2814789CNT
         || s->s3->tmp.new_cipher->algorithm_enc == SSL_eGOST2814789CNT12) {
-        s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
+        s->ext.use_etm = 0;
         return 1;
     }
 

diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c
index ebdc0fbd5296e2f74653a27f1a554fd831cf62c3..4158548568d657283a7abd60f458f3a5f2e0a334 100644 (file)
--- a/ssl/t1_enc.c
+++ b/ssl/t1_enc.c
@@ -129,6 +129,11 @@ int tls1_change_cipher_state(SSL *s, int which)
 #endif
 
     if (which & SSL3_CC_READ) {
+        if (s->ext.use_etm)
+            s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC_READ;
+        else
+            s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC_READ;
+
         if (s->s3->tmp.new_cipher->algorithm2 & TLS1_STREAM_MAC)
             s->mac_flags |= SSL_MAC_FLAG_READ_MAC_STREAM;
         else
@@ -167,6 +172,11 @@ int tls1_change_cipher_state(SSL *s, int which)
         mac_secret = &(s->s3->read_mac_secret[0]);
         mac_secret_size = &(s->s3->read_mac_secret_size);
     } else {
+        if (s->ext.use_etm)
+            s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE;
+        else
+            s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE;
+
         if (s->s3->tmp.new_cipher->algorithm2 & TLS1_STREAM_MAC)
             s->mac_flags |= SSL_MAC_FLAG_WRITE_MAC_STREAM;
         else
@@ -369,9 +379,8 @@ int tls1_setup_key_block(SSL *s)
     if (s->s3->tmp.key_block_length != 0)
         return (1);
 
-    if (!ssl_cipher_get_evp
-        (s->session, &c, &hash, &mac_type, &mac_secret_size, &comp,
-         SSL_USE_ETM(s))) {
+    if (!ssl_cipher_get_evp(s->session, &c, &hash, &mac_type, &mac_secret_size,
+                            &comp, s->ext.use_etm)) {
         SSLerr(SSL_F_TLS1_SETUP_KEY_BLOCK, SSL_R_CIPHER_OR_HASH_UNAVAILABLE);
         return (0);
     }

diff --git a/test/dtls_mtu_test.c b/test/dtls_mtu_test.c
index da970a7fa7151d508836bcfbe15612f9efb43784..1a05c541f7d66efb04970c696da51ef45945b763 100644 (file)
--- a/test/dtls_mtu_test.c
+++ b/test/dtls_mtu_test.c
@@ -16,7 +16,7 @@
 
 #include "ssltestlib.h"
 
-/* for SSL_USE_ETM() */
+/* for SSL_READ_ETM() */
 #include "../ssl/ssl_locl.h"
 
 static int debug = 0;
@@ -133,7 +133,7 @@ static int mtu_test(SSL_CTX *ctx, const char *cs, int no_etm)
         }
     }
     rv = 1;
-    if (SSL_USE_ETM(clnt_ssl))
+    if (SSL_READ_ETM(clnt_ssl))
         rv = 2;
  out:
     SSL_free(clnt_ssl);