From 8ccde3fc78b8db0acf8c11454b5dc4fb01485f4c Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 25 Oct 2022 15:47:36 +0100 Subject: [PATCH] Fix read pipelining During read pipelining we must ensure that the buffer is sufficiently large to read enough data to fill our pipelines. We also remove some code that moved data to the start of the packet if we can. This was unnecessary because of later code which would end up moving it anyway. The earlier move was also incorrect in the case that |clearold| was 0. This would cause the read pipelining code to fail with sufficiently large records. Reviewed-by: Hugo Landau Reviewed-by: Todd Short Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/19456) --- ssl/record/methods/tls_common.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index a7b31163c5..666a4f6ae2 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -223,8 +223,14 @@ int tls_setup_read_buffer(OSSL_RECORD_LAYER *rl) if (tls_allow_compression(rl)) len += SSL3_RT_MAX_COMPRESSED_OVERHEAD; #endif + + /* Ensure our buffer is large enough to support all our pipelines */ + if (rl->max_pipelines > 1) + len *= rl->max_pipelines; + if (b->default_len > len) len = b->default_len; + if ((p = OPENSSL_malloc(len)) == NULL) { /* * We've got a malloc failure, and we're still initialising buffers. @@ -284,27 +290,9 @@ int tls_default_read_n(OSSL_RECORD_LAYER *rl, size_t n, size_t max, int extend, if (!extend) { /* start with empty packet ... */ - if (left == 0) { + if (left == 0) rb->offset = align; - } else if (align != 0 && left >= SSL3_RT_HEADER_LENGTH) { - /* - * check if next packet length is large enough to justify payload - * alignment... - */ - pkt = rb->buf + rb->offset; - if (pkt[0] == SSL3_RT_APPLICATION_DATA - && (pkt[3] << 8 | pkt[4]) >= 128) { - /* - * Note that even if packet is corrupted and its length field - * is insane, we can only be led to wrong decision about - * whether memmove will occur or not. Header values has no - * effect on memmove arguments and therefore no buffer - * overrun can be triggered. - */ - memmove(rb->buf + align, pkt, left); - rb->offset = align; - } - } + rl->packet = rb->buf + rb->offset; rl->packet_length = 0; /* ... now we can act as if 'extend' was set */ -- 2.34.1