Change-Id: I73f9fd64b46f26978b897409d817b34ec9d93afd
Reviewed-on: https://boringssl-review.googlesource.com/11080
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This mirror's 2dc0204603 on the C side.
BUG=90
Change-Id: Iebb72df5a5ae98cb2fd8db519d973cd734ff05ea
Reviewed-on: https://boringssl-review.googlesource.com/11320
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Runner needs to implement fuzzer mode as well so we can record
transcripts from it. A bunch of tests were failing:
- C and Go disagreed on what fuzzer mode did to TLS 1.3 padding. So we
fuzz more code, align Go with C. Fuzzer mode TLS 1.3 still pads but
just skips the final AEAD.
- The deterministic RNG should be applied per test, not per exchange. It
turns out, if your RNG is deterministic, one tends to pick the same
session ID over and over which confuses clients. (Resumption is
signaled by echoing the session ID.)
Now the only failing tests are the ones one would expect to fail.
BUG=79
Change-Id: Ica23881a6e726adae71e6767730519214ebcd62a
Reviewed-on: https://boringssl-review.googlesource.com/11126
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is the equivalent of FragmentAcrossChangeCipherSuite for DTLS. It
is possible for us to, while receiving pre-CCS handshake messages, to
buffer up a message with sequence number meant for a post-CCS Finished.
When we then get to the new epoch and attempt to read the Finished, we
will process the buffered Finished although it was sent with the wrong
encryption.
Move ssl_set_{read,write}_state to SSL_PROTOCOL_METHOD hooks as this is
a property of the transport. Notably, read_state may fail. In DTLS
check the handshake buffer size. We could place this check in
read_change_cipher_spec, but TLS 1.3 has no ChangeCipherSpec message, so
we will need to implement this at the cipher change point anyway. (For
now, there is only an assert on the TLS side. This will be replaced with
a proper check in TLS 1.3.)
Change-Id: Ia52b0b81e7db53e9ed2d4f6d334a1cce13e93297
Reviewed-on: https://boringssl-review.googlesource.com/8790
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I'm surprised we'd never tested this. In addition to splitting handshake
records up, one may pack multiple handshakes into a single record, as
they fit. Generalize the DTLS handshake flush hook to do this in TLS as
well.
Change-Id: Ia546d18c7c56ba45e50f489c5b53e1fcd6404f51
Reviewed-on: https://boringssl-review.googlesource.com/8650
Reviewed-by: David Benjamin <davidben@google.com>
We were missing this case. It is possible to receive an early unencrypted
ChangeCipherSpec alert in DTLS because they aren't ordered relative to the
handshake. Test this case. (ChangeCipherSpec in DTLS is kind of pointless.)
Change-Id: I84268bc1821734f606fb20bfbeda91abf372f32c
Reviewed-on: https://boringssl-review.googlesource.com/8460
Reviewed-by: Adam Langley <agl@google.com>
This is the only codepath where ssl->version can get a garbage value, which is
a little concerning. Since, in all these cases, the peer is failing to connect
and speaks so low a version we don't even accept it anymore, there is probably
not much value in letting them distinguish protocol_version from a record-layer
version number mismatch, where enforced (which will give a version-related
error anyway).
Should we get a decode_error or so just before version negotiation, we'd have
this behavior already.
Change-Id: I9b3e5685ab9c9ad32a7b7e3129363cd1d4cdaaf4
Reviewed-on: https://boringssl-review.googlesource.com/8420
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This implements the cipher suite constraints in "fake TLS 1.3". It also makes
bssl_shim and runner enable it by default so we can start adding MaxVersion:
VersionTLS12 markers to tests as 1.2 vs. 1.3 differences begin to take effect.
Change-Id: If1caf6e43938c8d15b0a0f39f40963b8199dcef5
Reviewed-on: https://boringssl-review.googlesource.com/8340
Reviewed-by: David Benjamin <davidben@google.com>
The DTLS bbio logic is rather problematic, but this shouldn't make things
worse. In the in-handshake case, the new code merges the per-message
(unchecked) BIO_flush calls into one call at the end but otherwise the BIO is
treated as is. Otherwise any behavior around non-block writes should be
preserved.
In the post-handshake case, we now install the buffer when we didn't
previously. On write error, the buffer will have garbage in it, but it will be
discarded, so that will preserve any existing retry behavior. (Arguably the
existing retry behavior is a bug, but that's another matter.)
Add a test for all this, otherwise it is sure to regress. Testing for
record-packing is a little fuzzy, but we can assert ChangeCipherSpec always
shares a record with something.
BUG=57
Change-Id: I8603f20811d502c71ded2943b0e72a8bdc4e46f2
Reviewed-on: https://boringssl-review.googlesource.com/7871
Reviewed-by: Adam Langley <agl@google.com>
Although the DTLS transport layer logic drops failed writes on the floor, it is
actually set up to work correctly. If an SSL_write fails at the transport,
dropping the buffer is fine. Arguably it works better than in TLS because we
don't have the weird "half-committed to data" behavior. Likewise, the handshake
keeps track of how far its gotten and resumes the message at the right point.
This broke when the buffering logic was rewritten because I didn't understand
what the DTLS code was doing. The one thing that doesn't work as one might
expect is non-fatal write errors during rexmit are not recoverable. The next
timeout must fire before we try again.
This code is quite badly sprinkled in here, so add tests to guard it against
future turbulence. Because of the rexmit issues, the tests need some hacks
around calls which may trigger them. It also changes the Go DTLS implementation
from being completely strict about sequence numbers to only requiring they be
monotonic.
The tests also revealed another bug. This one seems to be upstream's fault, not
mine. The logic to reset the handshake hash on the second ClientHello (in the
HelloVerifyRequest case) was a little overenthusiastic and breaks if the
ClientHello took multiple tries to send.
Change-Id: I9b38b93fff7ae62faf8e36c4beaf848850b3f4b9
Reviewed-on: https://boringssl-review.googlesource.com/6417
Reviewed-by: Adam Langley <agl@google.com>
This change makes the runner tests (in ssl/test/runner) act like a
normal Go test rather than being a Go binary. This better aligns with
some internal tools.
Thus, from this point onwards, one has to run the runner tests with `go
test` rather than `go run` or `go build && ./runner`.
This will break the bots.
Change-Id: Idd72c31e8e0c2b7ed9939dacd3b801dbd31710dd
Reviewed-on: https://boringssl-review.googlesource.com/6009
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The existing tests only went monotonic. Allow an arbitrary mapping
function. Also test by sending more app data. The handshake is fairly
resilient to replayed packets, whereas our test code intentionally
isn't.
Change-Id: I0fb74bbacc260c65ec5f6a1ca8f3cb23b4192855
Reviewed-on: https://boringssl-review.googlesource.com/5556
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 9dcab127e14467733523ff7626da8906e67eedd6. The root problem
is dtls1_read_bytes is wrong, but we can get the right behavior now and add a
regression test for it before cleaning it up.
Change-Id: I4e5c39ab254a872d9f64242c9b77b020bdded6e6
Reviewed-on: https://boringssl-review.googlesource.com/5123
Reviewed-by: Adam Langley <agl@google.com>
9a41d1b946 broke handling of multiple records in
a single packet. If |extend| is true, not all of the previous packet should be
consumed, only up to the record length.
Add a test which stresses the DTLS stack's handling of multiple handshake
fragments in a handshake record and multiple handshake records in a packet.
Change-Id: I96571098ad9001e96440501c4730325227b155b8
Reviewed-on: https://boringssl-review.googlesource.com/4950
Reviewed-by: Adam Langley <agl@google.com>
This drops in a copy of a subset of golang.org/x/crypto/poly1305 to implement
Poly1305. Hopefully this will keep them from regression as we rework the record
layer.
Change-Id: Ic1e0d941a0a9e5ec260151ced8acdf9215c4b887
Reviewed-on: https://boringssl-review.googlesource.com/4257
Reviewed-by: Adam Langley <agl@google.com>
Notably, drop all special cases around receiving a message in order and
receiving a full message. It makes things more complicated and was the source
of bugs (the MixCompleteMessageWithFragments tests added in this CL did not
pass before). Instead, every message goes through an hm_fragment, and
dtls1_get_message always checks buffered_messages to see if the next is
complete.
The downside is that we pay one more copy of the message data in the common
case. This is only during connection setup, so I think it's worth the
simplicity. (If we want to optimize later, we could either tighten
ssl3_get_message's interface to allow the handshake data being in the
hm_fragment's backing store rather than s->init_buf or swap out s->init_buf
with the hm_fragment's backing store when a mesasge completes.
This CL does not address ssl_read_bytes being an inappropriate API for DTLS.
Future work will revise the handshake/transport boundary to align better with
DTLS's needs. Also other problems that I've left as TODOs.
Change-Id: Ib4570d45634b5181ecf192894d735e8699b1c86b
Reviewed-on: https://boringssl-review.googlesource.com/3764
Reviewed-by: Adam Langley <agl@google.com>
We actually don't really care about this special-case since we only test client
full handshakes where the runner sends the second Finished not the shim
(otherwise the overlap logic and retransmitting on every fragment would
probably break us), but it should probably live next to the fragmentation
logic.
Change-Id: I54097d84ad8294bc6c42a84d6f22f496e63eb2a8
Reviewed-on: https://boringssl-review.googlesource.com/3763
Reviewed-by: Adam Langley <agl@google.com>
SSL_AEAD_CTX ownership is currently too confusing. Instead, rely on the lack of
renego, so the previous epoch always uses the NULL cipher. (Were we to support
DTLS renego, we could keep track of s->d1->last_aead_write_ctx like
s->d1->last_write_sequence, but it isn't worth it.)
Buffered messages also tracked an old s->session, but this is unnecessary. The
s->session NULL check in tls1_enc dates to the OpenSSL initial commit and is
redundant with the aead NULL check.
Change-Id: I9a510468d95934c65bca4979094551c7536980ae
Reviewed-on: https://boringssl-review.googlesource.com/3234
Reviewed-by: Adam Langley <agl@google.com>
For now, only test reorderings when we always or never fragment messages.
There's a third untested case: when full messages and fragments are mixed. That
will be tested later after making it actually work.
Change-Id: Ic4efb3f5e87b1319baf2d4af31eafa40f6a50fa6
Reviewed-on: https://boringssl-review.googlesource.com/3216
Reviewed-by: Adam Langley <agl@google.com>
No behavior change. This is in preparation for buffering a flight of handshake
messages to reorder vigorously on flush.
Change-Id: Ic348829b340bf58d28f332027646559cb11046ac
Reviewed-on: https://boringssl-review.googlesource.com/3215
Reviewed-by: Adam Langley <agl@google.com>
This reverts commit c67a3ae6ba. With a
deterministic clock, we can now go back to being strict about retransmits. Our
tests will now require that the shim only retransmit when we expect it to.
Change-Id: Iab1deb9665dcd294790c8253d920089e83a9140c
Reviewed-on: https://boringssl-review.googlesource.com/3211
Reviewed-by: Adam Langley <agl@google.com>
BoringSSL currently retransmits non-deterministically on an internal timer
(rather than one supplied externally), so the tests currently fail flakily
depending on timing. Valgrind is a common source for this. We still assume an
in-order and reliable channel, but drop retransmits silently:
- Handshake messages may arrive with old sequence numbers.
- Retransmitted CCS records arrive from the previous epoch.
- We may receive a retransmitted Finished after we believe the handshake has
completed. (Aside: even in a real implementation, only Finished is possible
here. Even with out-of-order delivery, retransmitted or reordered messages
earlier in the handshake come in under a different epoch.)
Note that because DTLS renego and a Finished retransmit are ambiguous at the
record layer[*], this precludes us writing tests for DTLS renego. But DTLS
renego should get removed anyway. As BoringSSL currently implements renego,
this ambiguity is also a source of complexity in the real implementation. (See
the SSL3_MT_FINISHED check in dtls1_read_bytes.)
[*] As a further fun aside, it's also complex if dispatching renego vs Finished
after handshake message reassembly. The spec doesn't directly say the sequence
number is reset across renegos, but it says "The first message each side
transmits in /each/ handshake always has message_seq = 0". This means that such
an implementation needs the handshake message reassembly logic be aware that a
Finished fragment with high sequence number is NOT an out-of-order fragment for
the next handshake.
Change-Id: I35d13560f82bcb5eeda62f4de1571d28c818cc36
Reviewed-on: https://boringssl-review.googlesource.com/2770
Reviewed-by: Adam Langley <agl@google.com>
The minimum MTU (not consistently enforced) is just under 256, so it's
difficult to test everything, but this is a basic test. (E.g., without renego,
the only handshake message with encryption is Finished which fits in the MTU.)
It tests the server side because the Certificate message is large enough to
require fragmentation.
Change-Id: Ida11f1057cebae2b800ad13696f98bb3a7fbbc5e
Reviewed-on: https://boringssl-review.googlesource.com/2824
Reviewed-by: Adam Langley <agl@google.com>
The record-layer version of the ServerHello should match the final version. The
record-layer version of the ClientHello should be the advertised version, but
clamped at TLS 1.0. This is to ensure future rewrites do not regress this.
Change-Id: I96f1f0674944997ff38b562453a322ce61652635
Reviewed-on: https://boringssl-review.googlesource.com/2540
Reviewed-by: Adam Langley <agl@google.com>
At the record layer, DTLS maintains a window of seen sequence numbers to detect
replays. Add tests to cover that case. Test both repeated sequence numbers
within the window and sequence numbers past the window's left edge. Also test
receiving sequence numbers far past the window's right edge.
Change-Id: If6a7a24869db37fdd8fb3c4b3521b730e31f8f86
Reviewed-on: https://boringssl-review.googlesource.com/2221
Reviewed-by: Adam Langley <agl@google.com>
Run against openssl s_client and openssl s_server. This seems to work for a
start, although it may need to become cleverer to stress more of BoringSSL's
implementation for test purposes.
In particular, it assumes a reliable, in-order channel. And it requires that
the peer send handshake fragments in order. Retransmit and whatnot are not
implemented. The peer under test will be expected to handle a lossy channel,
but all loss in the channel will be controlled. MAC errors, etc., are fatal.
Change-Id: I329233cfb0994938fd012667ddf7c6a791ac7164
Reviewed-on: https://boringssl-review.googlesource.com/1390
Reviewed-by: Adam Langley <agl@google.com>
