From 4d01669b271adc22857e3b2bd7243033afc08b66 Mon Sep 17 00:00:00 2001 From: Andrew Hopkins Date: Mon, 1 Apr 2024 16:11:18 -0700 Subject: [PATCH] Support reading additional data from underlying BIO for each call to SSL_read: * SSL_set_read_ahead * SSL_CTX_set_read_ahead * SSL_set_default_read_buffer_len * SSL_CTX_set_default_read_buffer_len By enabling read ahead the system may reduce the number of calls to the SSL read BIO by reading more data than is specified in the TLS record header. --- include/openssl/ssl.h | 60 ++++++-- ssl/internal.h | 18 +++ ssl/ssl_buffer.cc | 31 +++- ssl/ssl_lib.cc | 67 +++++++-- ssl/ssl_test.cc | 138 +++++++++++++----- ssl/test/bssl_shim.cc | 5 + ssl/test/runner/common.go | 3 + ssl/test/runner/conn.go | 5 + ssl/test/runner/runner.go | 44 +++++- .../runner/ssl_transfer/test_case_names.txt | 2 + ssl/test/test_config.cc | 2 + ssl/test/test_config.h | 2 + 12 files changed, 307 insertions(+), 70 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 7fb69fb5faf..d2035c5cfd5 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -409,11 +409,16 @@ OPENSSL_EXPORT int SSL_pending(const SSL *ssl); // for read, or if |ssl| has buffered data from the transport that has not yet // been decrypted. If |ssl| has neither, this function returns zero. // -// In TLS, BoringSSL does not implement read-ahead, so this function returns one -// if and only if |SSL_pending| would return a non-zero value. In DTLS, it is -// possible for this function to return one while |SSL_pending| returns zero. -// For example, |SSL_read| may read a datagram with two records, decrypt the -// first, and leave the second buffered for a subsequent call to |SSL_read|. +// If read-ahead has been enabled with |SSL_CTX_set_read_ahead| or +// |SSL_set_read_ahead|, the behavior of |SSL_pending| will change, it may return +// 1 and a call to |SSL_read| to return no data. This can happen when a partial +// record has been read but can not be decrypted without more data from the read +// BIO. +// +// In DTLS, it is possible for this function to return one while |SSL_pending| +// returns zero. For example, |SSL_read| may read a datagram with two records, +// decrypt the first, and leave the second buffered for a subsequent call to +// |SSL_read|. // // As a result, if this function returns one, the next call to |SSL_read| may // still fail, read from the transport, or both. The buffered, undecrypted data @@ -4652,7 +4657,7 @@ OPENSSL_EXPORT int SSL_CTX_set_max_send_fragment(SSL_CTX *ctx, // SSL_set_max_send_fragment sets the maximum length, in bytes, of records sent // by |ssl|. Beyond this length, handshake messages and application data will // be split into multiple records. It returns one on success or zero on -// error. +// error. The minimum is 512, and the max is 16384. OPENSSL_EXPORT int SSL_set_max_send_fragment(SSL *ssl, size_t max_send_fragment); @@ -5070,18 +5075,53 @@ OPENSSL_EXPORT int SSL_CTX_set_tmp_rsa(SSL_CTX *ctx, const RSA *rsa); // SSL_set_tmp_rsa returns one. OPENSSL_EXPORT int SSL_set_tmp_rsa(SSL *ssl, const RSA *rsa); -// SSL_CTX_get_read_ahead returns zero. +// SSL_CTX_get_read_ahead returns 1 if read ahead is enabled, and 0 otherwise. OPENSSL_EXPORT int SSL_CTX_get_read_ahead(const SSL_CTX *ctx); -// SSL_CTX_set_read_ahead returns one. +// SSL_CTX_set_read_ahead enables or disables read ahead on |ctx|: +// if |yes| is 1 it enables read ahead and returns 1, +// if |yes| is 0 it disables read ahead and returns 1, +// if |yes| is any other value nothing is changed and 0 is returned. +// +// When read ahead is enabled all future reads will be up to the buffer size configured +// with |SSL_CTX_set_default_read_buffer_len|, the default buffer size is +// |SSL3_RT_MAX_PLAIN_LENGTH| + |SSL3_RT_MAX_ENCRYPTED_OVERHEAD| = 16704 bytes. +// +// Read ahead should only be enabled on non-blocking IO sources configured with |SSL_set_bio|. +// When read ahead is enabled AWS-LC will make reads for potentially more data than is +// avaliable in the BIO with the assumption a partial read will be returned. If +// a blocking BIO is used and never returns the read could get stuck forever. OPENSSL_EXPORT int SSL_CTX_set_read_ahead(SSL_CTX *ctx, int yes); -// SSL_get_read_ahead returns zero. +// SSL_get_read_ahead returns 1 if read ahead is enabled or 0 if it is not. OPENSSL_EXPORT int SSL_get_read_ahead(const SSL *ssl); -// SSL_set_read_ahead returns one. +// SSL_set_read_ahead enables or disables read ahead on |ssl|: +// if |yes| is 1 it enables read ahead and returns 1, +// if |yes| is 0 it disables read ahead and returns 1, +// if |yes| is any other value nothing is changed and 0 is returned. +// +// When read ahead is enabled all future reads will be for up to the buffer size configured +// with |SSL_CTX_set_default_read_buffer_len|. The default buffer size is +// |SSL3_RT_MAX_PLAIN_LENGTH| + |SSL3_RT_MAX_ENCRYPTED_OVERHEAD| = 16704 bytes +// +// Read ahead should only be enabled on non-blocking IO sources configured with |SSL_set_bio|, +// when read ahead is enabled AWS-LC will make reads for potentially more data than is +// available in the BIO. OPENSSL_EXPORT int SSL_set_read_ahead(SSL *ssl, int yes); +// SSL_CTX_set_default_read_buffer_len sets the size of the buffer reads will use on +// |ctx| if read ahead has been enabled. 0 is the minimum and 65535 is the maximum. +// A |len| of 0 is the default behavior with read ahead turned off: each call to +// |SSL_read| reads the amount specified in the TLS Record Header. +OPENSSL_EXPORT void SSL_CTX_set_default_read_buffer_len(SSL_CTX *ctx, size_t len); + +// SSL_CTX_set_default_read_buffer_len sets the size of the buffer reads will use on +// |ssl| if read ahead has been enabled. 0 is the minimum and 65535 is the maximum. +// A |len| of 0 is the default behavior with read ahead turned off: each call to +// |SSL_read| reads the amount specified in the TLS Record Header. +OPENSSL_EXPORT void SSL_set_default_read_buffer_len(SSL *ssl, size_t len); + // SSL_set_state does nothing. OPENSSL_EXPORT void SSL_set_state(SSL *ssl, int state); diff --git a/ssl/internal.h b/ssl/internal.h index c845683dc1f..2834d059026 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1312,6 +1312,8 @@ void ssl_do_msg_callback(const SSL *ssl, int is_write, int content_type, // Transport buffers. +#define SSLBUFFER_READ_AHEAD_MIN_CAPACITY 512 +#define SSLBUFFER_MAX_CAPACITY UINT16_MAX class SSLBuffer { public: SSLBuffer() {} @@ -3658,6 +3660,7 @@ struct ssl_method_st { const bssl::SSL_X509_METHOD *x509_method; }; +#define MIN_SAFE_FRAGMENT_SIZE 512 struct ssl_ctx_st { explicit ssl_ctx_st(const SSL_METHOD *ssl_method); ssl_ctx_st(const ssl_ctx_st &) = delete; @@ -3684,6 +3687,9 @@ struct ssl_ctx_st { /// in case the client makes several connections before getting a renewal. uint8_t num_tickets = 2; + // read_ahead_buffer_size is the amount of data to read if |enable_read_ahead| is true + size_t read_ahead_buffer_size = SSL3_RT_MAX_PLAIN_LENGTH + SSL3_RT_MAX_ENCRYPTED_OVERHEAD; + // quic_method is the method table corresponding to the QUIC hooks. const SSL_QUIC_METHOD *quic_method = nullptr; @@ -3981,6 +3987,11 @@ struct ssl_ctx_st { // If enable_early_data is true, early data can be sent and accepted. bool enable_early_data : 1; + // enable_read_ahead indicates whether the |SSL_CTX| is configured to read as much + // as will fit in the SSLBuffer from the BIO, or just enough to read the record + // header and then the length of the body + bool enable_read_ahead : 1; + // aes_hw_override if set indicates we should override checking for AES // hardware support, and use the value in aes_hw_override_value instead. bool aes_hw_override : 1; @@ -4030,6 +4041,8 @@ struct ssl_st { uint16_t max_send_fragment = 0; + size_t read_ahead_buffer_size = SSL3_RT_MAX_PLAIN_LENGTH + SSL3_RT_MAX_ENCRYPTED_OVERHEAD; + // There are 2 BIO's even though they are normally both the same. This is so // data can be read and written to different handlers @@ -4101,6 +4114,11 @@ struct ssl_st { // If enable_early_data is true, early data can be sent and accepted. bool enable_early_data : 1; + + // enable_read_ahead indicates whether the |SSL_CTX| is configured to read as much + // as will fit in the SSLBuffer from the BIO, or just enough to read the record + // header and then the length of the body + bool enable_read_ahead : 1; }; struct ssl_session_st { diff --git a/ssl/ssl_buffer.cc b/ssl/ssl_buffer.cc index e8f772191ac..7b7a38dc802 100644 --- a/ssl/ssl_buffer.cc +++ b/ssl/ssl_buffer.cc @@ -31,7 +31,7 @@ BSSL_NAMESPACE_BEGIN // BIO uses int instead of size_t. No lengths will exceed uint16_t, so this will // not overflow. -static_assert(0xffff <= INT_MAX, "uint16_t does not fit in int"); +static_assert(SSLBUFFER_MAX_CAPACITY <= INT_MAX, "uint16_t does not fit in int"); static_assert((SSL3_ALIGN_PAYLOAD & (SSL3_ALIGN_PAYLOAD - 1)) == 0, "SSL3_ALIGN_PAYLOAD must be a power of 2"); @@ -49,7 +49,7 @@ void SSLBuffer::Clear() { } bool SSLBuffer::EnsureCap(size_t header_len, size_t new_cap) { - if (new_cap > 0xffff) { + if (new_cap > SSLBUFFER_MAX_CAPACITY) { OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); return false; } @@ -235,8 +235,16 @@ static int tls_read_buffer_extend_to(SSL *ssl, size_t len) { while (buf->size() < len) { // The amount of data to read is bounded by |buf->cap|, which must fit in an // int. + + // If enable_read_ahead we want to attempt to fill the entire buffer, but + // the while loop will bail once we have at least the requested len amount + // of data. If not enable_read_ahead, only read as much to get to len bytes, + // at this point we know len is less than the overall size of the buffer. + size_t read_amount = ssl->enable_read_ahead ? buf->cap() - buf->size() : + len - buf->size(); + assert(read_amount <= buf->cap() - buf->size()); int ret = BIO_read(ssl->rbio.get(), buf->data() + buf->size(), - static_cast(len - buf->size())); + static_cast(read_amount)); if (ret <= 0) { ssl->s3->rwstate = SSL_ERROR_WANT_READ; return ret; @@ -250,17 +258,24 @@ static int tls_read_buffer_extend_to(SSL *ssl, size_t len) { int ssl_read_buffer_extend_to(SSL *ssl, size_t len) { // |ssl_read_buffer_extend_to| implicitly discards any consumed data. ssl->s3->read_buffer.DiscardConsumed(); - + size_t buffer_size = len; if (SSL_is_dtls(ssl)) { static_assert( - DTLS1_RT_HEADER_LENGTH + SSL3_RT_MAX_ENCRYPTED_LENGTH <= 0xffff, + DTLS1_RT_HEADER_LENGTH + SSL3_RT_MAX_ENCRYPTED_LENGTH <= SSLBUFFER_MAX_CAPACITY, "DTLS read buffer is too large"); // The |len| parameter is ignored in DTLS. len = DTLS1_RT_HEADER_LENGTH + SSL3_RT_MAX_ENCRYPTED_LENGTH; + buffer_size = len; + } else { + if (ssl->ctx.get()->enable_read_ahead) { + // If we're reading ahead allocate read_ahead_buffer size or the requested + // len whichever is bigger + buffer_size = std::max(len, ssl->read_ahead_buffer_size); + } } - if (!ssl->s3->read_buffer.EnsureCap(ssl_record_prefix_len(ssl), len)) { + if (!ssl->s3->read_buffer.EnsureCap(ssl_record_prefix_len(ssl), buffer_size)) { return -1; } @@ -330,12 +345,12 @@ int ssl_handle_open_record(SSL *ssl, bool *out_retry, ssl_open_record_t ret, static_assert(SSL3_RT_HEADER_LENGTH * 2 + SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD * 2 + SSL3_RT_MAX_PLAIN_LENGTH <= - 0xffff, + SSLBUFFER_MAX_CAPACITY, "maximum TLS write buffer is too large"); static_assert(DTLS1_RT_HEADER_LENGTH + SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD + SSL3_RT_MAX_PLAIN_LENGTH <= - 0xffff, + SSLBUFFER_MAX_CAPACITY, "maximum DTLS write buffer is too large"); static int tls_write_buffer_flush(SSL *ssl) { diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index e48415bea0a..9a93a937fc5 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -540,6 +540,7 @@ ssl_ctx_st::ssl_ctx_st(const SSL_METHOD *ssl_method) false_start_allowed_without_alpn(false), handoff(false), enable_early_data(false), + enable_read_ahead(false), aes_hw_override(false), aes_hw_override_value(false), conf_max_version_use_default(true), @@ -599,6 +600,7 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *method) { // defer to the protocol method's default min/max values in that case. ret->conf_max_version_use_default = true; ret->conf_min_version_use_default = true; + ret->enable_read_ahead = false; return ret.release(); } @@ -620,6 +622,7 @@ void SSL_CTX_free(SSL_CTX *ctx) { ssl_st::ssl_st(SSL_CTX *ctx_arg) : method(ctx_arg->method), max_send_fragment(ctx_arg->max_send_fragment), + read_ahead_buffer_size(ctx_arg->read_ahead_buffer_size), msg_callback(ctx_arg->msg_callback), msg_callback_arg(ctx_arg->msg_callback_arg), ctx(UpRef(ctx_arg)), @@ -629,7 +632,8 @@ ssl_st::ssl_st(SSL_CTX *ctx_arg) max_cert_list(ctx->max_cert_list), server(false), quiet_shutdown(ctx->quiet_shutdown), - enable_early_data(ctx->enable_early_data) { + enable_early_data(ctx->enable_early_data), + enable_read_ahead(ctx->enable_read_ahead) { CRYPTO_new_ex_data(&ex_data); } @@ -1759,13 +1763,58 @@ int SSL_get_extms_support(const SSL *ssl) { return 0; } -int SSL_CTX_get_read_ahead(const SSL_CTX *ctx) { return 0; } +int SSL_CTX_get_read_ahead(const SSL_CTX *ctx) { + return ctx->enable_read_ahead; +} + +int SSL_get_read_ahead(const SSL *ssl) { + return ssl->enable_read_ahead; +} -int SSL_get_read_ahead(const SSL *ssl) { return 0; } +void SSL_CTX_set_default_read_buffer_len(SSL_CTX *ctx, size_t len) { + // SSLBUFFER_MAX_CAPACITY(0xffff) is the maximum SSLBuffer supports reading at one time + if (len > SSLBUFFER_MAX_CAPACITY) { + len = SSLBUFFER_MAX_CAPACITY; + } + // Setting a very small read buffer won't cause issue because the SSLBuffer + // will always read at least the amount of data specified in the TLS record + // header + ctx->read_ahead_buffer_size = len; +} -int SSL_CTX_set_read_ahead(SSL_CTX *ctx, int yes) { return 1; } +void SSL_set_default_read_buffer_len(SSL *ssl, size_t len) { + // SSLBUFFER_MAX_CAPACITY(0xffff) is the maximum SSLBuffer supports reading at one time + if (len > SSLBUFFER_MAX_CAPACITY) { + len = SSLBUFFER_MAX_CAPACITY; + } + // Setting a very small read buffer won't cause issue because the SSLBuffer + // will always read at least the amount of data specified in the TLS record + // header + ssl->read_ahead_buffer_size = len; +} + +int SSL_CTX_set_read_ahead(SSL_CTX *ctx, int yes) { + if (yes == 0) { + ctx->enable_read_ahead = 0; + return 1; + } else if (yes == 1) { + ctx->enable_read_ahead = 1; + return 1; + } else { + return 0; + } +} -int SSL_set_read_ahead(SSL *ssl, int yes) { return 1; } +int SSL_set_read_ahead(SSL *ssl, int yes) { + if (yes == 0) { + ssl->enable_read_ahead = 0; + return 1; + } else if (yes == 1) { + ssl->enable_read_ahead = 1; + return 1; + } else { + return 0; + }} int SSL_pending(const SSL *ssl) { return static_cast(ssl->s3->pending_app_data.size()); @@ -1876,8 +1925,8 @@ void SSL_set_max_cert_list(SSL *ssl, size_t max_cert_list) { } int SSL_CTX_set_max_send_fragment(SSL_CTX *ctx, size_t max_send_fragment) { - if (max_send_fragment < 512) { - max_send_fragment = 512; + if (max_send_fragment < MIN_SAFE_FRAGMENT_SIZE) { + max_send_fragment = MIN_SAFE_FRAGMENT_SIZE; } if (max_send_fragment > SSL3_RT_MAX_PLAIN_LENGTH) { max_send_fragment = SSL3_RT_MAX_PLAIN_LENGTH; @@ -1888,8 +1937,8 @@ int SSL_CTX_set_max_send_fragment(SSL_CTX *ctx, size_t max_send_fragment) { } int SSL_set_max_send_fragment(SSL *ssl, size_t max_send_fragment) { - if (max_send_fragment < 512) { - max_send_fragment = 512; + if (max_send_fragment < MIN_SAFE_FRAGMENT_SIZE) { + max_send_fragment = MIN_SAFE_FRAGMENT_SIZE; } if (max_send_fragment > SSL3_RT_MAX_PLAIN_LENGTH) { max_send_fragment = SSL3_RT_MAX_PLAIN_LENGTH; diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index fc4d2adaddc..a4a38addc58 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -3457,7 +3457,7 @@ static const uint8_t kTestName[] = { // SSLVersionTest executes its test cases under all available protocol versions. // Test cases call |Connect| to create a connection using context objects with // the protocol version fixed to the current version under test. -class SSLVersionTest : public ::testing::TestWithParam { +class SSLVersionTest : public ::testing::TestWithParam<::std::tuple> { protected: SSLVersionTest() : cert_(GetTestCertificate()), key_(GetTestKey()) {} @@ -3470,6 +3470,11 @@ class SSLVersionTest : public ::testing::TestWithParam { !SSL_CTX_set_max_proto_version(ctx.get(), version())) { return nullptr; } + + if (enable_read_ahead()) { + SSL_CTX_set_read_ahead(ctx.get(), 1); + SSL_CTX_set_default_read_buffer_len(ctx.get(), read_ahead_buffer_size()); + } return ctx; } @@ -3499,18 +3504,30 @@ class SSLVersionTest : public ::testing::TestWithParam { return connected; } + VersionParam getVersionParam() const { + return std::get<0>(GetParam()); + } + void TransferServerSSL() { - if (!GetParam().transfer_ssl) { + if (!getVersionParam().transfer_ssl) { return; } // |server_| is reset to hold the transferred SSL. TransferSSL(&server_, server_ctx_.get(), nullptr); } - uint16_t version() const { return GetParam().version; } + uint16_t version() const { return getVersionParam().version; } bool is_dtls() const { - return GetParam().ssl_method == VersionParam::is_dtls; + return getVersionParam().ssl_method == VersionParam::is_dtls; + } + + size_t read_ahead_buffer_size() const { + return std::get<1>(GetParam()); + } + + bool enable_read_ahead() const { + return read_ahead_buffer_size() != 0; } void CheckCounterInit() { @@ -3546,10 +3563,11 @@ class SSLVersionTest : public ::testing::TestWithParam { }; INSTANTIATE_TEST_SUITE_P(WithVersion, SSLVersionTest, - testing::ValuesIn(kAllVersions), - [](const testing::TestParamInfo &i) { - return i.param.name; - }); + testing::Combine(::testing::ValuesIn(kAllVersions), testing::Values(0, 128, 512, 8192, 65535)), + [](const testing::TestParamInfo<::std::tuple>& test_info) { + std::string test_name = std::string(std::get<0>(test_info.param).name) + "_BufferSize_"; + return test_name + std::to_string(std::get<1>(test_info.param)); + }); TEST_P(SSLVersionTest, SequenceNumber) { CheckCounterInit(); @@ -4863,7 +4881,7 @@ TEST_P(SSLVersionTest, SSLClearFailsWithShedding) { TEST_P(SSLVersionTest, SSLClientCiphers) { // Client ciphers ARE NOT SERIALIZED, so skip tests that rely on transfer or // serialization of |ssl| and accompanying objects under test. - if (GetParam().transfer_ssl) { + if (getVersionParam().transfer_ssl) { return; } @@ -5201,30 +5219,38 @@ TEST_P(SSLVersionTest, SSLWriteRetry) { ASSERT_EQ(SSL_write(client_.get(), data_longer + kChunkLen, kChunkLen), kChunkLen); } else { - // Otherwise, although the first half made it to the transport, the second - // half is blocked. - ASSERT_EQ(ret, -1); - ASSERT_EQ(SSL_get_error(client_.get(), -1), SSL_ERROR_WANT_WRITE); - - // Check the first half and make room for another record. - ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen); - ASSERT_EQ(OPENSSL_memcmp(buf, "hello", kChunkLen), 0); - count--; - - // Retrying with fewer bytes than previously attempted is an error. If the - // input length is less than the number of bytes successfully written, the - // check happens at a different point, with a different error. - // - // TODO(davidben): Should these cases use the same error? - ASSERT_EQ( - SSL_get_error(client_.get(), - SSL_write(client_.get(), data_longer, kChunkLen - 1)), - SSL_ERROR_SSL); - ASSERT_TRUE(ExpectSingleError(ERR_LIB_SSL, SSL_R_BAD_LENGTH)); - - // Complete the write with the correct retry. - ASSERT_EQ(SSL_write(client_.get(), data_longer, 2 * kChunkLen), - 2 * kChunkLen); + if (enable_read_ahead()) { + // The client and server are sharing a BIO_pair which by default only + // allows 17 * 1024 bytes to be buffered in the shared BIO. This test + // relies on the buffer being full here. But if the client is reading + // ahead it is pulling data out of the BIO_pair's buffer and into it's + // own SSLBuffer freeing up space for the write above + ASSERT_EQ(ret, 2 * kChunkLen); + } else { + // Otherwise, although the first half made it to the transport, the second + // half is blocked. + ASSERT_EQ(ret, -1); + ASSERT_EQ(SSL_get_error(client_.get(), -1), SSL_ERROR_WANT_WRITE); + // Check the first half and make room for another record. + ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen); + ASSERT_EQ(OPENSSL_memcmp(buf, "hello", kChunkLen), 0); + count--; + + // Retrying with fewer bytes than previously attempted is an error. If the + // input length is less than the number of bytes successfully written, the + // check happens at a different point, with a different error. + // + // TODO(davidben): Should these cases use the same error? + ASSERT_EQ( + SSL_get_error(client_.get(), + SSL_write(client_.get(), data_longer, kChunkLen - 1)), + SSL_ERROR_SSL); + ASSERT_TRUE(ExpectSingleError(ERR_LIB_SSL, SSL_R_BAD_LENGTH)); + + // Complete the write with the correct retry. + ASSERT_EQ(SSL_write(client_.get(), data_longer, 2 * kChunkLen), + 2 * kChunkLen); + } } // Drain the input and ensure everything was written correctly. @@ -6515,9 +6541,9 @@ TEST_P(SSLVersionTest, SSLPending) { // fixing either of these bugs, this test may need to be redone. EXPECT_EQ(1, SSL_has_pending(client_.get())); } else { - // In TLS, we do not overread, so |SSL_has_pending| should report no data is - // buffered. - EXPECT_EQ(0, SSL_has_pending(client_.get())); + // In TLS if read ahead is enabled the two records would also have been read + // in a single call. + EXPECT_EQ(enable_read_ahead(), SSL_has_pending(client_.get())); } ASSERT_EQ(2, SSL_read(client_.get(), buf, 2)); @@ -6562,7 +6588,9 @@ TEST_P(SSLVersionTest, SSLPendingEx) { if (is_dtls()) { EXPECT_EQ(1, SSL_has_pending(client_.get())); } else { - EXPECT_EQ(0, SSL_has_pending(client_.get())); + // In TLS if read ahead is enabled the two records would also have been read + // in a single call. + EXPECT_EQ(enable_read_ahead(), SSL_has_pending(client_.get())); } ASSERT_EQ(1, SSL_read_ex(client_.get(), buf, 2, &buf_len)); @@ -6583,6 +6611,42 @@ TEST_P(SSLVersionTest, SSLPendingEx) { ASSERT_EQ(0, SSL_write_ex(client_.get(), (void *)"", 0, nullptr)); } +TEST_P(SSLVersionTest, ReadAhead) { + ASSERT_TRUE(Connect()); + size_t buf_len; + std::string test_string = "Hello, world!"; + SCOPED_TRACE(read_ahead_buffer_size()); + SCOPED_TRACE(enable_read_ahead()); + SCOPED_TRACE(BIO_pending(client_.get()->rbio.get())); + + for (char & i : test_string) { + ASSERT_EQ(1, SSL_write_ex(server_.get(), &i, 1, &buf_len)); + } + + char *buf = new char[test_string.length()]; + size_t starting_size = BIO_pending(client_.get()->rbio.get()); + SCOPED_TRACE(starting_size); + + ASSERT_NE(0UL, starting_size); + ASSERT_EQ(1, SSL_read_ex(client_.get(), buf, 1, &buf_len)); + if (enable_read_ahead() || is_dtls()) { + // Even though we didn't request the full string (11 bytes) everything + // should be read from the BIO + if (read_ahead_buffer_size() > starting_size || is_dtls()) { + ASSERT_EQ(0UL, BIO_pending(client_.get()->rbio.get())); + } else { + // Depending on D/TLS version each record will be a different size and a + // variable but non-zero amount of data will remain + ASSERT_NE(0UL, BIO_pending(client_.get()->rbio.get())); + } + } else { + // Only the requested 1 byte + TLS record overhead should have been read, + // the remaining 12 letters each in its own record should be in the BIO + // not the SSLBuffer + ASSERT_NE(0UL, BIO_pending(client_.get()->rbio.get())); + } +} + // Test that post-handshake tickets consumed by |SSL_shutdown| are ignored. TEST(SSLTest, ShutdownIgnoresTickets) { bssl::UniquePtr ctx(CreateContextWithTestCertificate(TLS_method())); diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index c9002c95dd4..66f10c48593 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -1319,6 +1319,11 @@ static bool DoExchange(bssl::UniquePtr *out_session, } } + if (config->read_ahead_buffer_size > 0) { + SSL_set_read_ahead(ssl, 1); + SSL_set_default_read_buffer_len(ssl, config->read_ahead_buffer_size); + } + if (!config->is_server && !config->false_start && !config->implicit_handshake && // Session tickets are sent post-handshake in TLS 1.3. diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 0ed3ce7c010..85305dde563 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -1404,6 +1404,9 @@ type ProtocolBugs struct { // arbitrarily large. SendLargeRecords bool + // If set fragment any writes into smaller pieces + MaxRecordSize int + // NegotiateALPNAndNPN, if true, causes the server to negotiate both // ALPN and NPN in the same connetion. NegotiateALPNAndNPN bool diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go index a3251dc1000..1507ae6b224 100644 --- a/ssl/test/runner/conn.go +++ b/ssl/test/runner/conn.go @@ -1204,6 +1204,11 @@ func (c *Conn) doWriteRecord(typ recordType, data []byte) (n int, err error) { m = 6 } } + + if typ != recordTypeHandshake && c.config.Bugs.MaxRecordSize > 0 && c.config.Bugs.MaxRecordSize < m { + m = c.config.Bugs.MaxRecordSize + } + explicitIVLen := 0 explicitIVIsSeq := false first = false diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 35408d2ef74..ce43bc46068 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -3653,6 +3653,38 @@ read alert 1 0 shouldFail: true, expectedLocalError: "local error: record overflow", }, + { + // Test the TLS 1.2 server reading with a large read-ahead buffer and the + // client sending small records + testType: serverTest, + name: "ReadAheadBuffer10k-1-byte-fragment-TLS12", + config: Config{ + MaxVersion: VersionTLS12, + Bugs: ProtocolBugs{ + MaxRecordSize: 6, + }, + }, + messageLen: 20000, + flags: []string{ + "-read-ahead-buffer-size", "10000", + }, + }, + { + // Test the TLS 1.3 server reading with a large read-ahead buffer and the + // client sending small records + testType: serverTest, + name: "ReadAheadBuffer10k-1-byte-fragment-TLS13", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + MaxRecordSize: 6, + }, + }, + messageLen: 20000, + flags: []string{ + "-read-ahead-buffer-size", "10000", + }, + }, { // Test that handshake data is tightly packed in TLS 1.3. testType: serverTest, @@ -19907,9 +19939,9 @@ func addMultipleCertSlotTests() { VerifySignatureAlgorithms: allAlgorithms, }, flags: func() []string { - flags := append([]string{}, certificateSlotFlags...) - flags = append(flags, curveFlags...) - return flags + flags := append([]string{}, certificateSlotFlags...) + flags = append(flags, curveFlags...) + return flags }(), expectations: connectionExpectations{ peerSignatureAlgorithm: alg.id, @@ -19944,9 +19976,9 @@ func addMultipleCertSlotTests() { Certificates: []Certificate{rsaCertificate, ecdsaP256Certificate, ed25519Certificate}, }, flags: func() []string { - flags := append([]string{}, certificateSlotFlags...) - flags = append(flags, curveFlags...) - return flags + flags := append([]string{}, certificateSlotFlags...) + flags = append(flags, curveFlags...) + return flags }(), } if alg.id != signatureEd25519 { diff --git a/ssl/test/runner/ssl_transfer/test_case_names.txt b/ssl/test/runner/ssl_transfer/test_case_names.txt index 7c4f7fad0ad..0ef2c03c8d3 100644 --- a/ssl/test/runner/ssl_transfer/test_case_names.txt +++ b/ssl/test/runner/ssl_transfer/test_case_names.txt @@ -399,6 +399,8 @@ QUICTransportParams-NonQUICServer-ClientNeither-ServerStandard-TLS-TLS13 RSA-PSS-Default-Sign RSAKeyUsage-Server-WantSignature-GotSignature-TLS12 RSAKeyUsage-Server-WantSignature-GotSignature-TLS13 +ReadAheadBuffer10k-1-byte-fragment-TLS12 +ReadAheadBuffer10k-1-byte-fragment-TLS13 Renegotiate-Server-Forbidden Renegotiate-Server-NoExt-SCSV Resume-Server-BinderWrongLength diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index ab1fef091f1..8e3a0a25205 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -423,6 +423,8 @@ std::vector SortedFlags() { &TestConfig::early_write_after_message), BoolFlag("-check-ssl-transfer", &TestConfig::check_ssl_transfer), BoolFlag("-do-ssl-transfer", &TestConfig::do_ssl_transfer), + IntFlag("-read-ahead-buffer-size", + &TestConfig::read_ahead_buffer_size), StringFlag("-ssl-fuzz-seed-path-prefix", &TestConfig::ssl_fuzz_seed_path_prefix), StringFlag("-tls13-ciphersuites", &TestConfig::tls13_ciphersuites), StringPairVectorFlag("-multiple-certs-slot", &TestConfig::multiple_certs_slot), diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 98120e5c055..05f73b3ed73 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -206,6 +206,8 @@ struct TestConfig { // when do_ssl_transfer is false, no transfer will happen. // when do_ssl_transfer is true, transfer will happen if the ssl is server. bool do_ssl_transfer = false; + // When not zero this enables read ahead and sets the buffer to this size. + int read_ahead_buffer_size = 0; // When not empty, this prefix with random suffix is used to create a file // stores the output of |SSL_to_bytes|. std::string ssl_fuzz_seed_path_prefix;