From 49c081a03cd8415bb7ff887e4c7b07f7e5ddf1a2 Mon Sep 17 00:00:00 2001 From: jsing Date: Tue, 31 Aug 2021 13:34:55 +0000 Subject: [PATCH] Defragment DTLS. In normal TLS, it is possible for record fragments to be sent that contain one byte of alert or handshake message payload. In this case we have to read and collate multiple message fragments before we can decide what to do with the record. However, in the case of DTLS, one record is effectively one packet and while it is possible to send handshake messages across multiple records/packets, the minimum payload is the DTLS handshake message header (plus one byte of data if the handshake message has a payload) - without this, there is insufficient information available to be able to reassemble the handshake message. Likewise, splitting an alert across multiple DTLS records simply does not work, as we have no way of knowing if we're collating the same alert or two different alerts that we lost half of each from (unfortunately, these details are not really specified in the DTLS RFC). This means that for DTLS we can expect to receive a full alert message (a whole two bytes) or a handshake record with at least the handshake message header (12 bytes). If we receive messages with less than these lengths we discard them and carry on (which is what the DTLS code already does). Remove all of the pointless fragment handling code from DTLS, while also fixing an issue where one case used rr->data instead of the handshake fragment. ok inoguchi@ tb@ --- lib/libssl/d1_pkt.c | 162 ++++++++++++----------------------------- lib/libssl/dtls_locl.h | 9 +-- 2 files changed, 48 insertions(+), 123 deletions(-) diff --git a/lib/libssl/d1_pkt.c b/lib/libssl/d1_pkt.c index 0b66bf7cc81..22f0167c750 100644 --- a/lib/libssl/d1_pkt.c +++ b/lib/libssl/d1_pkt.c @@ -1,4 +1,4 @@ -/* $OpenBSD: d1_pkt.c,v 1.108 2021/08/31 13:14:43 jsing Exp $ */ +/* $OpenBSD: d1_pkt.c,v 1.109 2021/08/31 13:34:55 jsing Exp $ */ /* * DTLS implementation written by Nagendra Modadugu * (nagendra@cs.stanford.edu) for the OpenSSL project 2005. @@ -178,8 +178,6 @@ satsub64be(const unsigned char *v1, const unsigned char *v2) return brw + (ret & 0xFF); } -static int have_handshake_fragment(SSL *s, int type, unsigned char *buf, - int len, int peek); static int dtls1_record_replay_check(SSL *s, DTLS1_BITMAP *bitmap, const unsigned char *seq); static void dtls1_record_bitmap_update(SSL *s, DTLS1_BITMAP *bitmap, @@ -530,15 +528,7 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) return -1; } - /* check whether there's a handshake message (client hello?) waiting */ - if ((ret = have_handshake_fragment(s, type, buf, len, peek))) - return ret; - - /* Now D1I(s)->handshake_fragment_len == 0 if type == SSL3_RT_HANDSHAKE. */ - - if (!s->internal->in_handshake && SSL_in_init(s)) - { - /* type == SSL3_RT_APPLICATION_DATA */ + if (!s->internal->in_handshake && SSL_in_init(s)) { i = s->internal->handshake_func(s); if (i < 0) return (i); @@ -645,89 +635,62 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) return (n); } - - /* If we get here, then type != rr->type; if we have a handshake - * message, 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. + /* + * If we get here, then type != rr->type; if we have a handshake + * message, then it was unexpected (Hello Request or Client Hello). */ + { - unsigned int k, dest_maxlen = 0; - unsigned char *dest = NULL; - unsigned int *dest_len = NULL; + unsigned int record_min_len = 0; if (rr->type == SSL3_RT_HANDSHAKE) { - dest_maxlen = sizeof D1I(s)->handshake_fragment; - dest = D1I(s)->handshake_fragment; - dest_len = &D1I(s)->handshake_fragment_len; + record_min_len = DTLS1_HM_HEADER_LENGTH; } else if (rr->type == SSL3_RT_ALERT) { - dest_maxlen = sizeof(D1I(s)->alert_fragment); - dest = D1I(s)->alert_fragment; - dest_len = &D1I(s)->alert_fragment_len; - } - /* else it's a CCS message, or application data or wrong */ - else if (rr->type != SSL3_RT_CHANGE_CIPHER_SPEC) { - /* Application data while renegotiating - * is allowed. Try again reading. + record_min_len = DTLS1_AL_HEADER_LENGTH; + } else if (rr->type == SSL3_RT_CHANGE_CIPHER_SPEC) { + record_min_len = DTLS1_CCS_HEADER_LENGTH; + } else if (rr->type == SSL3_RT_APPLICATION_DATA) { + /* + * Application data while renegotiating is allowed. + * Try reading again. */ - if (rr->type == SSL3_RT_APPLICATION_DATA) { - BIO *bio; - S3I(s)->in_read_app_data = 2; - bio = SSL_get_rbio(s); - s->internal->rwstate = SSL_READING; - BIO_clear_retry_flags(bio); - BIO_set_retry_read(bio); - return (-1); - } + BIO *bio; + S3I(s)->in_read_app_data = 2; + bio = SSL_get_rbio(s); + s->internal->rwstate = SSL_READING; + BIO_clear_retry_flags(bio); + BIO_set_retry_read(bio); + return (-1); + } else { /* Not certain if this is the right error handling */ al = SSL_AD_UNEXPECTED_MESSAGE; SSLerror(s, SSL_R_UNEXPECTED_RECORD); goto fatal_err; } - if (dest_maxlen > 0) { - /* XDTLS: In a pathalogical case, the Client Hello - * may be fragmented--don't always expect dest_maxlen bytes */ - if (rr->length < dest_maxlen) { - s->internal->rstate = SSL_ST_READ_HEADER; - rr->length = 0; - goto start; - } - - /* now move 'n' bytes: */ - for ( k = 0; k < dest_maxlen; k++) { - dest[k] = rr->data[rr->off++]; - rr->length--; - } - *dest_len = dest_maxlen; + if (record_min_len > 0 && rr->length < record_min_len) { + s->internal->rstate = SSL_ST_READ_HEADER; + rr->length = 0; + goto start; } } - /* D1I(s)->handshake_fragment_len == 12 iff rr->type == SSL3_RT_HANDSHAKE; - * D1I(s)->alert_fragment_len == 7 iff rr->type == SSL3_RT_ALERT. - * (Possibly rr is 'empty' now, i.e. rr->length may be 0.) */ - /* If we are a client, check for an incoming 'Hello Request': */ - if ((!s->server) && - (D1I(s)->handshake_fragment_len >= DTLS1_HM_HEADER_LENGTH) && - (D1I(s)->handshake_fragment[0] == SSL3_MT_HELLO_REQUEST) && - (s->session != NULL) && (s->session->cipher != NULL)) { - D1I(s)->handshake_fragment_len = 0; - - if ((D1I(s)->handshake_fragment[1] != 0) || - (D1I(s)->handshake_fragment[2] != 0) || - (D1I(s)->handshake_fragment[3] != 0)) { + if (!s->server && rr->type == SSL3_RT_HANDSHAKE && + rr->length >= DTLS1_HM_HEADER_LENGTH && rr->off == 0 && + rr->data[0] == SSL3_MT_HELLO_REQUEST && + s->session != NULL && s->session->cipher != NULL) { + if (rr->data[1] != 0 || rr->data[2] != 0 || rr->data[3] != 0) { al = SSL_AD_DECODE_ERROR; SSLerror(s, SSL_R_BAD_HELLO_REQUEST); goto fatal_err; } + rr->length = 0; /* no need to check sequence number on HELLO REQUEST messages */ - ssl_msg_callback(s, 0, SSL3_RT_HANDSHAKE, - D1I(s)->handshake_fragment, 4); + ssl_msg_callback(s, 0, SSL3_RT_HANDSHAKE, rr->data, 4); if (SSL_is_init_finished(s) && !(s->s3->flags & SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS) && @@ -763,16 +726,16 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) } /* we either finished a handshake or ignored the request, * now try again to obtain the (application) data we were asked for */ + rr->length = 0; goto start; } - if (D1I(s)->alert_fragment_len >= DTLS1_AL_HEADER_LENGTH) { - int alert_level = D1I(s)->alert_fragment[0]; - int alert_descr = D1I(s)->alert_fragment[1]; - - D1I(s)->alert_fragment_len = 0; + if (rr->type == SSL3_RT_ALERT && rr->length >= DTLS1_AL_HEADER_LENGTH && + rr->off == 0) { + int alert_level = rr->data[0]; + int alert_descr = rr->data[1]; - ssl_msg_callback(s, 0, SSL3_RT_ALERT, D1I(s)->alert_fragment, 2); + ssl_msg_callback(s, 0, SSL3_RT_ALERT, rr->data, 2); ssl_info_callback(s, SSL_CB_READ_ALERT, (alert_level << 8) | alert_descr); @@ -798,11 +761,11 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) goto fatal_err; } + rr->length = 0; goto start; } - if (s->internal->shutdown & SSL_SENT_SHUTDOWN) /* but we have not received a shutdown */ - { + if (s->internal->shutdown & SSL_SENT_SHUTDOWN) { s->internal->rwstate = SSL_NOTHING; rr->length = 0; return (0); @@ -819,14 +782,13 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) goto fatal_err; } - rr->length = 0; - ssl_msg_callback(s, 0, SSL3_RT_CHANGE_CIPHER_SPEC, rr->data, 1); /* We can't process a CCS now, because previous handshake * messages are still missing, so just drop it. */ if (!D1I(s)->change_cipher_spec_ok) { + rr->length = 0; goto start; } @@ -836,11 +798,13 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) if (!ssl3_do_change_cipher_spec(s)) goto err; + rr->length = 0; goto start; } /* Unexpected handshake message (Client Hello, or protocol violation) */ - if ((D1I(s)->handshake_fragment_len >= DTLS1_HM_HEADER_LENGTH) && + if (rr->type == SSL3_RT_HANDSHAKE && + rr->length >= DTLS1_HM_HEADER_LENGTH && rr->off == 0 && !s->internal->in_handshake) { struct hm_header_st msg_hdr; @@ -893,6 +857,7 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) return (-1); } } + rr->length = 0; goto start; } @@ -967,39 +932,6 @@ dtls1_write_app_data_bytes(SSL *s, int type, const void *buf_, int len) return i; } - - /* this only happens when a client hello is received and a handshake - * is started. */ -static int -have_handshake_fragment(SSL *s, int type, unsigned char *buf, - int len, int peek) -{ - - if ((type == SSL3_RT_HANDSHAKE) && (D1I(s)->handshake_fragment_len > 0)) - /* (partially) satisfy request from storage */ - { - unsigned char *src = D1I(s)->handshake_fragment; - unsigned char *dst = buf; - unsigned int k, n; - - /* peek == 0 */ - n = 0; - while ((len > 0) && (D1I(s)->handshake_fragment_len > 0)) { - *dst++ = *src++; - len--; - D1I(s)->handshake_fragment_len--; - n++; - } - /* move any remaining fragment bytes: */ - for (k = 0; k < D1I(s)->handshake_fragment_len; k++) - D1I(s)->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/lib/libssl/dtls_locl.h b/lib/libssl/dtls_locl.h index 83fb9e0e10c..502b42dcdd8 100644 --- a/lib/libssl/dtls_locl.h +++ b/lib/libssl/dtls_locl.h @@ -1,4 +1,4 @@ -/* $OpenBSD: dtls_locl.h,v 1.5 2021/08/30 19:12:25 jsing Exp $ */ +/* $OpenBSD: dtls_locl.h,v 1.6 2021/08/31 13:34:55 jsing Exp $ */ /* * DTLS implementation written by Nagendra Modadugu * (nagendra@cs.stanford.edu) for the OpenSSL project 2005. @@ -167,13 +167,6 @@ typedef struct dtls1_state_internal_st { struct dtls1_timeout_st timeout; - /* storage for Alert/Handshake protocol data received but not - * yet processed by ssl3_read_bytes: */ - unsigned char alert_fragment[DTLS1_AL_HEADER_LENGTH]; - unsigned int alert_fragment_len; - unsigned char handshake_fragment[DTLS1_HM_HEADER_LENGTH]; - unsigned int handshake_fragment_len; - unsigned int retransmitting; unsigned int change_cipher_spec_ok; } DTLS1_STATE_INTERNAL; -- 2.20.1