Fix up book-keeping between the write buffer and pending writes. Writing application data goes through three steps: 1. Encrypt the data into the write buffer. 2. Flush the write buffer to the network. 3. Report to SSL_write's caller that the write succeeded. In principle, steps 2 and 3 are done together, but it is possible that BoringSSL needs to write something, but we are not in the middle of servicing an SSL_write call. Then we must perform (2) but cannot perform (3). TLS 1.3 0-RTT on a client introduces a case like this. Suppose we write some 0-RTT data, but it is blocked on the network. Meanwhile, the application tries to read from the socket (protocols like HTTP/2 read and write concurrently). We discover ServerHello..Finished and must then respond with EndOfEarlyData..Finished. But to write, we must flush the current write buffer. To fix this, https://boringssl-review.googlesource.com/14164 split (2) and (3) more explicitly. The write buffer may be flushed to the network at any point, but the wpend_* book-keeping is separate. It represents whether (3) is done. As part of that, we introduced a wpend_pending boolean to track whether there was pending data. This introduces an interesting corner case. We now keep NewSessionTicket messages buffered until the next SSL_write. (KeyUpdate ACKs are implemented similarly.) Suppose the caller calls SSL_write(nullptr, 0) to flush the NewSessionTicket and this hits EWOULDBLOCK. We'll track a zero-length pending write in wpend_*! A future attempt to write non-zero data would then violate the moving buffer check. This is strange because we don't build records for zero-length application writes in the first place. Instead, wpend_pending should have been wpend_tot > 0. Remove that and rearrange the code to check that properly. Also remove wpend_ret as it has the same data as wpend_tot. Change-Id: I58c23842cd55e8a8dfbb1854b61278b108b5c7ea Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53546 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: Bob Beck <bbe@google.com>
diff --git a/ssl/internal.h b/ssl/internal.h index 2400f90..41630f5 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -2645,7 +2645,6 @@ unsigned int wnum = 0; // number of bytes sent so far int wpend_tot = 0; // number bytes written int wpend_type = 0; - int wpend_ret = 0; // number of bytes submitted const uint8_t *wpend_buf = nullptr; // read_shutdown is the shutdown state for the read half of the connection. @@ -2726,9 +2725,6 @@ // outstanding. bool key_update_pending : 1; - // wpend_pending is true if we have a pending write outstanding. - bool wpend_pending : 1; - // early_data_accepted is true if early data was accepted by the server. bool early_data_accepted : 1;
diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc index fa73d34..2adf386 100644 --- a/ssl/s3_lib.cc +++ b/ssl/s3_lib.cc
@@ -175,7 +175,6 @@ send_connection_binding(false), channel_id_valid(false), key_update_pending(false), - wpend_pending(false), early_data_accepted(false), alert_dispatch(false), renegotiate_pending(false),
diff --git a/ssl/s3_pkt.cc b/ssl/s3_pkt.cc index bc3e99a..efe5905 100644 --- a/ssl/s3_pkt.cc +++ b/ssl/s3_pkt.cc
@@ -147,10 +147,10 @@ // Ensure that if we end up with a smaller value of data to write out than // the the original len from a write which didn't complete for non-blocking // I/O and also somehow ended up avoiding the check for this in - // tls_write_pending/SSL_R_BAD_WRITE_RETRY as it must never be possible to - // end up with (len-tot) as a large number that will then promptly send - // beyond the end of the users buffer ... so we trap and report the error in - // a way the user will notice. + // do_tls_write/SSL_R_BAD_WRITE_RETRY as it must never be possible to end up + // with (len-tot) as a large number that will then promptly send beyond the + // end of the users buffer ... so we trap and report the error in a way the + // user will notice. if (len < 0 || (size_t)len < tot) { OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_LENGTH); return -1; @@ -196,29 +196,31 @@ } } -static int tls_write_pending(SSL *ssl, int type, const uint8_t *in, - unsigned int len) { - if (ssl->s3->wpend_tot > (int)len || - (!(ssl->mode & SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER) && - ssl->s3->wpend_buf != in) || - ssl->s3->wpend_type != type) { +// do_tls_write writes an SSL record of the given type. +static int do_tls_write(SSL *ssl, int type, const uint8_t *in, unsigned len) { + // If there is a pending write, the retry must be consistent. + if (ssl->s3->wpend_tot > 0 && + (ssl->s3->wpend_tot > (int)len || + (!(ssl->mode & SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER) && + ssl->s3->wpend_buf != in) || + ssl->s3->wpend_type != type)) { OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_WRITE_RETRY); return -1; } + // Flush any unwritten data to the transport. There may be data to flush even + // if |wpend_tot| is zero. int ret = ssl_write_buffer_flush(ssl); if (ret <= 0) { return ret; } - ssl->s3->wpend_pending = false; - return ssl->s3->wpend_ret; -} -// do_tls_write writes an SSL record of the given type. -static int do_tls_write(SSL *ssl, int type, const uint8_t *in, unsigned len) { - // If there is still data from the previous record, flush it. - if (ssl->s3->wpend_pending) { - return tls_write_pending(ssl, type, in, len); + // If there is a pending write, we just completed it. Report it to the caller. + if (ssl->s3->wpend_tot > 0) { + ret = ssl->s3->wpend_tot; + ssl->s3->wpend_buf = nullptr; + ssl->s3->wpend_tot = 0; + return ret; } SSLBuffer *buf = &ssl->s3->write_buffer; @@ -282,16 +284,19 @@ // acknowledgments. ssl->s3->key_update_pending = false; - // Memorize arguments so that tls_write_pending can detect bad write retries - // later. - ssl->s3->wpend_tot = len; - ssl->s3->wpend_buf = in; - ssl->s3->wpend_type = type; - ssl->s3->wpend_ret = len; - ssl->s3->wpend_pending = true; + // Flush the write buffer. + ret = ssl_write_buffer_flush(ssl); + if (ret <= 0) { + // Track the unfinished write. + if (len > 0) { + ssl->s3->wpend_tot = len; + ssl->s3->wpend_buf = in; + ssl->s3->wpend_type = type; + } + return ret; + } - // We now just need to write the buffer. - return tls_write_pending(ssl, type, in, len); + return len; } ssl_open_record_t tls_open_app_data(SSL *ssl, Span<uint8_t> *out,
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index bbcc3b1..7035748 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -1239,7 +1239,8 @@ // Discard any unfinished writes from the perspective of |SSL_write|'s // retry. The handshake will transparently flush out the pending record // (discarded by the server) to keep the framing correct. - ssl->s3->wpend_pending = false; + ssl->s3->wpend_buf = nullptr; + ssl->s3->wpend_tot = 0; } enum ssl_early_data_reason_t SSL_get_early_data_reason(const SSL *ssl) {
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 0811cb4..55b7051 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -8332,5 +8332,63 @@ } } +TEST(SSLTest, EmptyWriteBlockedOnHandshakeData) { + bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method())); + bssl::UniquePtr<SSL_CTX> server_ctx = + CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + // Configure only TLS 1.3. This test requires post-handshake NewSessionTicket. + ASSERT_TRUE(SSL_CTX_set_min_proto_version(client_ctx.get(), TLS1_3_VERSION)); + ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), TLS1_3_VERSION)); + + // Connect a client and server with tiny buffer between the two. + bssl::UniquePtr<SSL> client(SSL_new(client_ctx.get())), + server(SSL_new(server_ctx.get())); + ASSERT_TRUE(client); + ASSERT_TRUE(server); + SSL_set_connect_state(client.get()); + SSL_set_accept_state(server.get()); + BIO *bio1, *bio2; + ASSERT_TRUE(BIO_new_bio_pair(&bio1, 1, &bio2, 1)); + SSL_set_bio(client.get(), bio1, bio1); + SSL_set_bio(server.get(), bio2, bio2); + ASSERT_TRUE(CompleteHandshakes(client.get(), server.get())); + + // We defer NewSessionTicket to the first write, so the server has a pending + // NewSessionTicket. See https://boringssl-review.googlesource.com/34948. This + // means an empty write will flush the ticket. However, the transport only + // allows one byte through, so this will fail with |SSL_ERROR_WANT_WRITE|. + int ret = SSL_write(server.get(), nullptr, 0); + ASSERT_EQ(ret, -1); + ASSERT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_WANT_WRITE); + + // Attempting to write non-zero data should not trip |SSL_R_BAD_WRITE_RETRY|. + const uint8_t kData[] = {'h', 'e', 'l', 'l', 'o'}; + ret = SSL_write(server.get(), kData, sizeof(kData)); + ASSERT_EQ(ret, -1); + ASSERT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_WANT_WRITE); + + // Byte by byte, the data should eventually get through. + uint8_t buf[sizeof(kData)]; + for (;;) { + ret = SSL_read(client.get(), buf, sizeof(buf)); + ASSERT_EQ(ret, -1); + ASSERT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); + + ret = SSL_write(server.get(), kData, sizeof(kData)); + if (ret > 0) { + ASSERT_EQ(ret, 5); + break; + } + ASSERT_EQ(ret, -1); + ASSERT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_WANT_WRITE); + } + + ret = SSL_read(client.get(), buf, sizeof(buf)); + ASSERT_EQ(ret, static_cast<int>(sizeof(kData))); + ASSERT_EQ(Bytes(buf, ret), Bytes(kData)); +} + } // namespace BSSL_NAMESPACE_END