Commit bd990e25 authored by Matt Caswell's avatar Matt Caswell
Browse files

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: default avatarRich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3476)
parent e1cfd184

CHANGES

+10 −0
Original line number Diff line number Diff line
@@ -4,6 +4,16 @@ 














 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

































ssl/record/rec_layer_d1.c



+23
−132






























Original line number
Diff line number
Diff line








@@ -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) &&













    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;










@@ -801,39 +725,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,













    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.

































ssl/record/rec_layer_s3.c



+16
−28






























Original line number
Diff line number
Diff line








@@ -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];













    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;





























        s->rlayer.alert_fragment_len = 0;













        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)

































ssl/record/record.h



+0
−14






























Original line number
Diff line number
Diff line








@@ -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 */

































test/recipes/70-test_sslrecords.t



+4
−4






























Original line number
Diff line number
Diff line








@@ -59,14 +59,14 @@ $proxy->serverflags("-tls1_2");













$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();













ok(!TLSProxy::Message->end(), "Fragmented alert records test");













ok(TLSProxy::Message->fail(), "Fragmented alert records test");































#Run some SSLv2 ClientHello tests