Don't allow fragmented alerts
authorMatt Caswell <matt@openssl.org>
Mon, 15 May 2017 10:24:24 +0000 (11:24 +0100)
committerMatt Caswell <matt@openssl.org>
Wed, 17 May 2017 09:40:04 +0000 (10:40 +0100)
An alert message is 2 bytes long. In theory it is permissible in SSLv3 -
TLSv1.2 to fragment such alerts across multiple records (some of which
could be empty). In practice it make no sense to send an empty alert
record, or to fragment one. TLSv1.3 prohibts this altogether and other
libraries (BoringSSL, NSS) do not support this at all. Supporting it adds
significant complexity to the record layer, and its removal is unlikely
to cause inter-operability issues.

The DTLS code for this never worked anyway and it is not supported at a
protocol level for DTLS. Similarly fragmented DTLS handshake records only
work at a protocol level where at least the handshake message header
exists within the record. DTLS code existed for trying to handle fragmented
handshake records smaller than this size. This code didn't work either so
has also been removed.

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3476)

CHANGES
ssl/record/rec_layer_d1.c
ssl/record/rec_layer_s3.c
ssl/record/record.h
test/recipes/70-test_sslrecords.t

diff --git a/CHANGES b/CHANGES
index 7bd0f92..9e4271d 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,16 @@
 
  Changes between 1.1.0e and 1.1.1 [xx XXX xxxx]
 
 
  Changes between 1.1.0e and 1.1.1 [xx XXX xxxx]
 
+  *) Fragmented SSL/TLS alerts are no longer accepted. An alert message is 2
+     bytes long. In theory it is permissible in SSLv3 - TLSv1.2 to fragment such
+     alerts across multiple records (some of which could be empty). In practice
+     it make no sense to send an empty alert record, or to fragment one. TLSv1.3
+     prohibts this altogether and other libraries (BoringSSL, NSS) do not
+     support this at all. Supporting it adds significant complexity to the
+     record layer, and its removal is unlikely to cause inter-operability
+     issues.
+     [Matt Caswell]
+
   *) Add the ASN.1 types INT32, UINT32, INT64, UINT64 and variants prefixed
      with Z.  These are meant to replace LONG and ZLONG and to be size safe.
      The use of LONG and ZLONG is discouraged and scheduled for deprecation
   *) Add the ASN.1 types INT32, UINT32, INT64, UINT64 and variants prefixed
      with Z.  These are meant to replace LONG and ZLONG and to be size safe.
      The use of LONG and ZLONG is discouraged and scheduled for deprecation
index 243eff7..487b096 100644 (file)
@@ -15,6 +15,7 @@
 #include <openssl/buffer.h>
 #include "record_locl.h"
 #include <assert.h>
 #include <openssl/buffer.h>
 #include "record_locl.h"
 #include <assert.h>
+#include "../packet_locl.h"
 
 int DTLS_RECORD_LAYER_new(RECORD_LAYER *rl)
 {
 
 int DTLS_RECORD_LAYER_new(RECORD_LAYER *rl)
 {
@@ -114,9 +115,6 @@ void DTLS_RECORD_LAYER_set_write_sequence(RECORD_LAYER *rl, unsigned char *seq)
     memcpy(rl->write_sequence, seq, SEQ_NUM_SIZE);
 }
 
     memcpy(rl->write_sequence, seq, SEQ_NUM_SIZE);
 }
 
-static size_t have_handshake_fragment(SSL *s, int type, unsigned char *buf,
-                                      size_t len);
-
 /* copy buffered record into SSL structure */
 static int dtls1_copy_record(SSL *s, pitem *item)
 {
 /* copy buffered record into SSL structure */
 static int dtls1_copy_record(SSL *s, pitem *item)
 {
@@ -335,7 +333,7 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
                      size_t len, int peek, size_t *readbytes)
 {
     int al, i, j, iret;
                      size_t len, int peek, size_t *readbytes)
 {
     int al, i, j, iret;
-    size_t ret, n;
+    size_t n;
     SSL3_RECORD *rr;
     void (*cb) (const SSL *ssl, int type2, int val) = NULL;
 
     SSL3_RECORD *rr;
     void (*cb) (const SSL *ssl, int type2, int val) = NULL;
 
@@ -352,21 +350,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
         return -1;
     }
 
         return -1;
     }
 
