From bd990e2535ca387def9a01218a813dc3fa547e3c Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 15 May 2017 11:24:24 +0100 Subject: [PATCH] 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 (Merged from https://github.com/openssl/openssl/pull/3476) --- CHANGES | 10 ++ ssl/record/rec_layer_d1.c | 155 +++++------------------------- ssl/record/rec_layer_s3.c | 44 +++------ ssl/record/record.h | 14 --- test/recipes/70-test_sslrecords.t | 8 +- 5 files changed, 53 insertions(+), 178 deletions(-) diff --git a/CHANGES b/CHANGES index 7bd0f924f1..9e4271d323 100644 --- a/CHANGES +++ b/CHANGES @@ -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 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 #include "record_locl.h" #include +#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; @@ -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. 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 #include #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 */ diff --git a/test/recipes/70-test_sslrecords.t b/test/recipes/70-test_sslrecords.t index 99b0181dde..bac738c311 100644 --- a/test/recipes/70-test_sslrecords.t +++ b/test/recipes/70-test_sslrecords.t @@ -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 -- 2.25.1