Imported from upstream's 0971432f6f and
7d4c97add12cfa5d4589880b09d6139c3203e2f4, but with missing tests added. Along
the way, make Bytes work with any Span<const uint8_t>-convertable type.
Change-Id: If365f981fe8a8274e12000309ffd99b1bb719842
Reviewed-on: https://boringssl-review.googlesource.com/31086
Reviewed-by: Adam Langley <agl@google.com>
Update-Note: This changes causes BoringSSL to be stricter about handling
Unicode strings:
· Reject code points outside of Unicode
· Reject surrogate values
· Don't allow invalid UTF-8 to pass through when the source claims to
be UTF-8 already.
· Drop byte-order marks.
Previously, for example, a UniversalString could contain a large-valued
code point that would cause the UTF-8 encoder to emit invalid UTF-8.
Change-Id: I94d9db7796b70491b04494be84249907ff8fb46c
Reviewed-on: https://boringssl-review.googlesource.com/28325
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We compare pointer/length pairs constantly. To avoid needing to type it
everywhere and get GTest's output, add a StringPiece-alike for byte
slices which supports ==, !=, and std::ostream.
BUG=129
Change-Id: I108342cbd2c6a58fec0b9cb87ebdf50364bda099
Reviewed-on: https://boringssl-review.googlesource.com/13625
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 was done just by grepping for 'size_t i;' and 'size_t j;'. I left
everything in crypto/x509 and friends alone.
There's some instances in gcm.c that are non-trivial and pulled into a
separate CL for ease of review.
Change-Id: I6515804e3097f7e90855f1e7610868ee87117223
Reviewed-on: https://boringssl-review.googlesource.com/10801
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 only test amongst the tests for core crypto functionality
that depends on crypto/bio. This change removes that dependency. This
also factors out the duplicative hexdump logic into a shared function.
Change-Id: Ic280a71d086555a6993c05f183b94e1d38b60932
Reviewed-on: https://boringssl-review.googlesource.com/5622
Reviewed-by: Adam Langley <agl@google.com>
Nothing ever uses those structs. This to avoid having any structs in the
public header which use struct timeval.
In doing so, move the protocol version constants up to ssl.h so dtls1.h
may be empty. This also removes TLS1_get_version and TLS1_get_client_version
as they're unused and depend on TLS1_VERSION_MAJOR. This still lets tls1.h
be included independently from ssl.h (though I don't think anyone ever includes
it...).
Change-Id: Ieac8b90cf94f7f1e742a88bb75c0ee0aa4b1414c
Reviewed-on: https://boringssl-review.googlesource.com/4681
Reviewed-by: Adam Langley <agl@google.com>
This conceivably has a use, but NSS doesn't do this buffer either and it still
suffers from the same problems as the other uses of record_pqueue. This removes
the last use of record_pqueue. It also opens the door to removing pqueue
altogether as it isn't the right data structure for either of the remaining
uses either. (It's not clear it was right for record_pqueue either, but I don't
feel like digging into this code.)
Change-Id: If8a43e7332b3cd11a78a516f3e8ebf828052316f
Reviewed-on: https://boringssl-review.googlesource.com/4239
Reviewed-by: Adam Langley <agl@google.com>
It was only ever enabled for handshake and alert messages. The comments cite
renego as a use case though even then I'm not clear on why. The only use I see
is if, say, the Finished message and ClientKeyExchange came in out-of-order.
DTLS is unreliable so leaning on retransmit seems fine, and usually flights
will be packed into one packet where possible. NSS doesn't have any such
buffer and doesn't seem to have problems.
The buffering mechanism is also rather dubious. It stows away the entire packet
and read buffer---all 16K of it---and there may have been other records in that
packet.
Change-Id: Ic3b7bf817be380dc73102eec62c690ed093e6667
Reviewed-on: https://boringssl-review.googlesource.com/4238
Reviewed-by: Adam Langley <agl@google.com>
Replace unsigned long with the appropriate sized integer type.
Change-Id: I7b4641d84568f6c11efa25350a9e488a556fc92e
Reviewed-on: https://boringssl-review.googlesource.com/3766
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>
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>
Nothing recognized through those codepaths is fragmentable in DTLS. Also remove
an unnecessary epoch check. It's not possible to process a record from the
wrong epoch.
Change-Id: I9d0f592860bb096563e2bdcd2c8e50a0d2b65f59
Reviewed-on: https://boringssl-review.googlesource.com/3232
Reviewed-by: Adam Langley <agl@google.com>
All but one field is a no-op.
Change-Id: Ib7bc59a12ce792d5e42fb6e04a4aff54f42643a9
Reviewed-on: https://boringssl-review.googlesource.com/3213
Reviewed-by: Adam Langley <agl@google.com>
This is so the tests needn't be sensitive to the clock. It is, unfortunately, a
test-only hook, but the DTLS retransmit/timeout logic more-or-less requires it
currently. Use this hook to, for now, freeze the clock at zero. This makes the
tests deterministic.
It might be worth designing a saner API in the future. The current one,
notably, requires that the caller's clock be compatible with the one we
internally use. It's also not clear whether the caller needs to call
DTLSv1_handle_timeout or can just rely on the state machine doing it internally
(as it does do). But mock clocks are relatively tame and WebRTC wants to
compile against upstream OpenSSL for now, so we're limited in how much new API
we can build.
Change-Id: I7aad51570596f69275ed0fc1a8892393e4b7ba13
Reviewed-on: https://boringssl-review.googlesource.com/3210
Reviewed-by: Adam Langley <agl@google.com>
This CL removes the last of the EVP_CIPHER codepath in ssl/. The dead code is
intentionally not pruned for ease of review, except in DTLS-only code where
adding new logic to support both, only to remove half, would be cumbersome.
Fixes made:
- dtls1_retransmit_state is taught to retain aead_write_ctx rather than
enc_write_ctx.
- d1_pkt.c reserves space for the variable-length nonce when echoed into the
packet.
- dtls1_do_write sizes the MTU based on EVP_AEAD max overhead.
- tls1_change_cipher_state_cipher should not free AEAD write contexts in DTLS.
This matches the (rather confused) ownership for the EVP_CIPHER contexts.
I've added a TODO to resolve this craziness.
A follow-up CL will remove all the resultant dead code.
Change-Id: I644557f4db53bbfb182950823ab96d5e4c908866
Reviewed-on: https://boringssl-review.googlesource.com/2699
Reviewed-by: Adam Langley <agl@google.com>
This avoids needing a should_add_to_finished_hash boolean on do_write. The
logic in do_write was a little awkward because do_write would be called
multiple times if the write took several iterations. This also gets complex if
DTLS retransmits are involved. (At a glance, it's not obvious the
BIO_CTRL_DGRAM_MTU_EXCEEDED case actually works.)
Doing it as the handshake message is being prepared avoids this concern. It
also gives a natural point for the extended master secret logic which needs to
do work after the finished hash has been sampled.
As a bonus, we can remove s->d1->retransmitting which was only used to deal
with this issue.
Change-Id: Ifedf23ee4a6c5e08f960d296a6eb1f337a16dc7a
Reviewed-on: https://boringssl-review.googlesource.com/2604
Reviewed-by: Adam Langley <agl@google.com>
This makes SSLv23_method go through DTLS_ANY_VERSION's version negotiation
logic. This allows us to get rid of duplicate ClientHello logic. For
compatibility, SSL_METHOD is now split into SSL_PROTOCOL_METHOD and a version.
The legacy version-locked methods set min_version and max_version based this
version field to emulate the original semantics.
As a bonus, we can now handle fragmented ClientHello versions now.
Because SSLv23_method is a silly name, deprecate that too and introduce
TLS_method.
Change-Id: I8b3df2b427ae34c44ecf972f466ad64dc3dbb171
Parameters like these should not change between 32-bit and 64-bit. 64 is also
the value recommended in RFC 6347, section 4.1.2.6. Document those fields while
I'm here.
Change-Id: I8481ee0765ff3d261a96a2e1a53b6ad6695b2d42
Reviewed-on: https://boringssl-review.googlesource.com/2222
Reviewed-by: Adam Langley <agl@google.com>
This was added in http://rt.openssl.org/Ticket/Display.html?id=2033 to support
a mode where a DTLS socket would statelessly perform the ClientHello /
HelloVerifyRequest portion of the handshake, to be handed off to a socket
specific to this peer address.
This is not used by WebRTC or other current consumers. If we need to support
something like this, it would be cleaner to do the listen portion (cookieless
ClientHello + HelloVerifyRequest) externally and then spin up an SSL instance
on receipt of a cookied ClientHello. This would require a slightly more complex
BIO to replay the second ClientHello but would avoid peppering the DTLS
handshake state with a special short-circuiting mode.
Change-Id: I7a413932edfb62f8b9368912a9a0621d4155f1aa
Reviewed-on: https://boringssl-review.googlesource.com/2220
Reviewed-by: Adam Langley <agl@google.com>
This code isn't compiled in. It seems there was some half-baked logic for a
7-byte alert that includes more information about handshake messages
retransmit.
No such alert exists, and the code had a FIXME anyway. If it gets resurrected
in DTLS 1.3 or some extension, we can deal with it then.
Change-Id: I8784ea8ee44bb8da4b0fe5d5d507997526557432
Reviewed-on: https://boringssl-review.googlesource.com/2121
Reviewed-by: Adam Langley <agl@google.com>
Rather than switching the order of the ServerHello and HelloVerifyRequest
states and processing each twice, have the states follow the protocol order.
HelloVerifyRequest reading is optional and ServerHello is strict. Use the
send_cookie bit to determine whether we're expecting a cookie or not.
Fix the dtls1_stop_timer call in these states to consistently hit the end of a
server flight; the previous flight should not be cleared from the retransmit
buffer until the entire next flight is received. That said, OpenSSL doesn't
appear to implement the part where, on receipt of the previous peer flight, the
buffered flight is retransmitted. (With the exception of a SSL3_MT_FINISHED
special-case in dtls1_read_bytes.) So if the peer is also OpenSSL, this doesn't
do anything.
Also fix the DTLS test which wasn't actually asserting that the ClientHello
matched.
Change-Id: Ia542190972dbffabb837d32c9d453a243caa90b2
Reviewed-on: https://boringssl-review.googlesource.com/1551
Reviewed-by: Adam Langley <agl@google.com>
I see no internal users and the existence of a THIRD version encoding
complicates all version-checking logic. Also convert another version check to
SSL_IS_DTLS that was missed earlier.
Change-Id: I60d215f57d44880f6e6877889307dc39dbf838f7
Reviewed-on: https://boringssl-review.googlesource.com/1550
Reviewed-by: Adam Langley <agl@google.com>
Windows doesn't have ssize_t, sadly. There's SSIZE_T, but defining an
OPENSSL_SSIZE_T seems worse than just using an int.
Change-Id: I09bb5aa03f96da78b619e551f92ed52ce24d9f3f
Reviewed-on: https://boringssl-review.googlesource.com/1352
Reviewed-by: Adam Langley <agl@google.com>
Also fix some DTLS cookie bugs. rcvd_cookie is never referenced after being
saved (and the length isn't saved, so it couldn't be used anyway), and the
cookie verification failed to check the length.
For convenience, add a CBS_mem_equal helper function. Saves a bit of
repetition.
Change-Id: I187137733b069f0ac8d8b1bf151eeb80d388b971
Reviewed-on: https://boringssl-review.googlesource.com/1174
Reviewed-by: Adam Langley <agl@google.com>
Previously, public headers lived next to the respective code and there
were symlinks from include/openssl to them.
This doesn't work on Windows.
This change moves the headers to live in include/openssl. In cases where
some symlinks pointed to the same header, I've added a file that just
includes the intended target. These cases are all for backwards-compat.
Change-Id: I6e285b74caf621c644b5168a4877db226b07fd92
Reviewed-on: https://boringssl-review.googlesource.com/1180
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Brian Smith
Matt Braithwaite
David Benjamin
Adam Langley