-    /*
-     * check whether there's a handshake message (client hello?) waiting
-     */
-    ret = have_handshake_fragment(s, type, buf, len);
-    if (ret > 0) {
-        *recvd_type = SSL3_RT_HANDSHAKE;
-        *readbytes = ret;
-        return 1;
-    }
-
-    /*
-     * Now s->rlayer.d->handshake_fragment_len == 0 if
-     * type == SSL3_RT_HANDSHAKE.
-     */
-
     if (!ossl_statem_get_in_handshake(s) && SSL_in_init(s))
     {
         /* type == SSL3_RT_APPLICATION_DATA */
     if (!ossl_statem_get_in_handshake(s) && SSL_in_init(s))
     {
         /* type == SSL3_RT_APPLICATION_DATA */
@@ -530,82 +513,23 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
      * then it was unexpected (Hello Request or Client Hello).
      */
 
      * then it was unexpected (Hello Request or Client Hello).
      */
 
-    /*
-     * In case of record types for which we have 'fragment' storage, fill
-     * that so that we can process the data at a fixed place.
-     */
-    {
-        size_t k, dest_maxlen = 0;
-        unsigned char *dest = NULL;
-        size_t *dest_len = NULL;
-
-        if (SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) {
-            dest_maxlen = sizeof s->rlayer.d->handshake_fragment;
-            dest = s->rlayer.d->handshake_fragment;
-            dest_len = &s->rlayer.d->handshake_fragment_len;
-        } else if (SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
-            dest_maxlen = sizeof(s->rlayer.d->alert_fragment);
-            dest = s->rlayer.d->alert_fragment;
-            dest_len = &s->rlayer.d->alert_fragment_len;
-        }
-        /* else it's a CCS message, or application data or wrong */
-        else if (SSL3_RECORD_get_type(rr) != SSL3_RT_CHANGE_CIPHER_SPEC) {
-            /*
-             * Application data while renegotiating is allowed. Try again
-             * reading.
-             */
-            if (SSL3_RECORD_get_type(rr) == SSL3_RT_APPLICATION_DATA) {
-                BIO *bio;
-                s->s3->in_read_app_data = 2;
-                bio = SSL_get_rbio(s);
-                s->rwstate = SSL_READING;
-                BIO_clear_retry_flags(bio);
-                BIO_set_retry_read(bio);
-                return -1;
-            }
+    if (SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
+        unsigned int alert_level, alert_descr;
+        unsigned char *alert_bytes = SSL3_RECORD_get_data(rr)
+                                     + SSL3_RECORD_get_off(rr);
+        PACKET alert;
 
 
-            /* Not certain if this is the right error handling */
+        if (!PACKET_buf_init(&alert, alert_bytes, SSL3_RECORD_get_length(rr))
+                || !PACKET_get_1(&alert, &alert_level)
+                || !PACKET_get_1(&alert, &alert_descr)
+                || PACKET_remaining(&alert) != 0) {
             al = SSL_AD_UNEXPECTED_MESSAGE;
             al = SSL_AD_UNEXPECTED_MESSAGE;
-            SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_UNEXPECTED_RECORD);
+            SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_INVALID_ALERT);
             goto f_err;
         }
 
             goto f_err;
         }
 
-        if (dest_maxlen > 0) {
-            /*
-             * XDTLS: In a pathological case, the Client Hello may be
-             * fragmented--don't always expect dest_maxlen bytes
-             */
-            if (SSL3_RECORD_get_length(rr) < dest_maxlen) {
-                s->rlayer.rstate = SSL_ST_READ_HEADER;
-                SSL3_RECORD_set_length(rr, 0);
-                goto start;
-            }
-
-            /* now move 'n' bytes: */
-            for (k = 0; k < dest_maxlen; k++) {
-                dest[k] = SSL3_RECORD_get_data(rr)[SSL3_RECORD_get_off(rr)];
-                SSL3_RECORD_add_off(rr, 1);
-                SSL3_RECORD_add_length(rr, -1);
-            }
-            *dest_len = dest_maxlen;
-        }
-    }
-
-    /*-
-     * s->rlayer.d->handshake_fragment_len == 12  iff  rr->type == SSL3_RT_HANDSHAKE;
-     * s->rlayer.d->alert_fragment_len == 7      iff  rr->type == SSL3_RT_ALERT.
-     * (Possibly rr is 'empty' now, i.e. rr->length may be 0.)
-     */
-
-    if (s->rlayer.d->alert_fragment_len >= DTLS1_AL_HEADER_LENGTH) {
-        int alert_level = s->rlayer.d->alert_fragment[0];
-        int alert_descr = s->rlayer.d->alert_fragment[1];
-
-        s->rlayer.d->alert_fragment_len = 0;
-
         if (s->msg_callback)
         if (s->msg_callback)
-            s->msg_callback(0, s->version, SSL3_RT_ALERT,
-                            s->rlayer.d->alert_fragment, 2, s,
+            s->msg_callback(0, s->version, SSL3_RT_ALERT, alert_bytes, 2, s,
                             s->msg_callback_arg);
 
         if (s->info_callback != NULL)
                             s->msg_callback_arg);
 
         if (s->info_callback != NULL)
@@ -686,17 +610,22 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
     /*
      * Unexpected handshake message (Client Hello, or protocol violation)
      */
     /*
      * Unexpected handshake message (Client Hello, or protocol violation)
      */
-    if ((s->rlayer.d->handshake_fragment_len >= DTLS1_HM_HEADER_LENGTH) &&
-        !ossl_statem_get_in_handshake(s)) {
+    if ((SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) &&
+            !ossl_statem_get_in_handshake(s)) {
         struct hm_header_st msg_hdr;
 
         struct hm_header_st msg_hdr;
 
-        /* this may just be a stale retransmit */
-        dtls1_get_message_header(rr->data, &msg_hdr);
-        if (SSL3_RECORD_get_epoch(rr) != s->rlayer.d->r_epoch) {
+        /*
+         * This may just be a stale retransmit. Also sanity check that we have
+         * at least enough record bytes for a message header
+         */
+        if (SSL3_RECORD_get_epoch(rr) != s->rlayer.d->r_epoch
+                || SSL3_RECORD_get_length(rr) < DTLS1_HM_HEADER_LENGTH) {
             SSL3_RECORD_set_length(rr, 0);
             goto start;
         }
 
             SSL3_RECORD_set_length(rr, 0);
             goto start;
         }
 
+        dtls1_get_message_header(rr->data, &msg_hdr);
+
         /*
          * If we are server, we may have a repeated FINISHED of the client
          * here, then retransmit our CCS and FINISHED.
         /*
          * If we are server, we may have a repeated FINISHED of the client
          * here, then retransmit our CCS and FINISHED.
@@ -756,11 +685,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
 
     switch (SSL3_RECORD_get_type(rr)) {
     default:
 
     switch (SSL3_RECORD_get_type(rr)) {
     default:
-        /* TLS just ignores unknown message types */
-        if (s->version == TLS1_VERSION) {
-            SSL3_RECORD_set_length(rr, 0);
-            goto start;
-        }
         al = SSL_AD_UNEXPECTED_MESSAGE;
         SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_UNEXPECTED_RECORD);
         goto f_err;
         al = SSL_AD_UNEXPECTED_MESSAGE;
         SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_UNEXPECTED_RECORD);
         goto f_err;
@@ -801,39 +725,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
     return -1;
 }
 
     return -1;
 }
 
