We still have our <= 0 return values because anything with BIOs tries to
preserve BIO_write's error returns. (Maybe we can stop doing this?
BIO_read's error return is a little subtle with EOF vs error, but
BIO_write's is uninteresting.) But the rest of the logic is size_t-clean
and hopefully a little clearer. We still have to support SSL_write's
rather goofy calling convention, however.
I haven't pushed Spans down into the low-level record construction logic
yet. We should probably do that, but there are enough offsets tossed
around there that they warrant their own CL.
Bug: 507
Change-Id: Ia0c702d1a2d3713e71b0bbfa8d65649d3b20da9b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47544
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
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>
This is split out from
https://boringssl-review.googlesource.com/c/boringssl/+/47544 just to
get the bugfixes and tests out of the way of the refactor.
If we trip the SSL_R_BAD_LENGTH check in tls_write_app_data, wnum is set
to zero. But wnum should only be cleared on a successful write. It
tracks the number of input bytes that have been written to the transport
but not yet reported to the caller. Instead, move it to the success
return in that function. All the other error paths already set it to
something else.
Change-Id: Ib22f9cf04454ecdb0062077f183be5070ab7d791
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53545
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
ServerHello/EncryptedExtensions/Finished is logically one atomic flight
that exits the early data state, we have process each message
sequentially. Until we've processed Finished, we are still in the early
data state and must support writing data. Individual messages *are*
processed atomically, so the interesting points are before ServerHello
(already tested), after ServerHello, and after EncryptedExtensions.
The TLS 1.3 handshake internally clears ssl->session when processing
ServerHello, so getting the early data information from ssl->session
does not work. Instead, use hs->early_session, which is what other
codepaths use.
I've tested this with runner rather than ssl_test, so we can test both
post-SH and post-EE states. ssl_test would be more self-contained, since
we can directly control the API calls, but it cannot test the post-EE
state. To reduce record overhead, our production implementation packs EE
and Finished into the same record, which means the handshake will
process the two atomically. Instead, I've tested this in runner, with a
flag to partially drive the handshake before reading early data.
I've also tweaked the logic to hopefully be a little clearer.
Bug: chromium:1208784
Change-Id: Ia4901042419c5324054f97743bd1aac59ebf8f24
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47485
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Adam Langley