diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 1db1712a098677b07f1b01dd51502e6e39daeec2..3c38bf78be01069c8b1f86c1499405c0a9de620e 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -81,6 +81,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) { @@ -239,6 +248,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, SSL_F_SSL3_READ_N, ERR_R_INTERNAL_ERROR); + return -1; + } + len = s->rlayer.packet_length; pkt = rb->buf + align; /* diff --git a/ssl/record/record.h b/ssl/record/record.h index af56206e07c97a5aa789518fc1a6de3e6c51ef38..513ab3988868334631a4747d7f96c42f584dabb7 100644 --- a/ssl/record/record.h +++ b/ssl/record/record.h @@ -197,6 +197,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/record/ssl3_buffer.c b/ssl/record/ssl3_buffer.c index fa597c274671631e7b2bb2a79ec29a52cbc30e4d..b8b91d13152b1b6807b14df9130643621841f93e 100644 --- a/ssl/record/ssl3_buffer.c +++ b/ssl/record/ssl3_buffer.c @@ -179,5 +179,7 @@ int ssl3_release_read_buffer(SSL *s) b = RECORD_LAYER_get_rbuf(&s->rlayer); OPENSSL_free(b->buf); b->buf = NULL; + s->rlayer.packet = NULL; + s->rlayer.packet_length = 0; return 1; } diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 2a44960fac9dabefd463ddb191e8f85de0d17720..00410a7385cf948453b769921701340fe9dea117 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -5279,6 +5279,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; } diff --git a/test/sslbuffertest.c b/test/sslbuffertest.c index b5f815f7dbc926bdbd54dfe732839e0a63f88b8d..f57db366308c90ff643c0706ae2f46424d2658fa 100644 --- a/test/sslbuffertest.c +++ b/test/sslbuffertest.c @@ -12,7 +12,7 @@ #include #include #include - +#include #include "../ssl/packet_local.h" #include "ssltestlib.h" @@ -157,6 +157,166 @@ int global_init(void) return 1; } +/* + * 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 + * 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[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; + + + /* + * 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. 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; + 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; + } + + 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; + } + /* + * Put back just the partial record (plus the whole initial record in + * the pipelining case) + */ + 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; + } + } + + /* + * 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); +#ifndef OPENSSL_NO_DYNAMIC_ENGINE + if (e != NULL) { + ENGINE_unregister_ciphers(e); + ENGINE_finish(e); + ENGINE_free(e); + } +#endif + return result; +} + int setup_tests(void) { char *cert, *pkey; @@ -173,6 +333,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; } diff --git a/test/ssltestlib.c b/test/ssltestlib.c index 422787b0f58284070b34b58960108f0e9f50fb13..b1da6866c0966e064041741d3f7cc4f6cb50932a 100644 --- a/test/ssltestlib.c +++ b/test/ssltestlib.c @@ -9,6 +9,7 @@ #include +#include #include "internal/nelem.h" #include "ssltestlib.h" #include "testutil.h" @@ -975,3 +976,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/ssltestlib.h b/test/ssltestlib.h index 8f0a1b5308c374826cb21723e51e569d1dfd5a72..38f97a8cbdb8f5332b622285972d3df265e38220 100644 --- a/test/ssltestlib.h +++ b/test/ssltestlib.h @@ -54,4 +54,5 @@ typedef struct mempacket_st MEMPACKET; DEFINE_STACK_OF(MEMPACKET) +ENGINE *load_dasync(void); #endif /* OSSL_TEST_SSLTESTLIB_H */