From c8f3a8f14b7ccbd2617cf1c6091c0185fd805c53 Mon Sep 17 00:00:00 2001 From: Marco Oliverio <3876480+rizlik@users.noreply.github.com> Date: Thu, 15 Feb 2024 22:48:19 +0100 Subject: [PATCH] fix: negotiate handshake until the end in wolfSSL_read/wolfSSL_write (#7237) * tls: negotiate until hs is complete in wolfSSL_read/wolfSSL_write Don't rely on ssl->options.handShakeSate == HANDSHAKE_DONE to check if negotiation is needed. wolfSSL_Connect() or wolfSSL_Accept() job may not yet be completed and/or some messages may be waiting in the buffer because of non-blocking I/O. * tests: test case for handshake with wolfSSL_read()/wolfSSL_write() * doc: clarify wolfSSL_write() * internal.c: rename: need_negotiate -> ssl_in_handshake --- doc/dox_comments/header_files/ssl.h | 21 ++--- src/internal.c | 62 +++++++++++---- tests/api.c | 116 ++++++++++++++++++++++++++++ 3 files changed, 176 insertions(+), 23 deletions(-) diff --git a/doc/dox_comments/header_files/ssl.h b/doc/dox_comments/header_files/ssl.h index b8e7d3df0..fbb1e2c7d 100644 --- a/doc/dox_comments/header_files/ssl.h +++ b/doc/dox_comments/header_files/ssl.h @@ -2086,15 +2086,18 @@ int wolfSSL_get_using_nonblock(WOLFSSL*); \brief This function writes sz bytes from the buffer, data, to the SSL connection, ssl. If necessary, wolfSSL_write() will negotiate an SSL/TLS session if the handshake has not already been performed yet by - wolfSSL_connect() or wolfSSL_accept(). wolfSSL_write() works with both - blocking and non-blocking I/O. When the underlying I/O is non-blocking, - wolfSSL_write() will return when the underlying I/O could not satisfy the - needs of wolfSSL_write() to continue. In this case, a call to - wolfSSL_get_error() will yield either SSL_ERROR_WANT_READ or - SSL_ERROR_WANT_WRITE. The calling process must then repeat the call to - wolfSSL_write() when the underlying I/O is ready. If the underlying I/O - is blocking, wolfSSL_write() will only return once the buffer data of - size sz has been completely written or an error occurred. + wolfSSL_connect() or wolfSSL_accept(). When using (D)TLSv1.3 and early data + feature is compiled in, this function progresses the handshake only up to + the point when it is possible to send data. Next invokations of + wolfSSL_Connect()/wolfSSL_Accept()/wolfSSL_read() will complete the + handshake. wolfSSL_write() works with both blocking and non-blocking I/O. + When the underlying I/O is non-blocking, wolfSSL_write() will return when + the underlying I/O could not satisfy the needs of wolfSSL_write() to + continue. In this case, a call to wolfSSL_get_error() will yield either + SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE. The calling process must then + repeat the call to wolfSSL_write() when the underlying I/O is ready. If the + underlying I/O is blocking, wolfSSL_write() will only return once the buffer + data of size sz has been completely written or an error occurred. \return >0 the number of bytes written upon success. \return 0 will be returned upon failure. Call wolfSSL_get_error() for diff --git a/src/internal.c b/src/internal.c index e08545dde..480f567b2 100644 --- a/src/internal.c +++ b/src/internal.c @@ -24040,6 +24040,52 @@ static int CheckTLS13AEADSendLimit(WOLFSSL* ssl) } #endif /* WOLFSSL_TLS13 && !WOLFSSL_TLS13_IGNORE_AEAD_LIMITS */ +/** + * ssl_in_handshake(): + * Invoked in wolfSSL_read/wolfSSL_write to check if wolfSSL_negotiate() is + * needed in the handshake. + * + * In TLSv1.2 negotiate until the end of the handshake, unless: + * 1 in SCR and sending data or + * 2 in SCR and we have plain data ready + * Early data logic may bypass this logic in TLSv1.3 when appropriate. + */ +static int ssl_in_handshake(WOLFSSL *ssl, int send) +{ + if (IsSCR(ssl)) { + if (send) { + /* allow sending data in SCR */ + return 0; + } else { + /* allow reading buffered data in SCR */ + if (ssl->buffers.clearOutputBuffer.length != 0) + return 0; + } + return 1; + } + + if (ssl->options.handShakeState != HANDSHAKE_DONE) + return 1; + + if (ssl->options.side == WOLFSSL_SERVER_END) { + if (IsAtLeastTLSv1_3(ssl->version)) + return ssl->options.acceptState < TLS13_TICKET_SENT; + if (IsAtLeastTLSv1_2(ssl)) + return ssl->options.acceptState < ACCEPT_THIRD_REPLY_DONE; + return 0; + } + + if (ssl->options.side == WOLFSSL_CLIENT_END) { + if (IsAtLeastTLSv1_3(ssl->version)) + return ssl->options.connectState < FINISHED_DONE; + if (IsAtLeastTLSv1_2(ssl)) + return ssl->options.connectState < SECOND_REPLY_DONE; + return 0; + } + + return 0; +} + int SendData(WOLFSSL* ssl, const void* data, int sz) { int sent = 0, /* plainText size */ @@ -24091,7 +24137,7 @@ int SendData(WOLFSSL* ssl, const void* data, int sz) } else #endif - if (ssl->options.handShakeState != HANDSHAKE_DONE && !IsSCR(ssl)) { + if (ssl_in_handshake(ssl, 1)) { int err; WOLFSSL_MSG("handshake not complete, trying to finish"); if ( (err = wolfSSL_negotiate(ssl)) != WOLFSSL_SUCCESS) { @@ -24343,19 +24389,7 @@ int ReceiveData(WOLFSSL* ssl, byte* output, int sz, int peek) else #endif { - int negotiate = 0; -#ifdef HAVE_SECURE_RENEGOTIATION - if (ssl->secure_renegotiation && ssl->secure_renegotiation->enabled) { - if (ssl->options.handShakeState != HANDSHAKE_DONE - && ssl->buffers.clearOutputBuffer.length == 0) - negotiate = 1; - } - else -#endif - if (ssl->options.handShakeState != HANDSHAKE_DONE) - negotiate = 1; - - if (negotiate) { + if (ssl_in_handshake(ssl, 0)) { int err; WOLFSSL_MSG("Handshake not complete, trying to finish"); if ( (err = wolfSSL_negotiate(ssl)) != WOLFSSL_SUCCESS) { diff --git a/tests/api.c b/tests/api.c index b11a43e17..0461046a1 100644 --- a/tests/api.c +++ b/tests/api.c @@ -69676,6 +69676,121 @@ static int test_write_dup(void) return EXPECT_RESULT(); } +static int test_read_write_hs(void) +{ + + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(WOLFSSL_NO_TLS12) + WOLFSSL_CTX *ctx_s = NULL, *ctx_c = NULL; + WOLFSSL *ssl_s = NULL, *ssl_c = NULL; + struct test_memio_ctx test_ctx; + uint8_t test_buffer[16]; + unsigned int test; + + /* test == 0 : client writes, server reads */ + /* test == 1 : server writes, client reads */ + for (test = 0; test < 2; test++) { + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_2_client_method, + wolfTLSv1_2_server_method), 0); + ExpectIntEQ(wolfSSL_set_group_messages(ssl_s), WOLFSSL_SUCCESS); + /* CH -> */ + if (test == 0) { + ExpectIntEQ(wolfSSL_write(ssl_c, "hello", 5), -1); + } else { + ExpectIntEQ(wolfSSL_read(ssl_c, test_buffer, + sizeof(test_buffer)), -1); + } + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + + /* <- SH + SKE + SHD */ + if (test == 0) { + ExpectIntEQ(wolfSSL_read(ssl_s, test_buffer, + sizeof(test_buffer)), -1); + } else { + ExpectIntEQ(wolfSSL_write(ssl_s, "hello", 5), -1); + } + ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_READ); + + /* -> CKE + CLIENT FINISHED */ + if (test == 0) { + ExpectIntEQ(wolfSSL_write(ssl_c, "hello", 5), -1); + } else { + ExpectIntEQ(wolfSSL_read(ssl_c, test_buffer, + sizeof(test_buffer)), -1); + } + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + + /* abide clang static analyzer */ + if (ssl_s != NULL) { + /* disable group message to separate sending of ChangeCipherspec + * from Finished */ + ssl_s->options.groupMessages = 0; + } + /* allow writing of CS, but not FINISHED */ + test_ctx.c_len = TEST_MEMIO_BUF_SZ - 6; + + /* <- CS */ + if (test == 0) { + ExpectIntEQ(wolfSSL_read(ssl_s, test_buffer, + sizeof(test_buffer)), -1); + } else { + ExpectIntEQ(wolfSSL_write(ssl_s, "hello", 5), -1); + } + ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_WRITE); + + /* move CS message where the client can read it */ + memmove(test_ctx.c_buff, + (test_ctx.c_buff + TEST_MEMIO_BUF_SZ - 6), 6); + test_ctx.c_len = 6; + /* read CS */ + if (test == 0) { + ExpectIntEQ(wolfSSL_write(ssl_c, "hello", 5), -1); + } else { + ExpectIntEQ(wolfSSL_read(ssl_c, test_buffer, + sizeof(test_buffer)), -1); + } + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + ExpectIntEQ(test_ctx.c_len, 0); + + if (test == 0) { + /* send SERVER FINISHED */ + ExpectIntEQ(wolfSSL_read(ssl_s, test_buffer, + sizeof(test_buffer)), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), + WOLFSSL_ERROR_WANT_READ); + } else { + /* send SERVER FINISHED + App Data */ + ExpectIntEQ(wolfSSL_write(ssl_s, "hello", 5), 5); + } + + ExpectIntGT(test_ctx.c_len, 0); + + /* Send and receive the data */ + if (test == 0) { + ExpectIntEQ(wolfSSL_write(ssl_c, "hello", 5), 5); + ExpectIntEQ(wolfSSL_read(ssl_s, test_buffer, + sizeof(test_buffer)), 5); + } else { + ExpectIntEQ(wolfSSL_read(ssl_c, test_buffer, + sizeof(test_buffer)), 5); + } + + ExpectBufEQ(test_buffer, "hello", 5); + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); + ssl_c = ssl_s = NULL; + ctx_c = ctx_s = NULL; + } + +#endif + return EXPECT_RESULT(); +} + /*----------------------------------------------------------------------------* | Main *----------------------------------------------------------------------------*/ @@ -70983,6 +71098,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_tls13_early_data), TEST_DECL(test_tls_multi_handshakes_one_record), TEST_DECL(test_write_dup), + TEST_DECL(test_read_write_hs), /* This test needs to stay at the end to clean up any caches allocated. */ TEST_DECL(test_wolfSSL_Cleanup) };