Don't flush the ClientHello if we're going to send early data

We'd like the first bit of early_data and the ClientHello to go in the
same TCP packet if at all possible to enable things like TCP Fast Open.
Also, if you're only going to send one block of early data then you also
don't need to worry about TCP_NODELAY.

Fixes #4783

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4802)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index fb113bab6d77cb1509b9340fdfc87ea2b0df44c8..61e5ebb2d111040e465596b2a986df5daa77a345 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1969,6 +1969,7 @@ int SSL_write_ex(SSL *s, const void *buf, size_t num, size_t *written)
 int SSL_write_early_data(SSL *s, const void *buf, size_t num, size_t *written)
 {
     int ret, early_data_state;
+    size_t writtmp;
 
     switch (s->early_data_state) {
     case SSL_EARLY_DATA_NONE:
@@ -1994,9 +1995,25 @@ int SSL_write_early_data(SSL *s, const void *buf, size_t num, size_t *written)
 
     case SSL_EARLY_DATA_WRITE_RETRY:
         s->early_data_state = SSL_EARLY_DATA_WRITING;
-        ret = SSL_write_ex(s, buf, num, written);
+        ret = SSL_write_ex(s, buf, num, &writtmp);
+        if (!ret) {
+            s->early_data_state = SSL_EARLY_DATA_WRITE_RETRY;
+            return ret;
+        }
+        s->early_data_state = SSL_EARLY_DATA_WRITE_FLUSH;
+        /* fall through */
+
+    case SSL_EARLY_DATA_WRITE_FLUSH:
+        /* The buffering BIO is still in place so we need to flush it */
+        if (statem_flush(s) != 1)
+            return 0;
+        /*
+         * TODO(TLS1.3): Technically this may not be correct in the event of
+         * SSL_MODE_ENABLE_PARTIAL_WRITE. What should we do about this?
+         */
+        *written = num;
         s->early_data_state = SSL_EARLY_DATA_WRITE_RETRY;
-        return ret;
+        return 1;
 
     case SSL_EARLY_DATA_FINISHED_READING:
     case SSL_EARLY_DATA_READ_RETRY:

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index eec5be3f191a28e5cf34d857849f69db725a213f..b5705ed28e7796314dbba266501096a342d020d5 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -615,6 +615,7 @@ typedef enum {
     SSL_EARLY_DATA_CONNECTING,
     SSL_EARLY_DATA_WRITE_RETRY,
     SSL_EARLY_DATA_WRITING,
+    SSL_EARLY_DATA_WRITE_FLUSH,
     SSL_EARLY_DATA_UNAUTH_WRITING,
     SSL_EARLY_DATA_FINISHED_WRITING,
     SSL_EARLY_DATA_ACCEPT_RETRY,

diff --git a/ssl/statem/statem.h b/ssl/statem/statem.h
index 83bebe77e6ba589b69f1437a17fd115a66c7cef3..e8d9174b8f4451987842a6358e5aa28e950b5292 100644 (file)
--- a/ssl/statem/statem.h
+++ b/ssl/statem/statem.h
@@ -132,3 +132,6 @@ __owur int ossl_statem_skip_early_data(SSL *s);
 void ossl_statem_check_finish_init(SSL *s, int send);
 void ossl_statem_set_hello_verify_done(SSL *s);
 __owur int ossl_statem_app_data_allowed(SSL *s);
+
+/* Flush the write BIO */
+int statem_flush(SSL *s);

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 51cdd585d75e5c40d901220c2ce20daa3c0f9570..b47ae1ea109601e903bff9643777871645fc887a 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -665,9 +665,11 @@ WORK_STATE ossl_statem_client_pre_work(SSL *s, WORK_STATE wst)
         /* Fall through */
 
     case TLS_ST_EARLY_DATA:
+        return tls_finish_handshake(s, wst, 0, 1);
+
     case TLS_ST_OK:
         /* Calls SSLfatal() as required */
-        return tls_finish_handshake(s, wst, 1);
+        return tls_finish_handshake(s, wst, 1, 1);
     }
 
     return WORK_FINISHED_CONTINUE;
@@ -697,8 +699,6 @@ WORK_STATE ossl_statem_client_post_work(SSL *s, WORK_STATE wst)
              * we call tls13_change_cipher_state() directly.
              */
             if ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) == 0) {
-                if (!statem_flush(s))
-                    return WORK_MORE_A;
                 if (!tls13_change_cipher_state(s,
                             SSL3_CC_EARLY | SSL3_CHANGE_CIPHER_CLIENT_WRITE)) {
                     /* SSLfatal() already called */
@@ -737,8 +737,6 @@ WORK_STATE ossl_statem_client_post_work(SSL *s, WORK_STATE wst)
             break;
         if (s->early_data_state == SSL_EARLY_DATA_CONNECTING
                     && s->max_early_data > 0) {
-            if (statem_flush(s) != 1)
-                return WORK_MORE_A;
             /*
              * We haven't selected TLSv1.3 yet so we don't call the change
              * cipher state function associated with the SSL_METHOD. Instead

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index b65dfa15872f0be350575a33c87d088fa1725316..02d75e79aca19d952db795be10307ef7b3ac59d1 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -1004,7 +1004,7 @@ unsigned long ssl3_output_cert_chain(SSL *s, WPACKET *pkt, CERT_PKEY *cpk)
  * in NBIO events. If |clearbufs| is set then init_buf and the wbio buffer is
  * freed up as well.
  */
-WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs)
+WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop)
 {
     int discard;
     void (*cb) (const SSL *ssl, int type, int val) = NULL;
@@ -1083,11 +1083,7 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs)
         }
     }
 