-/*
- * this only happens when a client hello is received and a handshake
- * is started.
- */
-static size_t have_handshake_fragment(SSL *s, int type, unsigned char *buf,
-                                      size_t len)
-{
-
-    if ((type == SSL3_RT_HANDSHAKE)
-        && (s->rlayer.d->handshake_fragment_len > 0))
-        /* (partially) satisfy request from storage */
-    {
-        unsigned char *src = s->rlayer.d->handshake_fragment;
-        unsigned char *dst = buf;
-        size_t k, n;
-
-        /* peek == 0 */
-        n = 0;
-        while ((len > 0) && (s->rlayer.d->handshake_fragment_len > 0)) {
-            *dst++ = *src++;
-            len--;
-            s->rlayer.d->handshake_fragment_len--;
-            n++;
-        }
-        /* move any remaining fragment bytes: */
-        for (k = 0; k < s->rlayer.d->handshake_fragment_len; k++)
-            s->rlayer.d->handshake_fragment[k] = *src++;
-        return n;
-    }
-
-    return 0;
-}
-
 /*
  * Call this to write data in records of type 'type' It will return <= 0 if
  * not all data has been sent or non-blocking IO.
 /*
  * Call this to write data in records of type 'type' It will return <= 0 if
  * not all data has been sent or non-blocking IO.
index de112cc..dabb02c 100644 (file)
@@ -17,6 +17,7 @@
 #include <openssl/buffer.h>
 #include <openssl/rand.h>
 #include "record_locl.h"
 #include <openssl/buffer.h>
 #include <openssl/rand.h>
 #include "record_locl.h"
+#include "../packet_locl.h"
 
 #if     defined(OPENSSL_SMALL_FOOTPRINT) || \
         !(      defined(AES_ASM) &&     ( \
 
 #if     defined(OPENSSL_SMALL_FOOTPRINT) || \
         !(      defined(AES_ASM) &&     ( \
@@ -47,8 +48,6 @@ void RECORD_LAYER_clear(RECORD_LAYER *rl)
     rl->packet = NULL;
     rl->packet_length = 0;
     rl->wnum = 0;
     rl->packet = NULL;
     rl->packet_length = 0;
     rl->wnum = 0;
-    memset(rl->alert_fragment, 0, sizeof(rl->alert_fragment));
-    rl->alert_fragment_len = 0;
     memset(rl->handshake_fragment, 0, sizeof(rl->handshake_fragment));
     rl->handshake_fragment_len = 0;
     rl->wpend_tot = 0;
     memset(rl->handshake_fragment, 0, sizeof(rl->handshake_fragment));
     rl->handshake_fragment_len = 0;
     rl->wpend_tot = 0;
@@ -1402,10 +1401,6 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
             dest_maxlen = sizeof s->rlayer.handshake_fragment;
             dest = s->rlayer.handshake_fragment;
             dest_len = &s->rlayer.handshake_fragment_len;
             dest_maxlen = sizeof s->rlayer.handshake_fragment;
             dest = s->rlayer.handshake_fragment;
             dest_len = &s->rlayer.handshake_fragment_len;
-        } else if (SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
-            dest_maxlen = sizeof s->rlayer.alert_fragment;
-            dest = s->rlayer.alert_fragment;
-            dest_len = &s->rlayer.alert_fragment_len;
         }
 
         if (dest_maxlen > 0) {
         }
 
         if (dest_maxlen > 0) {
@@ -1422,20 +1417,6 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
             if (SSL3_RECORD_get_length(rr) == 0)
                 SSL3_RECORD_set_read(rr);
 
             if (SSL3_RECORD_get_length(rr) == 0)
                 SSL3_RECORD_set_read(rr);
 
-            if (SSL_IS_TLS13(s)
-                    && SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
-                if (*dest_len < dest_maxlen
-                        || SSL3_RECORD_get_length(rr) != 0) {
-                    /*
-                     * TLSv1.3 forbids fragmented alerts, and only one alert
-                     * may be present in a record
-                     */
-                    al = SSL_AD_UNEXPECTED_MESSAGE;
-                    SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_INVALID_ALERT);
-                    goto f_err;
-                }
-            }
-
             if (*dest_len < dest_maxlen)
                 goto start;     /* fragment was too small */
         }
             if (*dest_len < dest_maxlen)
                 goto start;     /* fragment was too small */
         }
