From 24da907dfde1794b58b5dc570f9f1d5c6a54f2c9 Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 15:39:58 +0800 Subject: [PATCH 01/11] Only free the read buffers if we're not using them If we're part way through processing a record, or the application has not released all the records then we should not free our buffer because they are still needed. CVE-2024-4741 --- ssl/record/rec_layer_s3.c | 9 +++++++++ ssl/record/record.h | 1 + ssl/ssl_lib.c | 3 +++ 3 files changed, 13 insertions(+) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index fe1b3f3c1..af4aeb1bd 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -80,6 +80,15 @@ int RECORD_LAYER_read_pending(const RECORD_LAYER *rl) return SSL3_BUFFER_get_left(&rl->rbuf) != 0; } +int RECORD_LAYER_data_present(const RECORD_LAYER *rl) +{ + if (rl->rstate == SSL_ST_READ_BODY) + return 1; + if (RECORD_LAYER_processed_read_pending(rl)) + return 1; + return 0; +} + /* Checks if we have decrypted unread record data pending */ int RECORD_LAYER_processed_read_pending(const RECORD_LAYER *rl) { diff --git a/ssl/record/record.h b/ssl/record/record.h index 234656bf9..b60f71c8c 100644 --- a/ssl/record/record.h +++ b/ssl/record/record.h @@ -205,6 +205,7 @@ void RECORD_LAYER_release(RECORD_LAYER *rl); int RECORD_LAYER_read_pending(const RECORD_LAYER *rl); int RECORD_LAYER_processed_read_pending(const RECORD_LAYER *rl); int RECORD_LAYER_write_pending(const RECORD_LAYER *rl); +int RECORD_LAYER_data_present(const RECORD_LAYER *rl); void RECORD_LAYER_reset_read_sequence(RECORD_LAYER *rl); void RECORD_LAYER_reset_write_sequence(RECORD_LAYER *rl); int RECORD_LAYER_is_sslv2_record(RECORD_LAYER *rl); diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 03f2f051f..6b0b21d0b 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -6075,6 +6075,9 @@ int SSL_free_buffers(SSL *ssl) if (RECORD_LAYER_read_pending(rl) || RECORD_LAYER_write_pending(rl)) return 0; + if (RECORD_LAYER_data_present(rl)) + return 0; + RECORD_LAYER_release(rl); return 1; } From 5f6339433b9f8ff9686fd7fffea73960629418f9 Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 15:42:06 +0800 Subject: [PATCH 02/11] Set rlayer.packet to NULL after we've finished using it In order to ensure we do not have a UAF we reset the rlayer.packet pointer to NULL after we free it. CVE-2024-4741 --- ssl/record/rec_layer_s3.c | 7 +++++++ ssl/record/ssl3_buffer.c | 2 ++ 2 files changed, 9 insertions(+) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index af4aeb1bd..3d57380a9 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -16,6 +16,7 @@ #include #include "record_local.h" #include "internal/packet.h" +#include "internal/cryptlib.h" #if defined(OPENSSL_SMALL_FOOTPRINT) || \ !( defined(AES_ASM) && ( \ @@ -235,6 +236,12 @@ int ssl3_read_n(SSL *s, size_t n, size_t max, int extend, int clearold, /* ... now we can act as if 'extend' was set */ } + if (!ossl_assert(s->rlayer.packet != NULL)) { + /* does not happen */ + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + return -1; + } + len = s->rlayer.packet_length; pkt = rb->buf + align; /* diff --git a/ssl/record/ssl3_buffer.c b/ssl/record/ssl3_buffer.c index 01c553ebf..bdfb5b740 100644 --- a/ssl/record/ssl3_buffer.c +++ b/ssl/record/ssl3_buffer.c @@ -181,5 +181,7 @@ int ssl3_release_read_buffer(SSL *s) OPENSSL_cleanse(b->buf, b->len); OPENSSL_free(b->buf); b->buf = NULL; + s->rlayer.packet = NULL; + s->rlayer.packet_length = 0; return 1; } From 8b7953cf351baf613960aaea364782836b865717 Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 15:43:59 +0800 Subject: [PATCH 03/11] Extend the SSL_free_buffers testing Test that attempting to free the buffers at points where they should not be freed works as expected. Follow on from CVE-2024-4741 --- test/sslbuffertest.c | 93 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/test/sslbuffertest.c b/test/sslbuffertest.c index 3c3e69d61..133fdb13e 100644 --- a/test/sslbuffertest.c +++ b/test/sslbuffertest.c @@ -150,6 +150,98 @@ static int test_func(int test) return result; } +/* + * Test that attempting to free the buffers at points where they cannot be freed + * works as expected + * Test 0: Attempt to free buffers after a full record has been processed, but + * the application has only performed a partial read + * Test 1: Attempt to free buffers after only a partial record header has been + * received + * Test 2: Attempt to free buffers after a full record header but no record body + * Test 3: Attempt to free buffers after a full record hedaer and partial record + * body + */ +static int test_free_buffers(int test) +{ + int result = 0; + SSL *serverssl = NULL, *clientssl = NULL; + const char testdata[] = "Test data"; + char buf[40]; + size_t written, readbytes; + + if (!TEST_true(create_ssl_objects(serverctx, clientctx, &serverssl, + &clientssl, NULL, NULL))) + goto end; + + if (!TEST_true(create_ssl_connection(serverssl, clientssl, + SSL_ERROR_NONE))) + goto end; + + + if (!TEST_true(SSL_write_ex(clientssl, testdata, sizeof(testdata), + &written))) + goto end; + + if (test == 0) { + /* + * Deliberately only read the first byte - so the remaining bytes are + * still buffered + */ + if (!TEST_true(SSL_read_ex(serverssl, buf, 1, &readbytes))) + goto end; + } else { + BIO *tmp; + size_t partial_len; + + /* Remove all the data that is pending for read by the server */ + tmp = SSL_get_rbio(serverssl); + if (!TEST_true(BIO_read_ex(tmp, buf, sizeof(buf), &readbytes)) + || !TEST_size_t_lt(readbytes, sizeof(buf)) + || !TEST_size_t_gt(readbytes, SSL3_RT_HEADER_LENGTH)) + goto end; + + switch(test) { + case 1: + partial_len = SSL3_RT_HEADER_LENGTH - 1; + break; + case 2: + partial_len = SSL3_RT_HEADER_LENGTH; + break; + case 3: + partial_len = readbytes - 1; + break; + default: + TEST_error("Invalid test index"); + goto end; + } + + /* Put back just the partial record */ + if (!TEST_true(BIO_write_ex(tmp, buf, partial_len, &written))) + goto end; + + /* + * Attempt a read. This should fail because only a partial record is + * available. + */ + if (!TEST_false(SSL_read_ex(serverssl, buf, 1, &readbytes))) + goto end; + } + + /* + * Attempting to free the buffers at this point should fail because they are + * still in use + */ + if (!TEST_false(SSL_free_buffers(serverssl))) + goto end; + + result = 1; + end: + SSL_free(clientssl); + SSL_free(serverssl); + + return result; +} + OPT_TEST_DECLARE_USAGE("certfile privkeyfile\n") int setup_tests(void) @@ -173,6 +265,7 @@ int setup_tests(void) } ADD_ALL_TESTS(test_func, 9); + ADD_ALL_TESTS(test_free_buffers, 4); return 1; } From 4a717ca58a26c6d8a177ced28cb10a74be82336d Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 15:49:11 +0800 Subject: [PATCH 04/11] Move the ability to load the dasync engine into ssltestlib.c The sslapitest has a helper function to load the dasync engine which is useful for testing pipelining. We would like to have the same facility from sslbuffertest, so we move the function to the common location ssltestlib.c Follow on from CVE-2024-4741 --- test/helpers/ssltestlib.c | 33 +++++++++++++++++++++++++++++++++ test/helpers/ssltestlib.h | 1 + 2 files changed, 34 insertions(+) diff --git a/test/helpers/ssltestlib.c b/test/helpers/ssltestlib.c index 70438867f..2f5f1a364 100644 --- a/test/helpers/ssltestlib.c +++ b/test/helpers/ssltestlib.c @@ -7,8 +7,17 @@ * https://www.openssl.org/source/license.html */ +/* + * We need access to the deprecated low level ENGINE APIs for legacy purposes + * when the deprecated calls are not hidden + */ +#ifndef OPENSSL_NO_DEPRECATED_3_0 +# define OPENSSL_SUPPRESS_DEPRECATED +#endif + #include +#include #include "internal/nelem.h" #include "ssltestlib.h" #include "../testutil.h" @@ -1060,3 +1069,27 @@ void shutdown_ssl_connection(SSL *serverssl, SSL *clientssl) SSL_free(serverssl); SSL_free(clientssl); } + +ENGINE *load_dasync(void) +{ +#if !defined(OPENSSL_NO_TLS1_2) && !defined(OPENSSL_NO_DYNAMIC_ENGINE) + ENGINE *e; + + if (!TEST_ptr(e = ENGINE_by_id("dasync"))) + return NULL; + + if (!TEST_true(ENGINE_init(e))) { + ENGINE_free(e); + return NULL; + } + + if (!TEST_true(ENGINE_register_ciphers(e))) { + ENGINE_free(e); + return NULL; + } + + return e; +#else + return NULL; +#endif +} diff --git a/test/helpers/ssltestlib.h b/test/helpers/ssltestlib.h index 046628636..b9ad697f5 100644 --- a/test/helpers/ssltestlib.h +++ b/test/helpers/ssltestlib.h @@ -56,4 +56,5 @@ typedef struct mempacket_st MEMPACKET; DEFINE_STACK_OF(MEMPACKET) +ENGINE *load_dasync(void); #endif /* OSSL_TEST_SSLTESTLIB_H */ From 25080cf566a7de0cea705f41a600644b530d8684 Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 15:51:42 +0800 Subject: [PATCH 05/11] Further extend the SSL_free_buffers testing We extend the testing to test what happens when pipelining is in use. Follow on from CVE-2024-4741 --- test/sslbuffertest.c | 113 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 97 insertions(+), 16 deletions(-) diff --git a/test/sslbuffertest.c b/test/sslbuffertest.c index 133fdb13e..7079d04e1 100644 --- a/test/sslbuffertest.c +++ b/test/sslbuffertest.c @@ -8,10 +8,19 @@ * or in the file LICENSE in the source distribution. */ +/* + * We need access to the deprecated low level Engine APIs for legacy purposes + * when the deprecated calls are not hidden + */ +#ifndef OPENSSL_NO_DEPRECATED_3_0 +# define OPENSSL_SUPPRESS_DEPRECATED +#endif + #include #include #include #include +#include #include "internal/packet.h" @@ -160,34 +169,65 @@ static int test_func(int test) * Test 2: Attempt to free buffers after a full record header but no record body * Test 3: Attempt to free buffers after a full record hedaer and partial record * body + * Test 4-7: We repeat tests 0-3 but including data from a second pipelined + * record */ static int test_free_buffers(int test) { int result = 0; SSL *serverssl = NULL, *clientssl = NULL; const char testdata[] = "Test data"; - char buf[40]; + char buf[120]; size_t written, readbytes; + int i, pipeline = test > 3; + ENGINE *e = NULL; + + if (pipeline) { + e = load_dasync(); + if (e == NULL) + goto end; + test -= 4; + } if (!TEST_true(create_ssl_objects(serverctx, clientctx, &serverssl, &clientssl, NULL, NULL))) goto end; + if (pipeline) { + if (!TEST_true(SSL_set_cipher_list(serverssl, "AES128-SHA")) + || !TEST_true(SSL_set_max_proto_version(serverssl, + TLS1_2_VERSION)) + || !TEST_true(SSL_set_max_pipelines(serverssl, 2))) + goto end; + } + if (!TEST_true(create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE))) goto end; - - if (!TEST_true(SSL_write_ex(clientssl, testdata, sizeof(testdata), - &written))) - goto end; + /* + * For the non-pipeline case we write one record. For pipelining we write + * two records. + */ + for (i = 0; i <= pipeline; i++) { + if (!TEST_true(SSL_write_ex(clientssl, testdata, strlen(testdata), + &written))) + goto end; + } if (test == 0) { + size_t readlen = 1; + /* - * Deliberately only read the first byte - so the remaining bytes are - * still buffered - */ - if (!TEST_true(SSL_read_ex(serverssl, buf, 1, &readbytes))) + * Deliberately only read the first byte - so the remaining bytes are + * still buffered. In the pipelining case we read as far as the first + * byte from the second record. + */ + if (pipeline) + readlen += strlen(testdata); + + if (!TEST_true(SSL_read_ex(serverssl, buf, readlen, &readbytes)) + || !TEST_size_t_eq(readlen, readbytes)) goto end; } else { BIO *tmp; @@ -215,16 +255,47 @@ static int test_free_buffers(int test) goto end; } - /* Put back just the partial record */ - if (!TEST_true(BIO_write_ex(tmp, buf, partial_len, &written))) - goto end; + if (pipeline) { + /* We happen to know the first record is 57 bytes long */ + const size_t first_rec_len = 57; + + if (test != 3) + partial_len += first_rec_len; + + /* + * Sanity check. If we got the record len right then this should + * never fail. + */ + if (!TEST_int_eq(buf[first_rec_len], SSL3_RT_APPLICATION_DATA)) + goto end; + } /* - * Attempt a read. This should fail because only a partial record is - * available. + * Put back just the partial record (plus the whole initial record in + * the pipelining case) */ - if (!TEST_false(SSL_read_ex(serverssl, buf, 1, &readbytes))) + if (!TEST_true(BIO_write_ex(tmp, buf, partial_len, &written))) goto end; + + if (pipeline) { + /* + * Attempt a read. This should pass but only return data from the + * first record. Only a partial record is available for the second + * record. + */ + if (!TEST_true(SSL_read_ex(serverssl, buf, sizeof(buf), + &readbytes)) + || !TEST_size_t_eq(readbytes, strlen(testdata))) + goto end; + } else { + /* + * Attempt a read. This should fail because only a partial record is + * available. + */ + if (!TEST_false(SSL_read_ex(serverssl, buf, sizeof(buf), + &readbytes))) + goto end; + } } /* @@ -238,7 +309,13 @@ static int test_free_buffers(int test) end: SSL_free(clientssl); SSL_free(serverssl); - +#ifndef OPENSSL_NO_DYNAMIC_ENGINE + if (e != NULL) { + ENGINE_unregister_ciphers(e); + ENGINE_finish(e); + ENGINE_free(e); + } +#endif return result; } @@ -265,7 +342,11 @@ int setup_tests(void) } ADD_ALL_TESTS(test_func, 9); +#if !defined(OPENSSL_NO_TLS1_2) && !defined(OPENSSL_NO_DYNAMIC_ENGINE) + ADD_ALL_TESTS(test_free_buffers, 8); +#else ADD_ALL_TESTS(test_free_buffers, 4); +#endif return 1; } From c1acf0854e0d18ded46be70931e54a9bb4af963e Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 15:53:54 +0800 Subject: [PATCH 06/11] Add a CHANGES entry for CVE-2024-4741 --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 586f334fc..42fbd5704 100644 --- a/CHANGES +++ b/CHANGES @@ -4,6 +4,8 @@ Changes between 8.4.0 and 8.4.1 [xx XXX xxxx] + *) 修复CVE-2024-4741 + *) 修复CVE-2024-4603 *) 修复CVE-2024-2511 From 36b1c644ee977df5c9fcc7048fafdd80122dd7bf Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 17:55:42 +0800 Subject: [PATCH 07/11] Add a test for TLS pipelining TLS pipelining provides the ability for libssl to read or write multiple records in parallel. It requires special ciphers to do this, and there are currently no built-in ciphers that provide this capability. However, the dasync engine does have such a cipher, so we add a test for this capability using that engine. --- test/sslapitest.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 203 insertions(+) diff --git a/test/sslapitest.c b/test/sslapitest.c index f891e646d..aa65d2cd4 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -33,6 +33,7 @@ #include #include #include +#include #include "helpers/ssltestlib.h" #include "testutil.h" @@ -9914,6 +9915,205 @@ static int test_load_dhfile(void) #endif } +#if !defined(OPENSSL_NO_TLS1_2) && !defined(OPENSSL_NO_DYNAMIC_ENGINE) +/* + * Test TLSv1.2 with a pipeline capable cipher. TLSv1.3 and DTLS do not + * support this yet. The only pipeline capable cipher that we have is in the + * dasync engine (providers don't support this yet), so we have to use + * deprecated APIs for this test. + * + * Test 0: Client has pipelining enabled, server does not + * Test 1: Server has pipelining enabled, client does not + * Test 2: Client has pipelining enabled, server does not: not enough data to + * fill all the pipelines + * Test 3: Client has pipelining enabled, server does not: not enough data to + * fill all the pipelines by more than a full pipeline's worth + * Test 4: Client has pipelining enabled, server does not: more data than all + * the available pipelines can take + * Test 5: Client has pipelining enabled, server does not: Maximum size pipeline + * Test 6: Repeat of test 0, but the engine is loaded late (after the SSL_CTX + * is created) + */ +static int test_pipelining(int idx) +{ + SSL_CTX *cctx = NULL, *sctx = NULL; + SSL *clientssl = NULL, *serverssl = NULL, *peera, *peerb; + int testresult = 0, numreads; + /* A 55 byte message */ + unsigned char *msg = (unsigned char *) + "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz123"; + size_t written, readbytes, offset, msglen, fragsize = 10, numpipes = 5; + size_t expectedreads; + unsigned char *buf = NULL; + ENGINE *e = NULL; + + if (idx != 6) { + e = load_dasync(); + if (e == NULL) + return 0; + } + + if (!TEST_true(create_ssl_ctx_pair(libctx, TLS_server_method(), + TLS_client_method(), 0, + TLS1_2_VERSION, &sctx, &cctx, cert, + privkey))) + goto end; + + if (idx == 6) { + e = load_dasync(); + if (e == NULL) + goto end; + /* Now act like test 0 */ + idx = 0; + } + + if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl, + &clientssl, NULL, NULL))) + goto end; + + if (!TEST_true(SSL_set_cipher_list(clientssl, "AES128-SHA"))) + goto end; + + /* peera is always configured for pipelining, while peerb is not. */ + if (idx == 1) { + peera = serverssl; + peerb = clientssl; + + } else { + peera = clientssl; + peerb = serverssl; + } + + if (idx == 5) { + numpipes = 2; + /* Maximum allowed fragment size */ + fragsize = SSL3_RT_MAX_PLAIN_LENGTH; + msglen = fragsize * numpipes; + msg = OPENSSL_malloc(msglen); + if (!TEST_ptr(msg)) + goto end; + if (!TEST_int_gt(RAND_bytes_ex(libctx, msg, msglen, 0), 0)) + goto end; + } else if (idx == 4) { + msglen = 55; + } else { + msglen = 50; + } + if (idx == 2) + msglen -= 2; /* Send 2 less bytes */ + else if (idx == 3) + msglen -= 12; /* Send 12 less bytes */ + + buf = OPENSSL_malloc(msglen); + if (!TEST_ptr(buf)) + goto end; + + if (idx == 5) { + /* + * Test that setting a split send fragment longer than the maximum + * allowed fails + */ + if (!TEST_false(SSL_set_split_send_fragment(peera, fragsize + 1))) + goto end; + } + + /* + * In the normal case. We have 5 pipelines with 10 bytes per pipeline + * (50 bytes in total). This is a ridiculously small number of bytes - + * but sufficient for our purposes + */ + if (!TEST_true(SSL_set_max_pipelines(peera, numpipes)) + || !TEST_true(SSL_set_split_send_fragment(peera, fragsize))) + goto end; + + if (!TEST_true(create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE))) + goto end; + + /* Write some data from peera to peerb */ + if (!TEST_true(SSL_write_ex(peera, msg, msglen, &written)) + || !TEST_size_t_eq(written, msglen)) + goto end; + + /* + * If the pipelining code worked, then we expect all |numpipes| pipelines to + * have been used - except in test 3 where only |numpipes - 1| pipelines + * will be used. This will result in |numpipes| records (|numpipes - 1| for + * test 3) having been sent to peerb. Since peerb is not using read_ahead we + * expect this to be read in |numpipes| or |numpipes - 1| separate + * SSL_read_ex calls. In the case of test 4, there is then one additional + * read for left over data that couldn't fit in the previous pipelines + */ + for (offset = 0, numreads = 0; + offset < msglen; + offset += readbytes, numreads++) { + if (!TEST_true(SSL_read_ex(peerb, buf + offset, + msglen - offset, &readbytes))) + goto end; + } + + expectedreads = idx == 4 ? numpipes + 1 + : (idx == 3 ? numpipes - 1 : numpipes); + if (!TEST_mem_eq(msg, msglen, buf, offset) + || !TEST_int_eq(numreads, expectedreads)) + goto end; + + /* + * Write some data from peerb to peera. We do this in up to |numpipes + 1| + * chunks to exercise the read pipelining code on peera. + */ + for (offset = 0; offset < msglen; offset += fragsize) { + size_t sendlen = msglen - offset; + + if (sendlen > fragsize) + sendlen = fragsize; + if (!TEST_true(SSL_write_ex(peerb, msg + offset, sendlen, &written)) + || !TEST_size_t_eq(written, sendlen)) + goto end; + } + + /* + * The data was written in |numpipes|, |numpipes - 1| or |numpipes + 1| + * separate chunks (depending on which test we are running). If the + * pipelining is working then we expect peera to read up to numpipes chunks + * and process them in parallel, giving back the complete result in a single + * call to SSL_read_ex + */ + if (!TEST_true(SSL_read_ex(peera, buf, msglen, &readbytes)) + || !TEST_size_t_le(readbytes, msglen)) + goto end; + + if (idx == 4) { + size_t readbytes2; + + if (!TEST_true(SSL_read_ex(peera, buf + readbytes, + msglen - readbytes, &readbytes2))) + goto end; + readbytes += readbytes2; + if (!TEST_size_t_le(readbytes, msglen)) + goto end; + } + + if (!TEST_mem_eq(msg, msglen, buf, readbytes)) + goto end; + + testresult = 1; +end: + SSL_free(serverssl); + SSL_free(clientssl); + SSL_CTX_free(sctx); + SSL_CTX_free(cctx); + if (e != NULL) { + ENGINE_unregister_ciphers(e); + ENGINE_finish(e); + ENGINE_free(e); + } + OPENSSL_free(buf); + if (fragsize == SSL3_RT_MAX_PLAIN_LENGTH) + OPENSSL_free(msg); + return testresult; +} +#endif /* !defined(OPENSSL_NO_TLS1_2) && !defined(OPENSSL_NO_DYNAMIC_ENGINE) */ + #ifndef OPENSSL_NO_CERT_COMPRESSION #define CERT_COMPRESSION_XOR 16384 #define CERT_COMPRESSION_ROL 65535 @@ -10604,6 +10804,9 @@ int setup_tests(void) ADD_ALL_TESTS(test_session_cache_overflow, 4); #endif ADD_TEST(test_load_dhfile); +#if !defined(OPENSSL_NO_TLS1_2) && !defined(OPENSSL_NO_DYNAMIC_ENGINE) + ADD_ALL_TESTS(test_pipelining, 7); +#endif ADD_ALL_TESTS(test_multi_resume, 5); return 1; From 62432de0c7f56e5fe7b712e9ec879c694a18d678 Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 17:56:56 +0800 Subject: [PATCH 08/11] Pipeline output/input buf arrays must live until the EVP_Cipher is called The pipeline input/output buf arrays must remain accessible to the EVP_CIPHER_CTX until EVP_Cipher is subsequently called. This fixes an asan error discovered by the newly added pipeline test. --- ssl/record/ssl3_record.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index 08d50bc6a..8b8527fcd 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -982,6 +982,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending, EVP_CIPHER_CTX *ds; size_t reclen[SSL_MAX_PIPELINES]; unsigned char buf[SSL_MAX_PIPELINES][EVP_AEAD_TLS1_AAD_LEN]; + unsigned char *data[SSL_MAX_PIPELINES]; int i, pad = 0, tmpr; size_t bs, ctr, padnum, loop; unsigned char padval; @@ -1139,8 +1140,6 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending, } } if (n_recs > 1) { - unsigned char *data[SSL_MAX_PIPELINES]; - /* Set the output buffers */ for (ctr = 0; ctr < n_recs; ctr++) { data[ctr] = recs[ctr].data; From 2c91a4a7fdaaef69e1cce4c09b88300d92e86f6a Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 17:58:06 +0800 Subject: [PATCH 09/11] 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. --- ssl/record/rec_layer_s3.c | 20 +------------------- ssl/record/ssl3_buffer.c | 5 +++++ 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 3d57380a9..ab4bf5876 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -212,25 +212,7 @@ int ssl3_read_n(SSL *s, size_t n, size_t max, int extend, int clearold, /* start with empty packet ... */ 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; - } - } + s->rlayer.packet = rb->buf + rb->offset; s->rlayer.packet_length = 0; /* ... now we can act as if 'extend' was set */ diff --git a/ssl/record/ssl3_buffer.c b/ssl/record/ssl3_buffer.c index bdfb5b740..e08666c5b 100644 --- a/ssl/record/ssl3_buffer.c +++ b/ssl/record/ssl3_buffer.c @@ -58,6 +58,11 @@ int ssl3_setup_read_buffer(SSL *s) if (ssl_allow_compression(s)) len += SSL3_RT_MAX_COMPRESSED_OVERHEAD; #endif + + /* Ensure our buffer is large enough to support all our pipelines */ + if (s->max_pipelines > 1) + len *= s->max_pipelines; + if (b->default_len > len) len = b->default_len; if ((p = OPENSSL_malloc(len)) == NULL) { From ea0da0757abfb6d55c943351065fbeae94dfde49 Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 17:59:18 +0800 Subject: [PATCH 10/11] Do not have more data in a pipeline than the split_send_fragment We shouldn't be putting more data into a pipeline than the value of split_send_fragment. This is a backport of a fix which was included in a much larger commit in master (c6186792b98) related to moving the pipelining code into the new record layer that exists there. --- ssl/record/rec_layer_s3.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index ab4bf5876..3eb4fb6ab 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -611,14 +611,13 @@ int ssl3_write_bytes(SSL *s, int type, const void *buf_, size_t len, if (numpipes > maxpipes) numpipes = maxpipes; - if (n / numpipes >= max_send_fragment) { + if (n / numpipes >= split_send_fragment) { /* * We have enough data to completely fill all available * pipelines */ - for (j = 0; j < numpipes; j++) { - pipelens[j] = max_send_fragment; - } + for (j = 0; j < numpipes; j++) + pipelens[j] = split_send_fragment; } else { /* We can partially fill all available pipelines */ tmppipelen = n / numpipes; From 0a02ab816c4d047f6a4a372b42b712087f3374e1 Mon Sep 17 00:00:00 2001 From: K1 Date: Tue, 25 Jun 2024 20:03:42 +0800 Subject: [PATCH 11/11] Don't attempt to set provider params on an ENGINE based cipher If an ENGINE has been loaded after the SSL_CTX has been created then the cipher we have cached might be provider based, but the cipher we actually end up using might not be. Don't try to set provider params on a cipher that is actually ENGINE based. --- ssl/s3_enc.c | 6 +++++- ssl/t1_enc.c | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c index 2ca3f74ae..ee4f58e75 100644 --- a/ssl/s3_enc.c +++ b/ssl/s3_enc.c @@ -225,7 +225,11 @@ int ssl3_change_cipher_state(SSL *s, int which) goto err; } - if (EVP_CIPHER_get0_provider(c) != NULL + /* + * The cipher we actually ended up using in the EVP_CIPHER_CTX may be + * different to that in c if we have an ENGINE in use + */ + if (EVP_CIPHER_get0_provider(EVP_CIPHER_CTX_get0_cipher(dd)) != NULL && !tls_provider_set_tls_params(s, dd, c, m)) { /* SSLfatal already called */ goto err; diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c index 2b449e1bc..67b45905d 100644 --- a/ssl/t1_enc.c +++ b/ssl/t1_enc.c @@ -397,7 +397,12 @@ int tls1_change_cipher_state(SSL *s, int which) SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } - if (EVP_CIPHER_get0_provider(c) != NULL + + /* + * The cipher we actually ended up using in the EVP_CIPHER_CTX may be + * different to that in c if we have an ENGINE in use + */ + if (EVP_CIPHER_get0_provider(EVP_CIPHER_CTX_get0_cipher(dd)) != NULL && !tls_provider_set_tls_params(s, dd, c, m)) { /* SSLfatal already called */ goto err;