From d1bfd8076e28b134f7d6a03611e60381b522c1c9 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 9 Oct 2018 10:22:06 +0100 Subject: [PATCH] Buffer a ClientHello with a cookie received via DTLSv1_listen Previously when a ClientHello arrives with a valid cookie using DTLSv1_listen() we only "peeked" at the message and left it on the underlying fd. This works fine for single threaded applications but for multi-threaded apps this does not work since the fd is typically reused for the server thread, while a new fd is created and connected for the client. By "peeking" we leave the message on the server fd, and consequently we think we've received another valid ClientHello and so we create yet another fd for the client, and so on until we run out of fds. In this new approach we remove the ClientHello and buffer it in the SSL object. Fixes #6934 Reviewed-by: Ben Kaduk (Merged from https://github.com/openssl/openssl/pull/7375) (cherry picked from commit 079ef6bd534d2f708d8013cfcd8ea0d2f600c788) --- ssl/d1_lib.c | 48 +++++++++++++++++----------------------- ssl/record/record.h | 2 ++ ssl/record/ssl3_record.c | 25 +++++++++++++++++++++ 3 files changed, 47 insertions(+), 28 deletions(-) diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index 38adda3355..7a7a4bed69 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c @@ -445,12 +445,12 @@ static void get_current_time(struct timeval *t) #ifndef OPENSSL_NO_SOCK int DTLSv1_listen(SSL *s, BIO_ADDR *client) { - int next, n, ret = 0, clearpkt = 0; + int next, n, ret = 0; unsigned char cookie[DTLS1_COOKIE_LENGTH]; unsigned char seq[SEQ_NUM_SIZE]; const unsigned char *data; unsigned char *buf, *wbuf; - size_t fragoff, fraglen, msglen; + size_t fragoff, fraglen, msglen, reclen, align = 0; unsigned int rectype, versmajor, msgseq, msgtype, clientvers, cookielen; BIO *rbio, *wbio; BIO_ADDR *tmpclient = NULL; @@ -475,13 +475,6 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) return -1; } - /* - * We only peek at incoming ClientHello's until we're sure we are going to - * to respond with a HelloVerifyRequest. If its a ClientHello with a valid - * cookie then we leave it in the BIO for accept to handle. - */ - BIO_ctrl(SSL_get_rbio(s), BIO_CTRL_DGRAM_SET_PEEK_MODE, 1, NULL); - /* * Note: This check deliberately excludes DTLS1_BAD_VER because that version * requires the MAC to be calculated *including* the first ClientHello @@ -500,6 +493,19 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) } buf = RECORD_LAYER_get_rbuf(&s->rlayer)->buf; wbuf = RECORD_LAYER_get_wbuf(&s->rlayer)[0].buf; +#if defined(SSL3_ALIGN_PAYLOAD) +# if SSL3_ALIGN_PAYLOAD != 0 + /* + * Using SSL3_RT_HEADER_LENGTH here instead of DTLS1_RT_HEADER_LENGTH for + * consistency with ssl3_read_n. In practice it should make no difference + * for sensible values of SSL3_ALIGN_PAYLOAD because the difference between + * SSL3_RT_HEADER_LENGTH and DTLS1_RT_HEADER_LENGTH is exactly 8 + */ + align = (size_t)buf + SSL3_RT_HEADER_LENGTH; + align = SSL3_ALIGN_PAYLOAD - 1 - ((align - 1) % SSL3_ALIGN_PAYLOAD); +# endif +#endif + buf += align; do { /* Get a packet */ @@ -507,7 +513,6 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) clear_sys_error(); n = BIO_read(rbio, buf, SSL3_RT_MAX_PLAIN_LENGTH + DTLS1_RT_HEADER_LENGTH); - if (n <= 0) { if (BIO_should_retry(rbio)) { /* Non-blocking IO */ @@ -516,9 +521,6 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) return -1; } - /* If we hit any problems we need to clear this packet from the BIO */ - clearpkt = 1; - if (!PACKET_buf_init(&pkt, buf, n)) { SSLerr(SSL_F_DTLSV1_LISTEN, ERR_R_INTERNAL_ERROR); return -1; @@ -571,6 +573,7 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) SSLerr(SSL_F_DTLSV1_LISTEN, SSL_R_LENGTH_MISMATCH); goto end; } + reclen = PACKET_remaining(&msgpkt); /* * We allow data remaining at the end of the packet because there could * be a second record (but we ignore it) @@ -690,14 +693,6 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) * to resend, we just drop it. */ - /* - * Dump the read packet, we don't need it any more. Ignore return - * value - */ - BIO_ctrl(SSL_get_rbio(s), BIO_CTRL_DGRAM_SET_PEEK_MODE, 0, NULL); - BIO_read(rbio, buf, SSL3_RT_MAX_PLAIN_LENGTH); - BIO_ctrl(SSL_get_rbio(s), BIO_CTRL_DGRAM_SET_PEEK_MODE, 1, NULL); - /* Generate the cookie */ if (s->ctx->app_gen_cookie_cb == NULL || s->ctx->app_gen_cookie_cb(s, cookie, &cookielen) == 0 || @@ -718,7 +713,7 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) /* Construct the record and message headers */ if (!WPACKET_init_static_len(&wpkt, wbuf, - SSL3_RT_MAX_PLAIN_LENGTH + ssl_get_max_send_fragment(s) + DTLS1_RT_HEADER_LENGTH, 0) || !WPACKET_put_bytes_u8(&wpkt, SSL3_RT_HANDSHAKE) @@ -853,16 +848,13 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) if (BIO_dgram_get_peer(rbio, client) <= 0) BIO_ADDR_clear(client); + /* Buffer the record in the processed_rcds queue */ + if (!dtls_buffer_listen_record(s, reclen, seq, align)) + return -1; ret = 1; - clearpkt = 0; end: BIO_ADDR_free(tmpclient); - BIO_ctrl(SSL_get_rbio(s), BIO_CTRL_DGRAM_SET_PEEK_MODE, 0, NULL); - if (clearpkt) { - /* Dump this packet. Ignore return value */ - BIO_read(rbio, buf, SSL3_RT_MAX_PLAIN_LENGTH); - } return ret; } #endif diff --git a/ssl/record/record.h b/ssl/record/record.h index 93f6f9ee3f..76a2b81f0d 100644 --- a/ssl/record/record.h +++ b/ssl/record/record.h @@ -232,3 +232,5 @@ __owur int dtls1_write_bytes(SSL *s, int type, const void *buf, size_t len, int do_dtls1_write(SSL *s, int type, const unsigned char *buf, size_t len, int create_empty_fragment, size_t *written); void dtls1_reset_seq_numbers(SSL *s, int rw); +int dtls_buffer_listen_record(SSL *s, size_t len, unsigned char *seq, + size_t off); diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index a616bf0409..e59ac5a676 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -2030,3 +2030,28 @@ int dtls1_get_record(SSL *s) return 1; } + +int dtls_buffer_listen_record(SSL *s, size_t len, unsigned char *seq, size_t off) +{ + SSL3_RECORD *rr; + + rr = RECORD_LAYER_get_rrec(&s->rlayer); + memset(rr, 0, sizeof(SSL3_RECORD)); + + rr->length = len; + rr->type = SSL3_RT_HANDSHAKE; + memcpy(rr->seq_num, seq, sizeof(rr->seq_num)); + rr->off = off; + + s->rlayer.packet = RECORD_LAYER_get_rbuf(&s->rlayer)->buf; + s->rlayer.packet_length = DTLS1_RT_HEADER_LENGTH + len; + rr->data = s->rlayer.packet + DTLS1_RT_HEADER_LENGTH; + + if (dtls1_buffer_record(s, &(s->rlayer.d->processed_rcds), + SSL3_RECORD_get_seq_num(s->rlayer.rrec)) <= 0) { + /* SSLfatal() already called */ + return 0; + } + + return 1; +}