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>

ssl/internal.h

@@ -2645,7 +2645,6 @@ struct SSL3_STATE {
   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 @@ struct SSL3_STATE {
   // 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;
 

ssl/s3_lib.cc

@@ -175,7 +175,6 @@ SSL3_STATE::SSL3_STATE()
       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),

ssl/s3_pkt.cc

@@ -147,10 +147,10 @@ int tls_write_app_data(SSL *ssl, bool *out_needs_handshake, const uint8_t *in,
   // 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 @@ int tls_write_app_data(SSL *ssl, bool *out_needs_handshake, const uint8_t *in,
   }
 }
 
-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 @@ static int do_tls_write(SSL *ssl, int type, const uint8_t *in, unsigned len) {
   // 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,

ssl/ssl_lib.cc

@@ -1239,7 +1239,8 @@ void SSL_reset_early_data_reject(SSL *ssl) {
   // 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) {

ssl/ssl_test.cc

@@ -8332,5 +8332,63 @@ xNCwyMX9mtdXdQicOfNjIGUCD5OLV5PgHFPRKiHHioBAhg==
   }
 }
 
+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