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

Author: rizlik

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

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) {

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)
 };