-    /*
-     * If we've not cleared the buffers its because we've got more work to do,
-     * so continue.
-     */
-    if (!clearbufs)
+    if (!stop)
         return WORK_FINISHED_CONTINUE;
 
     ossl_statem_set_in_init(s, 0);

diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h
index 5e0ce7e9978d3c38f01928a3621eaf9f51dcefc0..eae9a361d7af306d46fd958b9cfa81281a05ef14 100644 (file)
--- a/ssl/statem/statem_locl.h
+++ b/ssl/statem/statem_locl.h
@@ -52,9 +52,6 @@ typedef enum {
     MSG_PROCESS_CONTINUE_READING
 } MSG_PROCESS_RETURN;
 
-/* Flush the write BIO */
-int statem_flush(SSL *s);
-
 typedef int (*confunc_f) (SSL *s, WPACKET *pkt);
 
 int check_in_list(SSL *s, uint16_t group_id, const uint16_t *groups,
@@ -106,7 +103,8 @@ __owur int dtls_construct_change_cipher_spec(SSL *s, WPACKET *pkt);
 __owur int tls_construct_finished(SSL *s, WPACKET *pkt);
 __owur int tls_construct_key_update(SSL *s, WPACKET *pkt);
 __owur MSG_PROCESS_RETURN tls_process_key_update(SSL *s, PACKET *pkt);
-__owur WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs);
+__owur WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs,
+                                       int stop);
 __owur WORK_STATE dtls_wait_for_dry(SSL *s);
 
 /* some client-only functions */

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index c6a9a66781c25cb1d31e3a6091699f0f62758c24..70e026d0e492e8a9b3168100e939134f3ad1d5f3 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -657,7 +657,7 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst)
              *
              * Calls SSLfatal as required.
              */
-            return tls_finish_handshake(s, wst, 0);
+            return tls_finish_handshake(s, wst, 0, 0);
         } if (SSL_IS_DTLS(s)) {
             /*
              * We're into the last flight. We don't retransmit the last flight
@@ -693,7 +693,7 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst)
 
     case TLS_ST_OK:
         /* Calls SSLfatal() as required */
-        return tls_finish_handshake(s, wst, 1);
+        return tls_finish_handshake(s, wst, 1, 1);
     }
 
     return WORK_FINISHED_CONTINUE;