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