@@ -1443,7 +1424,6 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
 
     /*-
      * s->rlayer.handshake_fragment_len == 4  iff  rr->type == SSL3_RT_HANDSHAKE;
 
     /*-
      * s->rlayer.handshake_fragment_len == 4  iff  rr->type == SSL3_RT_HANDSHAKE;
-     * s->rlayer.alert_fragment_len == 2      iff  rr->type == SSL3_RT_ALERT.
      * (Possibly rr is 'empty' now, i.e. rr->length may be 0.)
      */
 
      * (Possibly rr is 'empty' now, i.e. rr->length may be 0.)
      */
 
@@ -1466,15 +1446,23 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
         ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION);
         goto start;
     }
         ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION);
         goto start;
     }
-    if (s->rlayer.alert_fragment_len >= 2) {
-        int alert_level = s->rlayer.alert_fragment[0];
-        int alert_descr = s->rlayer.alert_fragment[1];
-
-        s->rlayer.alert_fragment_len = 0;
+    if (SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
+        unsigned int alert_level, alert_descr;
+        unsigned char *alert_bytes = SSL3_RECORD_get_data(rr)
+                                     + SSL3_RECORD_get_off(rr);
+        PACKET alert;
+
+        if (!PACKET_buf_init(&alert, alert_bytes, SSL3_RECORD_get_length(rr))
+                || !PACKET_get_1(&alert, &alert_level)
+                || !PACKET_get_1(&alert, &alert_descr)
+                || PACKET_remaining(&alert) != 0) {
+            al = SSL_AD_UNEXPECTED_MESSAGE;
+            SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_INVALID_ALERT);
+            goto f_err;
+        }
 
         if (s->msg_callback)
 
         if (s->msg_callback)
-            s->msg_callback(0, s->version, SSL3_RT_ALERT,
-                            s->rlayer.alert_fragment, 2, s,
+            s->msg_callback(0, s->version, SSL3_RT_ALERT, alert_bytes, 2, s,
                             s->msg_callback_arg);
 
         if (s->info_callback != NULL)
                             s->msg_callback_arg);
 
         if (s->info_callback != NULL)
index 6880f77..32db821 100644 (file)
@@ -111,14 +111,6 @@ typedef struct dtls_record_layer_st {
      * loss.
      */
     record_pqueue buffered_app_data;
      * loss.
      */
     record_pqueue buffered_app_data;
-    /*
-     * storage for Alert/Handshake protocol data received but not yet
-     * processed by ssl3_read_bytes:
-     */
-    unsigned char alert_fragment[DTLS1_AL_HEADER_LENGTH];
-    size_t alert_fragment_len;
-    unsigned char handshake_fragment[DTLS1_HM_HEADER_LENGTH];
-    size_t handshake_fragment_len;
     /* save last and current sequence numbers for retransmissions */
     unsigned char last_write_sequence[8];
     unsigned char curr_write_sequence[8];
     /* save last and current sequence numbers for retransmissions */
     unsigned char last_write_sequence[8];
     unsigned char curr_write_sequence[8];
@@ -157,12 +149,6 @@ typedef struct record_layer_st {
     size_t packet_length;
     /* number of bytes sent so far */
     size_t wnum;
     size_t packet_length;
     /* number of bytes sent so far */
     size_t wnum;
-    /*
-     * storage for Alert/Handshake protocol data received but not yet
-     * processed by ssl3_read_bytes:
-     */
-    unsigned char alert_fragment[2];
-    size_t alert_fragment_len;
     unsigned char handshake_fragment[4];
     size_t handshake_fragment_len;
     /* The number of consecutive empty records we have received */
     unsigned char handshake_fragment[4];
     size_t handshake_fragment_len;
     /* The number of consecutive empty records we have received */
index 99b0181..bac738c 100644 (file)
@@ -59,14 +59,14 @@ $proxy->serverflags("-tls1_2");
 $proxy->start();
 ok(TLSProxy::Message->fail(), "Too many in context empty records test");
 
 $proxy->start();
 ok(TLSProxy::Message->fail(), "Too many in context empty records test");
 
-#Test 4: Injecting a fragmented fatal alert should fail. We actually expect no
-#        alerts to be sent from either side because *we* injected the fatal
-#        alert, i.e. this will look like a disorderly close
+#Test 4: Injecting a fragmented fatal alert should fail. We expect the server to
+#        send back an alert of its own because it cannot handle fragmented
+#        alerts
 $proxy->clear();
 $proxy->filter(\&add_frag_alert_filter);
 $proxy->serverflags("-tls1_2");
 $proxy->start();
 $proxy->clear();
 $proxy->filter(\&add_frag_alert_filter);
 $proxy->serverflags("-tls1_2");
 $proxy->start();
-ok(!TLSProxy::Message->end(), "Fragmented alert records test");
+ok(TLSProxy::Message->fail(), "Fragmented alert records test");
 
 #Run some SSLv2 ClientHello tests
 
 
 #Run some SSLv2 ClientHello tests