Thanks to Eric Rescorla for catching this.
Change-Id: Id0a024d7f705519cfe76d350e0ef2688dbd11a22
Reviewed-on: https://boringssl-review.googlesource.com/12303
Reviewed-by: Nick Harper <nharper@chromium.org>
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>
Having that logic in two different places is a nuisance when we go to
add new checks like resumption stuff. Along the way, this adds missing
tests for the ClientHello cipher/session consistency check. (We'll
eventually get it for free once the cipher/resumption change is
unblocked, but get this working in the meantime.)
This also fixes a bug where the session validity checks happened in the
wrong order relative to whether tickets_supported or renew_ticket was
looked at. Fix that by lifting that logic closer to the handshake.
Change-Id: I3f4b59cfe01064f9125277dc5834e62a36e64aae
Reviewed-on: https://boringssl-review.googlesource.com/12230
Reviewed-by: Adam Langley <agl@google.com>
This was removed a while ago. As of -18, the early data indication
extension is just a boolean.
Change-Id: I328b9abfafad326d4c2a3b5fe981af111f8401ad
Reviewed-on: https://boringssl-review.googlesource.com/12302
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We missed that the TLS 1.3 code was inconsistent with the TLS 1.2 code.
Only on the server did we push an error code. But consistency between
client and server is probably worthwhile so, fix the 1.2 code to match
for now.
Change-Id: I17952c72048697dc66eacf0f144a66ced9cb3be8
Reviewed-on: https://boringssl-review.googlesource.com/12260
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>
Change-Id: I0767cd4801924170ce13b8143a9586485b8f78af
Reviewed-on: https://boringssl-review.googlesource.com/12280
Reviewed-by: Steven Valdez <svaldez@chromium.org>
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>
Change-Id: I07c4b67206440d169b314f24e1b3c1c697dda24f
Reviewed-on: https://boringssl-review.googlesource.com/12204
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>
TLS 1.3 adds a number of places with extensions blocks that don't easily
fit into our ClientHello/EncryptedExtensions callbacks. Between
HelloRetryRequest, ServerHello, draft 18 going nuts with Certificate,
and NewSessionTicket when we do 0-RTT, this passes the "abstract things
that are repeated three times" sniff test.
For now, it rejects unknown extensions, but it will probably grow an
allow_unknown parameter for NewSessionTicket.
This involves disabling some MSVC warnings, but they're invalid as of
C99 which we otherwise require. See
https://connect.microsoft.com/VisualStudio/feedback/details/1230248/remove-c99-related-warnings-or-make-them-off-by-default
Change-Id: Iea8bf8ab216270c081dd63e79aaad9ec73b3b550
Reviewed-on: https://boringssl-review.googlesource.com/12233
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>
BUG=112
Change-Id: I88ef17e32e33b091ff1e27b7950f88e1d48f9278
Reviewed-on: https://boringssl-review.googlesource.com/12239
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>
For TLS 1.3 draft 18, it will be useful to get at the full current
message and not just the body. Add a hook to expose it and replace
hash_current_message with a wrapper over it.
BUG=112
Change-Id: Ib9e00dd1b78e8b72e12409d85c80e96c5b411a8b
Reviewed-on: https://boringssl-review.googlesource.com/12238
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>
Certificate chain with intermediate taken from Chromium's tests. Though
it doesn't really matter because the runner tests don't verify
certificates.
BUG=70
Change-Id: I46fd1d4be0f371b5bfd43370b97d2c8053cfad60
Reviewed-on: https://boringssl-review.googlesource.com/12261
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We used to enforce after the version was set, but stopped enforcing with
TLS 1.3. NSS enforces the value for encrypted records, which makes sense
and avoids the problems gating it on have_version. Add tests for this.
Change-Id: I7fb5f94ab4a22e8e3b1c14205aa934952d671727
Reviewed-on: https://boringssl-review.googlesource.com/12143
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>
It's all of one bit, but having it on the SSL object means we need
manually to reset it on renego.
Change-Id: I989dacd430fe0fa63d76451b95f036a942aefcfe
Reviewed-on: https://boringssl-review.googlesource.com/12229
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>
dtls1_finish_message should NULL *out_msg before calling OPENSSL_free,
rather than asking ssl3_complete_message to do it. ssl3_finish_message
has no need to call it at all.
Change-Id: I22054217073690ab391cd19bf9993b1ceada41fd
Reviewed-on: https://boringssl-review.googlesource.com/12231
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is to allow for PSK binders to be munged into the ClientHello as part of
draft 18.
BUG=112
Change-Id: Ic4fd3b70fa45669389b6aaf55e61d5839f296748
Reviewed-on: https://boringssl-review.googlesource.com/12228
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>
We have AEAD-level coverage for these, but we should also test this in
the TLS stack, and at maximum size per upstream's CVE-2016-7054.
Change-Id: I1f4ad0356e793d6a3eefdc2d55a9c7e05ea08261
Reviewed-on: https://boringssl-review.googlesource.com/12187
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This change renames |peer| to |x509_peer| and |cert_chain| to
|x509_chain| in |SSL_SESSION|. It also renames |x509| to |x509_leaf| and
|chain| to |x509_chain| in |CERT|. (All with an eye to maybe making
them lazily initialised in the future).
This a) catches anyone who might be accessing these members directly and
b) makes space for |CRYPTO_BUFFER|-based values to take the unprefixed
names.
Change-Id: I10573304fb7d6f1ea03f9e645f7fc0acdaf71ac2
Reviewed-on: https://boringssl-review.googlesource.com/12162
Reviewed-by: David Benjamin <davidben@google.com>
In transition to removing it altogether, set SSL_MODE_NO_AUTO_CHAIN by
default. If we find some consumer was relying on it, this will allow
them to revert locally with SSL_(CTX_)clear_mode, but hopefully this was
just unused.
BUG=42
Change-Id: Iaf70a436a3324ce02e02dfb18213b6715c034ff2
Reviewed-on: https://boringssl-review.googlesource.com/12180
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
3c6a1ea674 switched what layer handled
the DTLS version mapping but forgot to correct the HelloVerifyRequest
logic to account for this.
Thanks to Jed Davis for noticing this.
Change-Id: I94ea18fc43a7ba15dd7250bfbcf44dbb3361b3ce
Reviewed-on: https://boringssl-review.googlesource.com/11984
Reviewed-by: David Benjamin <davidben@google.com>
This is already manually released at the end of the handshake. With this
change, it can happen implicitly, and SSL3_STATE shrinks further by
another pointer.
Change-Id: I94b9f2e4df55e8f2aa0b3a8799baa3b9a34d7ac1
Reviewed-on: https://boringssl-review.googlesource.com/12121
Reviewed-by: Adam Langley <agl@google.com>
Fuzzer mode explores the handshake, but at the cost of losing coverage
on the record layer. Add a separate build flag and client/server
corpora for this mode.
Note this requires tweaks in consumers' fuzzer build definitions.
BUG=111
Change-Id: I1026dc7301645e165a761068a1daad6eedc9271e
Reviewed-on: https://boringssl-review.googlesource.com/12108
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The distinction for full handshakes is not meaningful (the timestamp is
currently the start of the handshake), but for renewed sessions, we
currently retain the timestamp of the original issuance.
Instead, when minting or receiving tickets, adjust session->time and
session->timeout so that session->time is the ticket issuance time.
This is still not our final TLS 1.3 behavior (which will need a both
renewable and non-renewable times to honor the server ticket lifetime),
but it gets us closer and unblocks handling ticket_age_add from TLS 1.3
draft 18 and sends the correct NewSessionTicket lifetime.
This fixes the ticket lifetime hint which we emit on the server to
mirror the true ticket lifetime. It also fixes the TLS 1.3 server code
to not set the ticket lifetime hint. There is no need to waste ticket
size with it, it is no longer a "hint" in TLS 1.3, and even in the TLS
1.3 code we didn't fill it in on the server.
Change-Id: I140541f1005a24e53e1b1eaa90996d6dada1c3a1
Reviewed-on: https://boringssl-review.googlesource.com/12105
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>
If there is a malloc failure while assembling the ticket, call
CBB_cleanup. Also return -1 instead of 0; zero means EOF in the old
state machine and -1 means error. (Except enough of the stack gets it
wrong that consumers handle both, but we should fix this.)
Change-Id: I98541a9fa12772ec159f9992d1f9f53e5ca4cc5a
Reviewed-on: https://boringssl-review.googlesource.com/12104
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
There's no sense in flushing twice in one flight. This means when
writing a message is finally synchronous, we don't need the intermediate
state at all.
Change-Id: Iaca60d64917f82dce0456a8b15de4ee00f2d557b
Reviewed-on: https://boringssl-review.googlesource.com/12103
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 clarifies that a ticket lifetime of zero means the session is
unusable. We don't currently pay attention to that field (to be fixed in
later changes) but, in preparation for this, switch the >= to a >.
Change-Id: I0e67a0d97bc8def04914f121e84d3e7a2d640d2c
Reviewed-on: https://boringssl-review.googlesource.com/12102
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
These don't make sense and mean some SSL_SESSIONs serialize and
deserialize as different values. If we ever managed to create an
SSL_SESSION without a time, it would never expire because time always
gets set to time(NULL). If we ever created an SSL_SESSION with a zero
timeout, the timeout would be... three? Once we start adjusting
time/timeout to issuance time, driving timeout to zero is actually
plausible, so it should work properly.
Instead, make neither field optional. We always fill both out, so this
shouldn't have any effects. If it does, the only effect would be to
decline to resume some existing tickets which must have been so old that
we'd want them to have expired anyway.
Change-Id: Iee3620658c467dd6d96a2b695fec831721b03b5b
Reviewed-on: https://boringssl-review.googlesource.com/12101
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The values are long, so check for negative numbers.
Change-Id: I8fc7333edbed50dc058547a4b53bc10b234071b4
Reviewed-on: https://boringssl-review.googlesource.com/12100
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This business with |ok| is unnecessary. This function is still rather a
mess, but this is a small improvement.
Change-Id: I28fdf1a3687fe6a9d58d81a22cf2f8e7ce5b9b2c
Reviewed-on: https://boringssl-review.googlesource.com/12080
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
A renewed session does not refresh the timeout. Add tests for this in
preparation for future changes which will revise this logic.
Specifically, TLS 1.3 draft 18's ticket_age_add logic will require some
tweaks in lifetime tracking to record when the ticket was minted. We'll
also likely wish to tweak the parameters for 1.3 to account for (a)
ECDHE-PSK means we're only worried about expiring a short-circuited
authentication rather than forward secrecy and (b) two hours is too
short for a QUIC 0-RTT replacement.
Change-Id: I0f1edd09151e7fcb5aee2742ef8600fbd7080df6
Reviewed-on: https://boringssl-review.googlesource.com/12002
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 is only used in one place where we don't take advantage of it being
sorted anyway.
Change-Id: If6f0d04e975db903e8a93c57c869ea4964c0be37
Reviewed-on: https://boringssl-review.googlesource.com/12062
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is the last blocker within BoringSSL itself to opaquifying SSL.
(There are still blockers in consumers, of course.)
BUG=6
Change-Id: Ie3b8dcb78eeaa9aea7311406c5431a8625d60401
Reviewed-on: https://boringssl-review.googlesource.com/12061
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 ciphers are now always enabled and come with a hard-coded
preference order.
BUG=110
Change-Id: Idd9cb0d75fb6bf2676ecdee27d88893ff974c4a3
Reviewed-on: https://boringssl-review.googlesource.com/12025
Reviewed-by: Adam Langley <agl@google.com>
HTTP/2 requires TLS 1.2 so the negotiated version should be available
during the ALPN callback.
Change-Id: Iea332808b531a6e5c917de5b8c8917c0aa7428a1
Reviewed-on: https://boringssl-review.googlesource.com/12060
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>
They will get very confused about which key they're using. Any caller
using exporters must either (a) leave renegotiation off or (b) be very
aware of when renegotiations happen anyway. (You need to somehow
coordinate with the peer about which epoch's exporter to use.)
Change-Id: I921ad01ac9bdc88f3fd0f8283757ce673a47ec75
Reviewed-on: https://boringssl-review.googlesource.com/12003
Reviewed-by: Adam Langley <agl@google.com>
The existing tests for this codepath require us to reconfigure the shim.
This will not work when TLS 1.3 cipher configuration is detached from
the old cipher language. It also doesn't hit codepaths like sessions
containing a TLS 1.3 version but TLS 1.2 cipher.
Instead, add some logic to the runner to rewrite tickets and build tests
out of that.
Change-Id: I57ac5d49c3069497ed9aaf430afc65c631014bf6
Reviewed-on: https://boringssl-review.googlesource.com/12024
Reviewed-by: Adam Langley <agl@google.com>
It is not ignored.
Change-Id: I2e607a6d6f7444838fc6fa65cd18e9aa142f139f
Reviewed-on: https://boringssl-review.googlesource.com/12023
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
HTTP/2 places requirements on the cipher suite. So that servers can
decline HTTP/2 when these requirements aren't met, defer ALPN
negotiation.
See also b/32553041.
Change-Id: Idbcf049f9c8bda06a8be52a0154fe76e84607268
Reviewed-on: https://boringssl-review.googlesource.com/11982
Reviewed-by: Adam Langley <agl@google.com>
We were only testing one side.
Change-Id: Ieb755e27b235aaf1317bd2c8e5fb374cb0ecfdb3
Reviewed-on: https://boringssl-review.googlesource.com/12001
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Trim a few more bytes from the future QUIC ClientHello.
Change-Id: If23c5cd078889a9a26cf2231b51b17c2615a38ea
Reviewed-on: https://boringssl-review.googlesource.com/12000
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>
Get some of the duplicate logic out of the way.
Change-Id: Iee7c64577e14d1ddfead7e1e32c42c5c9f2a310d
Reviewed-on: https://boringssl-review.googlesource.com/11981
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>
TLS 1.3 also uses this extension and doesn't use any EC-based suites.
Always offering the extension is simpler. Also this gets an
SSL_get_ciphers call out of the way (that function is somewhat messy in
semantics).
Change-Id: I2091cb1046e0aea85caa76e73f50e8416e6ed94c
Reviewed-on: https://boringssl-review.googlesource.com/11980
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This change is based on interpreting TLS 1.3 draft 18.
Change-Id: I727961aff2f7318bcbbc8bf6d62b7d6ad3e62da9
Reviewed-on: https://boringssl-review.googlesource.com/11921
Reviewed-by: David Benjamin <davidben@google.com>
This should never happen, but the SSL_AEAD_CTX_new layer should enforce
key sizes as it's not locally obvious at the call site the caller didn't
get confused. There's still a mess of asserts below, but those should be
fixed by cutting the SSL_CIPHER/SSL_AEAD_CTX boundary differently.
(enc_key_len is validated by virtue of being passed into EVP_AEAD.)
BUG=chromium:659593
Change-Id: I8c91609bcef14ca1509c87aab981bbad6556975f
Reviewed-on: https://boringssl-review.googlesource.com/11940
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>
These were forward-declared for SSL3_STATE but with that hidden, it's no
longer necessary.
Change-Id: I8c548822f56f6172b4033b2fa89c038adcec2caa
Reviewed-on: https://boringssl-review.googlesource.com/11860
Reviewed-by: Adam Langley <agl@google.com>
Later work is going to cause some turbulence here.
Change-Id: Iba98bcf56e81492ec0dca54a381b38d1c115247a
Reviewed-on: https://boringssl-review.googlesource.com/11843
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 still rather a mess with how it's tied to SSL_AEAD_CTX_new
(probably these should get encapsulated in an SSL_AEAD struct), but this
avoids running the TLS 1.3 nonce logic on fake AEADs. This is impossible
based on cipher version checks, but we shouldn't need to rely on it.
It's also a little tidier since out_mac_secret_len is purely a function
of algorithm_mac.
BUG=chromium:659593
Change-Id: Icc24d43c54a582bcd189d55958e2d232ca2db4dd
Reviewed-on: https://boringssl-review.googlesource.com/11842
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This shouldn't happen, but it is good to check to avoid the potential
underflow in ssl_session_is_time_valid.
This required tweaking the mock clock in bssl_shim to stop going back in
time.
Change-Id: Id3ab8755139e989190d0b53d4bf90fe1ac203022
Reviewed-on: https://boringssl-review.googlesource.com/11841
Reviewed-by: David Benjamin <davidben@google.com>
