Don't allow fragmented alerts

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)

3 files changed, 39 insertions, 174 deletions

diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c
index 243eff7004..487b096b24 100644
--- a/ssl/record/rec_layer_d1.c
+++ b/ssl/record/rec_layer_d1.c
@@ -15,6 +15,7 @@
 #include <openssl/buffer.h>
 #include "record_locl.h"
 #include <assert.h>
+#include "../packet_locl.h"
 
 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);
 }
 
-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)
 {
@@ -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 ret, n;
+    size_t n;
     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;
     }
 
-    /*
-     * 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 */
@@ -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).
      */
 
-    /*
-     * 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;
-            SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_UNEXPECTED_RECORD);
+            SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_INVALID_ALERT);
             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)
-            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)
@@ -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)
      */
-    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;
 
-        /* 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;
         }
 
+        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.
@@ -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:
-        /* 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;
@@ -802,39 +726,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
 }
 
 /*
- * 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.
  */
diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index de112cc806..dabb02cf1b 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -17,6 +17,7 @@
 #include <openssl/buffer.h>
 #include <openssl/rand.h>
 #include "record_locl.h"
+#include "../packet_locl.h"
 
 #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;
-    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;
@@ -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;
-        } 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) {
@@ -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 (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 */
         }
@@ -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.alert_fragment_len == 2      iff  rr->type == SSL3_RT_ALERT.
      * (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;
     }
-    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)
-            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)
diff --git a/ssl/record/record.h b/ssl/record/record.h
index 6880f77ddc..32db8212aa 100644
--- a/ssl/record/record.h
+++ b/ssl/record/record.h
@@ -111,14 +111,6 @@ typedef struct dtls_record_layer_st {
      * 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];
@@ -157,12 +149,6 @@ typedef struct record_layer_st {
     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 */