From 2a8db717132ec8be7dc24ce7083972245b1173ae Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 27 Nov 2017 15:20:06 +0000 Subject: [PATCH] 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 (Merged from https://github.com/openssl/openssl/pull/4802) --- ssl/ssl_lib.c | 21 +++++++++++++++++++-- ssl/ssl_locl.h | 1 + ssl/statem/statem.h | 3 +++ ssl/statem/statem_clnt.c | 8 +++----- ssl/statem/statem_lib.c | 8 ++------ ssl/statem/statem_locl.h | 6 ++---- ssl/statem/statem_srvr.c | 4 ++-- 7 files changed, 32 insertions(+), 19 deletions(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index fb113bab6d..61e5ebb2d1 100644 --- 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 eec5be3f19..b5705ed28e 100644 --- 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 83bebe77e6..e8d9174b8f 100644 --- 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 51cdd585d7..b47ae1ea10 100644 --- 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 b65dfa1587..02d75e79ac 100644 --- 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 5e0ce7e997..eae9a361d7 100644 --- 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 c6a9a66781..70e026d0e4 100644 --- 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; -- 2.39.2