This is kind of annoying (even new state is needed to keep the layering
right). As part of aligning the read paths of the TLS 1.2 and TLS 1.3
state machine, we'll want to move to states calling
ssl_hash_current_message when the process the message, rather than when
the message is read. Right now the TLS 1.2 optional message story
(reuse_message) depends on all messages preceded by an optional message
using ssl_hash_message. For instance, if TLS 1.2 decided to place
CertificateStatus before ServerKeyExchange, we would not be able to
handle it.
However, V2ClientHello, by being handled in the message layer, relies on
ssl_get_message-driven hashing to replace the usual ClientHello hash
with a hash of something custom. This switches things so rather than
ClientHellos being always pre-hashed by the message layer, simulated
ClientHellos no-op ssl_hash_current_message.
This just replaces one hack with another (V2ClientHello is inherently
nasty), but this hack should be more compatible with future plans.
BUG=128
Change-Id: If807ea749d91e306a37bb2362ecc69b84bf224c9
Reviewed-on: https://boringssl-review.googlesource.com/13265
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 aligns the TLS 1.2 state machine closer with the TLS 1.3 state
machine. This is more work for the handshake, but ultimately the
plan is to take the ssl_get_message call out of the handshake (so it is
just the state machine rather than calling into BIO), so the parameters
need to be folded out as in TLS 1.3.
The WrongMessageType-* family of tests should make sure we don't miss
one of these.
BUG=128
Change-Id: I17a1e6177c52a7540b2bc6b0b3f926ab386c4950
Reviewed-on: https://boringssl-review.googlesource.com/13264
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>
If an existing chain had a NULL placeholder for a leaf we could end up
trying to increment its reference count. That results in a crash at
configuration time. Found via the SSL_CTX API fuzzer.
BUG=oss-fuzz:480
Change-Id: I0ddc2cbde2e625015768f1bdc8da625e8a4f05fd
Reviewed-on: https://boringssl-review.googlesource.com/13383
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 originally changed so that flush_flight could forward BIO_write
errors as-is, but we can and probably should still map BIO_flush errors.
This is unlikely to matter (every relevant BIO likely just has a no-op
flush which returns one), but, e.g., our file BIO returns 0, not -1, on
error.
We possibly should also be mapping BIO_write errors, but I'll leave that
alone for now. It's primarily BIO_read where the BIO return value must
be preserved due to error vs. EOF.
(We probably can just remove the BIO_flush calls altogether, but since
the buffer BIO forwarded the flush signal it would be a user-visible
behavior change to confirm.)
Change-Id: Ib495cc5d043867cf964f99b7ee4535114f7b2230
Reviewed-on: https://boringssl-review.googlesource.com/13367
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
(I split this change off to minimise the noise in future diffs that
actually do something meaningful.)
Change-Id: I7a054dcfc90a44ab5bb89b8f46704e5e3410e524
Reviewed-on: https://boringssl-review.googlesource.com/13364
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This change converts the |CERT| struct to holding certificates as binary
blobs, rather than in parsed form. The members for holding the parsed
form are still there, however, but are only used as a cache for the
event that someone asks us for a non-owning pointer to the parsed leaf
or chain.
Next steps:
* Move more functions in to ssl_x509.c
* Create an X509_OPS struct of function pointers that will hang off
the |SSL_METHOD| to abstract out the current calls to crypto/x509
operations.
BUG=chromium:671420
Change-Id: Ifa05d88c49a987fd561b349705c9c48f106ec868
Reviewed-on: https://boringssl-review.googlesource.com/13280
Reviewed-by: Adam Langley <agl@google.com>
This resolves a TODO, trims per-connection memory, and makes more sense.
These masks have nothing to do with certificate configuration.
Change-Id: I783e6158e51f58cce88e3e68dfa0ed965bdc894c
Reviewed-on: https://boringssl-review.googlesource.com/13368
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>
Until we've gotten it fully working, we should not mint any of these
SSL_SESSIONs, to avoid constraining future versions of our client code.
Notably, if any of our TLS 1.3 clients today serialized sessions, we
would need to rev the serialization format. Without opting into 0-RTT, a
TLS 1.3 client will create SSL_SESSIONs tagged as 0-RTT-capable but
missing important fields (ALPN, etc.). When that serialized session
makes its way to a future version of our client code, it would disagree
with the server about the ALPN value stored in the ticket and cause
interop failures.
I believe the only client code enabling TLS 1.3 right now is Chrome, and
the window is small, so it should be fine. But fix this now before it
becomes a problem.
Change-Id: Ie2b109f8d158017a6f3b4cb6169050d38a66b31c
Reviewed-on: https://boringssl-review.googlesource.com/13342
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>
Change-Id: I324743e7d1864fbbb9653209ff93e4da872c8d31
Reviewed-on: https://boringssl-review.googlesource.com/13340
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: Idc93fdca2f1c5c23e4ba48c4efed2edbad1e857b
Reviewed-on: https://boringssl-review.googlesource.com/12521
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>
The TLS 1.2 state machine now looks actually much closer to the TLS 1.3
one on the write side. Although the write states still have a BIO-style
return, they don't actually send anything anymore. Only the BIO flush
state does. Reads are still integrated into the states themselves
though, so I haven't made it match TLS 1.3 yet.
BUG=72
Change-Id: I7708162efca13cd335723efa5080718a5f2808ab
Reviewed-on: https://boringssl-review.googlesource.com/13228
Reviewed-by: Adam Langley <agl@google.com>
The SSL code suffers from needing too many verbs for variations on
writing things without actually writing them. We used to have queuing
the message up to be written to the buffer BIO, writing to the buffer
BIO, and flushing the buffer BIO. (Reading, conversely, has a similar
mess of verbs.)
Now we just have adding to the pending flight and flushing the pending
flight, match the SSL_PROTOCOL_METHOD naming.
BUG=72
Change-Id: I332966928bf13f03dfb8eddd519c2fefdd7f24d4
Reviewed-on: https://boringssl-review.googlesource.com/13227
Reviewed-by: Adam Langley <agl@google.com>
Large chunks of contiguous messages can now be sent in a row. Notably,
the ServerHello flight involves a number of optional messages which can
now be collapsed into straight-line code.
BUG=72
Change-Id: I1429d22a12401aa0f811a04e495bd5d754c084a4
Reviewed-on: https://boringssl-review.googlesource.com/13226
Reviewed-by: Adam Langley <agl@google.com>
On the TLS side, we introduce a running buffer of ciphertext. Queuing up
pending data consists of encrypting the record into the buffer. This
effectively reimplements what the buffer BIO was doing previously, but
this resizes to fit the whole flight.
As part of this, rename all the functions to add to the pending flight
to be more uniform. This CL proposes "add_foo" to add to the pending
flight and "flush_flight" to drain it.
We add an add_alert hook for alerts but, for now, only the SSL 3.0
warning alert (sent mid-handshake) uses this mechanism. Later work will
push this down to the rest of the write path so closure alerts use it
too, as in DTLS. The intended end state is that all the ssl_buffer.c and
wpend_ret logic will only be used for application data and eventually
optionally replaced by the in-place API, while all "incidental" data
will be handled internally.
For now, the two buffers are mutually exclusive. Moving closure alerts
to "incidentals" will change this, but flushing application data early
is tricky due to wpend_ret. (If we call ssl_write_buffer_flush,
do_ssl3_write doesn't realize it still has a wpend_ret to replay.) That
too is all left alone in this change.
To keep the diff down, write_message is retained for now and will be
removed from the state machines in a follow-up change.
BUG=72
Change-Id: Ibce882f5f7196880648f25d5005322ca4055c71d
Reviewed-on: https://boringssl-review.googlesource.com/13224
Reviewed-by: Adam Langley <agl@google.com>
Instead, "writing" a message merely adds it to the outgoing_messages
structure. The code to write the flight then loops over it all and now
shares code with retransmission. The verbs here are all a little odd,
but they'll be fixed in later commits.
In doing so, this fixes a slight miscalculation of the record-layer
overhead when retransmitting a flight that spans two epochs. (We'd use
the encrypted epoch's overhead for the unencrypted epoch.)
BUG=72
Change-Id: I8ac897c955cc74799f8b5ca6923906e97d6dad17
Reviewed-on: https://boringssl-review.googlesource.com/13223
Reviewed-by: Adam Langley <agl@google.com>
This was replaced by the more general CLIENT_RANDOM scheme that records
the master secret. Support was added in Wireshark 1.8.0, released in
June 2012. At this point, ECDHE is sufficiently widely deployed that
anyone that cares about this feature must have upgraded their Wireshark
by now.
Change-Id: I9b708f245ec8728c1999daf91aca663be7d25661
Reviewed-on: https://boringssl-review.googlesource.com/13263
Reviewed-by: David Benjamin <davidben@google.com>
This will let us avoid a scratch buffer when assembling DTLS handshake
packets in the write_message-less flow.
BUG=72
Change-Id: I15e78efe3a9e3933c307e599f0043427330f4a9e
Reviewed-on: https://boringssl-review.googlesource.com/13262
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We need to suppress a few tests on the system Android build until
RSA-PSS is shipped there.
Change-Id: I5843997aae9fa499ec08d76f44fdf3b523599e1c
Reviewed-on: https://boringssl-review.googlesource.com/13267
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ifc28887cbf91c7a80bdaf56e3bf80b2f8cfa7d53
Reviewed-on: https://boringssl-review.googlesource.com/13260
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Eventually, all uses of crypto/x509 will be from ssl_x509.c, but this is
just a start.
Change-Id: I2f38cdcbf18b1f26add0aac10a70af10a79dee0e
Reviewed-on: https://boringssl-review.googlesource.com/13242
Reviewed-by: Adam Langley <agl@google.com>
This is the first part to removing the buffer BIO. The eventual end
state is the SSL_PROTOCOL_METHOD is responsible for maintaining one
flight's worth of messages. In TLS, it will just be a buffer containing
the flight's ciphertext. In DTLS, it's the existing structure for
retransmit purposes. There will be hooks:
- add_message (synchronous)
- add_change_cipher_spec (synchronous)
- add_warning_alert (synchronous; needed until we lose SSLv3 client auth
and TLS 1.3 draft 18; draft 19 will switch end_of_early_data to a
handshake message)
- write_flight (BIO; flush_flight will be renamed to this)
This also preserves the exact return value of BIO_flush. Eventually all
the BIO_write calls will be hidden behind BIO_flush to, to be consistent
with other BIO-based calls, preserve the return value.
BUG=72
Change-Id: I74cd23759a17356aab3bb475a8ea42bd2cd115c9
Reviewed-on: https://boringssl-review.googlesource.com/13222
Reviewed-by: Adam Langley <agl@google.com>
EVP_parse_public_key already acts like CBS_get_* in that it peels one
element off and leaves a remainder.
Change-Id: Ic90952785005ed81664a6f46503b13ecd293176c
Reviewed-on: https://boringssl-review.googlesource.com/13045
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
For now, this is the laziest conversion possible. The intent is to just
get the build setup ready so that we can get everything working in our
consumers. The intended end state is:
- The standalone build produces three test targets, one per library:
{crypto,ssl,decrepit}_tests.
- Each FOO_test is made up of:
FOO/**/*_test.cc
crypto/test/gtest_main.cc
test_support
- generate_build_files.py emits variables crypto_test_sources and
ssl_test_sources. These variables are populated with FindCFiles,
looking for *_test.cc.
- The consuming file assembles those variables into the two test targets
(plus decrepit) from there. This avoids having generate_build_files.py
emit actual build rules.
- Our standalone builders, Chromium, and Android just run the top-level
test targets using whatever GTest-based reporting story they have.
In transition, we start by converting one of two tests in each library
to populate the three test targets. Those are added to all_tests.json
and all_tests.go hacked to handle them transparently. This keeps our
standalone builder working.
generate_build_files.py, to start with, populates the new source lists
manually and subtracts them out of the old machinery. We emit both for
the time being. When this change rolls in, we'll write all the build
glue needed to build the GTest-based tests and add it to consumers'
continuous builders.
Next, we'll subsume a file-based test and get the consumers working with
that. (I.e. make sure the GTest targets can depend on a data file.)
Once that's all done, we'll be sure all this will work. At that point,
we start subsuming the remaining tests into the GTest targets and,
asynchronously, rewriting tests to use GTest properly rather than
cursory conversion here.
When all non-GTest tests are gone, the old generate_build_files.py hooks
will be removed, consumers updated to not depend on them, and standalone
builders converted to not rely on all_tests.go, which can then be
removed. (Unless bits end up being needed as a malloc test driver. I'm
thinking we'll want to do something with --gtest_filter.)
As part of this CL, I've bumped the CMake requirements (for
target_include_directories) and added a few suppressions for warnings
that GTest doesn't pass.
BUG=129
Change-Id: I881b26b07a8739cc0b52dbb51a30956908e1b71a
Reviewed-on: https://boringssl-review.googlesource.com/13232
Reviewed-by: Adam Langley <agl@google.com>
It's not completely clear to me why select_cetificate_cb behaves the way it
does, however not only is it confusing, but it makes assumptions about the
application using BoringSSL (it's not always possible to implement custom
logic outside of the callbacks provided by libssl), that make this callback
somewhat useless.
Case in point, the callback can be used for changing min/max protocol versions
based on per-site policies, and select_certificate_cb is the only place where
SSL_set_min/max_proto_version() can be used (e.g. you can't call them in
cert_cb because it's too late), but the decision on the specific versions to
use might depend on configuration that needs retrieving asynchronously from
over the network, which requires re-running the callback multiple times.
Change-Id: Ia8e151b163628545373e7fd1f327e9af207478a6
Reviewed-on: https://boringssl-review.googlesource.com/13000
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We have a test somewhere which tries to read off of it. Align the getter
roughly with upstream's SSL_SESSION_get0_id_context (which we don't
currently expose).
BUG=6
Change-Id: Iab240868838ba56c1f08d112888d9536574347b4
Reviewed-on: https://boringssl-review.googlesource.com/12636
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>
Before RFC 7539 we had a ChaCha20-Poly1305 cipher suite that had a 64/64
nonce/counter split (as DJB's original ChaCha20 did). RFC 7539 changed
that to 96/32 and we've supported both for some time.
This change removes the old version and the TLS cipher suites that used
it.
BUG=chromium:682816
Change-Id: I2345d6db83441691fe0c1ab6d7c6da4d24777849
Reviewed-on: https://boringssl-review.googlesource.com/13203
Reviewed-by: Adam Langley <agl@google.com>
This reverts commit def9b46801.
(I should have uploaded a new version before sending to the commit queue.)
Change-Id: Iaead89c8d7fc1f56e6294d869db9238b467f520a
Reviewed-on: https://boringssl-review.googlesource.com/13202
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>
TLS 1.3 forbids warning alerts, and sending these is a bad idea. Per RFC
6066:
If the server understood the ClientHello extension but
does not recognize the server name, the server SHOULD take one of two
actions: either abort the handshake by sending a fatal-level
unrecognized_name(112) alert or continue the handshake. It is NOT
RECOMMENDED to send a warning-level unrecognized_name(112) alert,
because the client's behavior in response to warning-level alerts is
unpredictable.
The motivation is to cut down on the number of places where we send
non-closing alerts. We can't remove them yet (SSL 3.0 and TLS 1.3 draft
18 need to go), but eventually this can be a simplifying assumption.
Already this means DTLS never sends warning alerts, which is good
because DTLS can't retransmit them.
Change-Id: I577a1eb9c23e66d28235c0fbe913f00965e19486
Reviewed-on: https://boringssl-review.googlesource.com/13221
Reviewed-by: Adam Langley <agl@google.com>
This doesn't do anything useful. Every caller either never sets the
callback as a client or goes out of their way to filter out clients in
the callback.
Change-Id: I6f07d000a727f9ccba080f812e6b8e7a38e04350
Reviewed-on: https://boringssl-review.googlesource.com/13220
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Before RFC 7539 we had a ChaCha20-Poly1305 cipher suite that had a 64/64
nonce/counter split (as DJB's original ChaCha20 did). RFC 7539 changed
that to 96/32 and we've supported both for some time.
This change removes the old version and the TLS cipher suites that used
it.
Change-Id: Icd9c2117c657f3aa6df55990c618d562194ef0e8
Reviewed-on: https://boringssl-review.googlesource.com/13201
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This is to make sure all of libssl's consumers' have sufficiently
reasonable toolchains. Once this bakes, we can go about moving
libssl to C++.
This is just starting with libssl for now because libcrypto has more
consumers and libssl would benefit more from C++ than libcrypto (though
libcrypto also has code that would benefit).
BUG=132
Change-Id: Ie02f7b0a8a95defd289cc7e62451d4b16408ca2a
Reviewed-on: https://boringssl-review.googlesource.com/13161
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ia6598ee4b2d4623abfc140d6a5c0eca4bcb30427
Reviewed-on: https://boringssl-review.googlesource.com/13180
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 are no longer needed.
Change-Id: I909f7d690f57dafcdad6254948b5683757da69f4
Reviewed-on: https://boringssl-review.googlesource.com/13160
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The last one was an RC4 cipher and those are gone.
Change-Id: I3473937ff6f0634296fc75a346627513c5970ddb
Reviewed-on: https://boringssl-review.googlesource.com/13108
Reviewed-by: Adam Langley <agl@google.com>
This gives coverage over needing to fragment something over multiple
records.
Change-Id: I2373613608ef669358d48f4e12f68577fa5a40dc
Reviewed-on: https://boringssl-review.googlesource.com/13101
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 doesn't support renegotiation in the first place, but so callers
don't report TLS 1.3 servers as missing it, always report it as
(vacuously) protected against this bug.
BUG=chromium:680281
Change-Id: Ibfec03102b2aec7eaa773c331d6844292e7bb685
Reviewed-on: https://boringssl-review.googlesource.com/13046
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>
08b65f4e31 introduced a memory leak and
also got enums confused. Also fix a codepath that was missing an error
code.
Thanks to OSS-Fuzz which appears to have found it in a matter of hours.
Change-Id: Ia9e926c28a01daab3e6154d363d0acda91209a22
Reviewed-on: https://boringssl-review.googlesource.com/13104
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This adds support for setting 0-RTT mode on tickets minted by
BoringSSL, allowing for testing of the initial handshake knowledge.
BUG=76
Change-Id: Ic199842c03b5401ef122a537fdb7ed9e9a5c635a
Reviewed-on: https://boringssl-review.googlesource.com/12740
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>
As long as we still have this code, we should make sure it doesn't
regress.
Change-Id: I0290792aedcf667ec49b251d747ffbc141c0cec4
Reviewed-on: https://boringssl-review.googlesource.com/13053
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
SSL_write is remarkably complicated.
Change-Id: I1cb8d00af1b4c5e2d90187d5f87951f25e27f224
Reviewed-on: https://boringssl-review.googlesource.com/13050
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>
Instead, add ssl_has_certificate to the ssl3_send_cert_verify check. If
writing the empty Certificate does not complete synchronously (it almost
always does due to the buffer BIO), but if the buffer boundary is at
exactly the wrong place, write_message will need a retry but, having
cleared cert_request, we never re-enter ssl3_send_client_certificate.
This will later be moot when we've gotten rid of the buffer BIO, but
this is cleaner anyway and is closer to the TLS 1.3 code.
With this change, blindly taking away the BIO buffer in TLS (which is
not what we want since we want the entire flight in one write but is a
nice sanity check), only the SSL 3.0 no client certificate tests fail.
They too rely on some writes completing synchronously due to SSL 3.0
sending a warning alert. There is a similar bug when
tlsext_servername_callback returns SSL_TLSEXT_ERR_ALERT_WARNING.
Those will be resolved after reworking the write path since it's a bit
of a mess.
Change-Id: I56b4df6163cae1df263cf36f0d93046d0375a5ac
Reviewed-on: https://boringssl-review.googlesource.com/13052
Reviewed-by: David Benjamin <davidben@google.com>
We repeat this in a bunch of places.
Change-Id: Iee2c95a13e1645453f101d8be4be9ac78d520387
Reviewed-on: https://boringssl-review.googlesource.com/13051
Reviewed-by: David Benjamin <davidben@google.com>
The write path for TLS is going to need some work. There are some fiddly
cases when there is a write in progress. Start adding tests to cover
this logic.
Later I'm hoping we can extend this flag so it drains the unfinished
write and thus test the interaction of read/write paths in 0-RTT. (We
may discover 1-RTT keys while we're in the middle of writing data.)
Change-Id: Iac2c417e4b5e84794fb699dd7cbba26a883b64ef
Reviewed-on: https://boringssl-review.googlesource.com/13049
Reviewed-by: Adam Langley <agl@google.com>
Channel ID is already enabled on the SSL. This dates to
49c7af1c42 which converted an instance of
tlsext_channel_id_enabled_new to it, but tlsext_channel_id_enabled_new
meant "if Channel ID is enabled, use the new one", not "enable Channel
ID".
Thanks to Eric Rescorla for catching this.
Change-Id: I2d5a82b930ffcbe5527a62a9aa5605ebb71a6b9f
Reviewed-on: https://boringssl-review.googlesource.com/13042
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Rather than doing it right before outputing, treat this as a part of the
pipeline to finalize the certificate chain, and run it right after
cert_cb to modify the certificate configuration itself. This means
nothing else in the stack needs to worry about this case existing.
It also makes it easy to support in both TLS 1.2 and TLS 1.3.
Change-Id: I6a088297a54449f1f5f5bb8b5385caa4e8665eb6
Reviewed-on: https://boringssl-review.googlesource.com/12966
Reviewed-by: Adam Langley <agl@google.com>
We have this function now. Probably good to use it.
Change-Id: I00fe1f4cf5c8cb6f61a8f6600cac4667e95ad7f3
Reviewed-on: https://boringssl-review.googlesource.com/13040
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>
Upstream accidentally started rejecting server-sent point formats in
https://github.com/openssl/openssl/issues/2133. Our own test coverage
here is also lacking, so flesh it out.
Change-Id: I99059558bd28d3a540c9687649d6db7e16579d29
Reviewed-on: https://boringssl-review.googlesource.com/12979
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>
Change-Id: Ie4c566c29c20faac7a9a5e04c88503fc2e1ff4db
Reviewed-on: https://boringssl-review.googlesource.com/12970
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
Alas, wpa_supplicant relies on the auto-chaining feature, so we can't
easily remove it. Write tests for it to ensure it stays working.
These test currently do not work for TLS 1.3 because the feature is
broken in 1.3. A follow-up change will fix this.
BUG=70
Change-Id: I2c04f55b712d66f5af1556254a5b017ebc3244f7
Reviewed-on: https://boringssl-review.googlesource.com/12965
Reviewed-by: Adam Langley <agl@google.com>
This extension will be used to test whether
https://github.com/tlswg/tls13-spec/pull/762 is deployable against
middleboxes. For simplicity, it is mutually exclusive with 0-RTT. If
client and server agree on the extension, TLS 1.3 records will use the
format in the PR rather than what is in draft 18.
BUG=119
Change-Id: I1372ddf7b328ddf73d496df54ac03a95ede961e1
Reviewed-on: https://boringssl-review.googlesource.com/12684
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>
Most C standard library functions are undefined if passed NULL, even
when the corresponding length is zero. This gives them (and, in turn,
all functions which call them) surprising behavior on empty arrays.
Some compilers will miscompile code due to this rule. See also
https://www.imperialviolet.org/2016/06/26/nonnull.html
Add OPENSSL_memcpy, etc., wrappers which avoid this problem.
BUG=23
Change-Id: I95f42b23e92945af0e681264fffaf578e7f8465e
Reviewed-on: https://boringssl-review.googlesource.com/12928
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We were only asserting on the shim-side error code.
Change-Id: Idc08c253a7723a2a7fd489da761a56c72f7a3b96
Reviewed-on: https://boringssl-review.googlesource.com/12923
Reviewed-by: Adam Langley <agl@google.com>
It should probably have a TLS 1.3 in the name to be clear that's what
it's testing.
Change-Id: I50b5f503a8038715114136179bde83e7da064e9b
Reviewed-on: https://boringssl-review.googlesource.com/12961
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>
Notably, Conscrypt uses SSL_SESSION_get_version, so we should have tests
for it.
Change-Id: I670f1b1b9951f840f27cb62dd36ef4f05042c974
Reviewed-on: https://boringssl-review.googlesource.com/12881
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 in preparation for implementing 0-RTT where, like
with client_traffic_secret_0, client_handshake_secret must
be derived slightly earlier than it is used. (The secret is
derived at ServerHello, but used at server Finished.)
Change-Id: I6a186b84829800704a62fda412992ac730422110
Reviewed-on: https://boringssl-review.googlesource.com/12920
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>
There are no longer any consumers of these APIs.
These were useful back when the CBC vs. RC4 tradeoff varied by version
and it was worth carefully tuning this cutoff. Nowadays RC4 is
completely gone and there's no use in configuring these anymore.
To avoid invalidating the existing ssl_ctx_api corpus and requiring it
regenerated, I've left the entries in there. It's probably reasonable
for new API fuzzers to reuse those slots.
Change-Id: I02bf950e3828062341e4e45c8871a44597ae93d5
Reviewed-on: https://boringssl-review.googlesource.com/12880
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This dates to
62d05888d1
which intended to be removed in a later Android release once X25519 was
added. That has since happened.
This intentionally leaves the P-521 hooked up for now. Detaching it
completely is a more aggressive change (since it's slightly tied up with
SHA-512) that should wait until removing ECDSA+SHA512 has stuck in Chrome.
Change-Id: I04553c3eddf33a13b6e3e9a6e7ac4c4725676cb0
Reviewed-on: https://boringssl-review.googlesource.com/10923
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 function always returns the full chain and will hopefully eliminate
the need for some code in Conscrypt.
Change-Id: Ib662005322c40824edf09d100a784ff00492896a
Reviewed-on: https://boringssl-review.googlesource.com/12780
Reviewed-by: Adam Langley <agl@google.com>
This removes another dependency on the crypto/x509 code.
Change-Id: Ia72da4d47192954c2b9a32cf4bcfd7498213c0c7
Reviewed-on: https://boringssl-review.googlesource.com/12709
Reviewed-by: Adam Langley <agl@google.com>
The loop is getting a little deeply nested and hard to read.
Change-Id: I3a99fba54c2f352850b83aef91ab72d5d9aabfb8
Reviewed-on: https://boringssl-review.googlesource.com/12685
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>
Also fix the error code. It's a missing extension, not an unexpected
one.
Change-Id: I48e48c37e27173f6d7ac5e993779948ead3706f2
Reviewed-on: https://boringssl-review.googlesource.com/12683
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>
So we can report it cleanly out of DevTools, it should behave like
SSL_get_curve_id and be reported on resumption too.
BUG=chromium:658905
Change-Id: I0402e540a1e722e09eaebadf7fb4785d8880c389
Reviewed-on: https://boringssl-review.googlesource.com/12694
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>
Also test that TLS 1.3 can be resumed at a different curve.
Change-Id: Ic58e03ad858c861958b7c934813c3e448fb2829c
Reviewed-on: https://boringssl-review.googlesource.com/12692
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>
The only accessor for this field is the group/curve ID. Switch to only
storing that so no cipher checks are needed to interpret it. Instead,
ignore older values at parse time.
Change-Id: Id0946d4ac9e7482c69e64cc368a9d0cddf328bd3
Reviewed-on: https://boringssl-review.googlesource.com/12693
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>
Nothing calls this anymore. DHE is nearly gone. This unblocks us from
making key_exchange_info only apply to the curve.
Change-Id: I3099e7222a62441df6e01411767d48166a0729b1
Reviewed-on: https://boringssl-review.googlesource.com/12691
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 change removes the use of |X509_get_pubkey| from the TLS <= 1.2
code. That function is replaced with a shallow parse of the certificate
to extract the public key instead.
Change-Id: I8938c6c5a01b32038c6b6fa58eb065e5b44ca6d2
Reviewed-on: https://boringssl-review.googlesource.com/12707
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 currently only works for certificates parsed from the network, but
if making several connections that share certificates, some KB of memory
might be saved.
BUG=chromium:671420
Change-Id: I1c7a71d84e1976138641f71830aafff87f795f9d
Reviewed-on: https://boringssl-review.googlesource.com/12706
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 change adds a STACK_OF(CRYPTO_BUFFER) to an SSL_SESSION which
contains the raw form of the received certificates. The X509-based
members still exist, but their |enc| buffer will alias the
CRYPTO_BUFFERs.
(This is a second attempt at
https://boringssl-review.googlesource.com/#/c/12163/.)
BUG=chromium:671420
Change-Id: I508a8a46cab89a5a3fcc0c1224185d63e3d59cb8
Reviewed-on: https://boringssl-review.googlesource.com/12705
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>
OpenSSL includes a leaf certificate in a certificate chain when it's a
client, but doesn't when it's a server. This is also reflected in the
serialisation of sessions.
This change makes the internal semantics consistent: the leaf is always
included in the chain in memory, and never duplicated when serialised.
To maintain the same API, SSL_get_peer_cert_chain will construct a copy
of the chain without the leaf if needed.
Since the serialised format of a client session has changed, an
|is_server| boolean is added to the ASN.1 that defaults to true. Thus
any old client sessions will be parsed as server sessions and (silently)
discarded by a client.
Change-Id: Ibcf72bc8a130cedb423bc0fd3417868e0af3ca3e
Reviewed-on: https://boringssl-review.googlesource.com/12704
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>
state is now initialized to SSL_ST_INIT in SSL_HANDSHAKE. If there is no
handshake present, we report SSL_ST_OK. This saves 8 bytes of
per-connection post-handshake memory.
Change-Id: Idb3f7031045caed005bd7712bc8c4b42c81a1d04
Reviewed-on: https://boringssl-review.googlesource.com/12697
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 to free up moving ssl->state into SSL_HANDSHAKE. ssl->state
serves two purposes right now. First, it is the state tracking for
SSL_HANDSHAKE. Second, it lets the system know there is a handshake
waiting to complete.
Instead, arrange things so that, if there is a handshake waiting to
complete, hs is not NULL. That means we need to initialize it when
creating a new connection and when discovering a renego.
Note this means we cannot make initializing an SSL_HANDSHAKE depend on
client vs. server.
Change-Id: I585a8d7e700c4ffe4d372248d34c44106ad7e7a0
Reviewed-on: https://boringssl-review.googlesource.com/12696
Reviewed-by: David Benjamin <davidben@google.com>
This avoids needing a extra state around client certificates to avoid
calling the callbacks twice. This does, however, come with a behavior
change: configuring both callbacks won't work. No consumer does this.
(Except bssl_shim which needed slight tweaks.)
Change-Id: Ia5426ed2620e40eecdcf352216c4a46764e31a9a
Reviewed-on: https://boringssl-review.googlesource.com/12690
Reviewed-by: Adam Langley <agl@google.com>
This is never used. Removing it allows us to implement the old callback
using the new one.
Change-Id: I4be70cc16e609ce79b51836c19fec565c67ff3d6
Reviewed-on: https://boringssl-review.googlesource.com/12689
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reduce the amount of boilerplate needed to add more of these. Also tidy
things up a little.
Change-Id: I90ea7f70dba5a2b38a1fb716faff97eb4f6afafc
Reviewed-on: https://boringssl-review.googlesource.com/12687
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is to free up the hs->state name for the upper-level handshake
state.
Change-Id: I1183a329f698c56911f3879a91809edad5b5e94e
Reviewed-on: https://boringssl-review.googlesource.com/12695
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: Iaac633616a54ba1ed04c14e4778865c169a68621
Reviewed-on: https://boringssl-review.googlesource.com/12703
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 reverts commits 5a6e616961 and
e8509090cf. I'm going to unify how the
chains are kept in memory between client and server first otherwise the
mess just keeps growing.
Change-Id: I76df0d94c9053b2454821d22a3c97951b6419831
Reviewed-on: https://boringssl-review.googlesource.com/12701
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: Ie947ab176d10feb709c6e135d5241c6cf605b8e8
Reviewed-on: https://boringssl-review.googlesource.com/12700
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 currently only works for certificates parsed from the network, but
if making several connections that share certificates, some KB of memory
might be saved.
Change-Id: I0ea4589d7a8b5c41df225ad7f282b6d1376a8db4
Reviewed-on: https://boringssl-review.googlesource.com/12164
Reviewed-by: Adam Langley <alangley@gmail.com>
This change adds a STACK_OF(CRYPTO_BUFFER) to an SSL_SESSION which
contains the raw form of the received certificates. The X509-based
members still exist, but their |enc| buffer will alias the
CRYPTO_BUFFERs.
The serialisation format of SSL_SESSIONs is also changed, in a backwards
compatible way. Previously, some sessions would duplicate the leaf
certificate in the certificate chain. These sessions can still be read,
but will be written in a way incompatible with older versions of the
code. This should be fine because the situation where multiple versions
exchange serialised sessions is at the server, and the server doesn't
duplicate the leaf certifiate in the chain anyway.
Change-Id: Id3b75d24f1745795315cb7f8089a4ee4263fa938
Reviewed-on: https://boringssl-review.googlesource.com/12163
Reviewed-by: Adam Langley <alangley@gmail.com>
Right now the only way to set an OCSP response is SSL_CTX_set_ocsp_response
however this assumes that all the SSLs generated from a SSL_CTX share the
same OCSP response, which is wrong.
This is similar to the OpenSSL "function" SSL_get_tlsext_status_ocsp_resp,
the main difference being that this doesn't take ownership of the OCSP buffer.
In order to avoid memory duplication in case SSL_CTX has its own response,
a CRYPTO_BUFFER is used for both SSL_CTX and SSL.
Change-Id: I3a0697f82b805ac42a22be9b6bb596aa0b530025
Reviewed-on: https://boringssl-review.googlesource.com/12660
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>
All callers were long since updated.
Change-Id: Ibdc9b186076dfbcbc3bd7dcc72610c8d5a522cfc
Reviewed-on: https://boringssl-review.googlesource.com/12624
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>
Prior to 87eab4902d, due to some
confusions between configuration and connection state, SSL_clear had the
side effect of offering the previously established session on the new
connection.
wpa_supplicant relies on this behavior, so restore it for TLS 1.2 and
below and add a test. (This behavior is largely incompatible with TLS
1.3's post-handshake tickets, so it won't work in 1.3. It'll act as if
we configured an unresumable session instead.)
Change-Id: Iaee8c0afc1cb65c0ab7397435602732b901b1c2d
Reviewed-on: https://boringssl-review.googlesource.com/12632
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>
size_t at the public API, uint8_t on the SSL structs since everything
fits in there comfortably.
Change-Id: I837c3b21e04e03dfb957c1a3e6770300d0b49c0b
Reviewed-on: https://boringssl-review.googlesource.com/12638
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 is impossible to have an SSL* without a corresponding method.
Change-Id: Icaf826a06aaaa2c7caf98b1e4a950f9c1d48e6bd
Reviewed-on: https://boringssl-review.googlesource.com/12621
Reviewed-by: Adam Langley <agl@google.com>
There is no need to retain it beyond this point.
Change-Id: Ib5722ab30fc013380198b1582d1240f0fe0aa770
Reviewed-on: https://boringssl-review.googlesource.com/12620
Reviewed-by: Adam Langley <agl@google.com>
These too have no reason to be called across files.
Change-Id: Iee477e71f956c2fa0d8817bf2777cb3a81e1c853
Reviewed-on: https://boringssl-review.googlesource.com/12585
Reviewed-by: Adam Langley <agl@google.com>
s3_lib.c is nearly gone. ssl_get_cipher_preferences will fall away once
we remove the version-specific cipher lists. ssl_get_algorithm_prf and
the PRF stuff in general needs some revising (it was the motivation for
all the SSL_HANDSHAKE business). I've left ssl3_new / ssl3_free alone
for now because we don't have a good separation between common TLS/DTLS
connection state and state internal to the TLS SSL_PROTOCOL_METHOD.
Leaving that alone for now as there's lower-hanging fruit.
Change-Id: Idf7989123a387938aa89b6a052161c9fff4cbfb3
Reviewed-on: https://boringssl-review.googlesource.com/12584
Reviewed-by: Adam Langley <agl@google.com>
This was useful when we were transitioning NPN off in Chromium, but now
there are no callers remaining.
Change-Id: Ic619613d6d475eea6bc258c4a90148f129ea4a81
Reviewed-on: https://boringssl-review.googlesource.com/12637
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>
We've taken to writing bssl::UniquePtr in full, so it's not buying
us much.
Change-Id: Ia2689366cbb17282c8063608dddcc675518ec0ca
Reviewed-on: https://boringssl-review.googlesource.com/12628
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: Ida26e32a700c68e1899f9f6ccff73e2fa5252313
Reviewed-on: https://boringssl-review.googlesource.com/12633
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This allows a consumer to disable Channel ID (for instance, it may be
enabled on the SSL_CTX and later disabled on the SSL) without reaching
into the SSL struct directly.
Deprecate the old APIs in favor of these.
BUG=6
Change-Id: I193bf94bc1f537e1a81602a39fc2b9a73f44c73b
Reviewed-on: https://boringssl-review.googlesource.com/12623
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>
Each of these functions is called only once, but they're interspersed
between s3_lib.c and ssl_lib.c.
Change-Id: Ic496e364b091fc8e01fc0653fe73c83c47f690d9
Reviewed-on: https://boringssl-review.googlesource.com/12583
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>
It's our ClientHello representation. May as well name it accordingly.
Also switch away from calling the variable name ctx as that conflicts
with SSL_CTX.
Change-Id: Iec0e597af37137270339e9754c6e08116198899e
Reviewed-on: https://boringssl-review.googlesource.com/12581
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>
The various key schedule cleanups have removed the need for this enum.
Change-Id: I3269aa19b834815926ad56b2d919e21b5e2603fe
Reviewed-on: https://boringssl-review.googlesource.com/12582
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I1e28ba84de59336cab432d1db3dd9c6023909081
Reviewed-on: https://boringssl-review.googlesource.com/12625
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: Ie46d45cdb07c692a789594e13040a1ce9d6cf83d
Reviewed-on: https://boringssl-review.googlesource.com/12640
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>
The remaining direct accesses are in functions which expect to be called
in and out of the handshake. Accordingly, they are NULL-checked.
Change-Id: I07a7de6bdca7b6f8d09e22da11b8863ebf41389a
Reviewed-on: https://boringssl-review.googlesource.com/12343
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: I84a8ff1d717f3291403f6fc49668c84f89b910da
Reviewed-on: https://boringssl-review.googlesource.com/12342
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: I5ef0fe5cc3ae0d5029ae41db36e66d22d76f6158
Reviewed-on: https://boringssl-review.googlesource.com/12341
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: Id8543a88929091eb004a5205a30b483253cdaa25
Reviewed-on: https://boringssl-review.googlesource.com/12319
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 removes all explicit ssl->s3->hs access in those files.
Change-Id: I801ca1c894936aecef21e56ec7e7acb9d1b99688
Reviewed-on: https://boringssl-review.googlesource.com/12318
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 takes care of many of the explicit ssl->s3->hs accesses.
Change-Id: I380fae959f3a7021d6de9d19a4ca451b9a0aefe5
Reviewed-on: https://boringssl-review.googlesource.com/12317
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 cuts down on a lot of unchecked ssl->s3->hs accesses. Next is
probably the mass of extensions callbacks, and then we can play
whack-a-mole with git grep.
Change-Id: I81c506ea25c2569a51ceda903853465b8b567b0f
Reviewed-on: https://boringssl-review.googlesource.com/12237
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>
We currently look up SSL_HANDSHAKE off of ssl->s3->hs everywhere, but
this is a little dangerous. Unlike ssl->s3->tmp, ssl->s3->hs may not be
present. Right now we just know not to call some functions outside the
handshake.
Instead, code which expects to only be called during a handshake should
take an explicit SSL_HANDSHAKE * parameter and can assume it non-NULL.
This replaces the SSL * parameter. Instead, that is looked up from
hs->ssl.
Code which is called in both cases, reads from ssl->s3->hs. Ultimately,
we should get to the point that all direct access of ssl->s3->hs needs
to be NULL-checked.
As a start, manage the lifetime of the ssl->s3->hs in SSL_do_handshake.
This allows the top-level handshake_func hooks to be passed in the
SSL_HANDSHAKE *. Later work will route it through the stack. False Start
is a little wonky, but I think this is cleaner overall.
Change-Id: I26dfeb95f1bc5a0a630b5c442c90c26a6b9e2efe
Reviewed-on: https://boringssl-review.googlesource.com/12236
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>
finishedHash should keep a running secret and incorporate entropy as is
available.
Change-Id: I2d245897e7520b2317bc0051fa4d821c32eeaa10
Reviewed-on: https://boringssl-review.googlesource.com/12586
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>
Although we ignore all but the first identity, keep clients honest by
parsing the whole thing. Also explicitly check that the binder and
identity counts match.
Change-Id: Ib9c4caae18398360f3b80f8db1b22d4549bd5746
Reviewed-on: https://boringssl-review.googlesource.com/12469
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>
Due to recent changes, changing the SSL session timeout from cert_cb is
not possible anymore since the new |SSL_SESSION| is initialized *after*
cert_cb is run. The alternative would be using |SSL_CTX_set_timeout| but
the specific |SSL_CTX| could be shared by multiple |SSL|s.
Setting a value on a per-connection basis is useful in case timeouts
need to be calculated dynamically based on specific certificate/domain
information that would be retrieved from inside cert_cb (or other
callbacks).
It would also be possible to set the value to 0 to prevent session
resumption, which is not otherwise doable in the handshake callbacks.
Change-Id: I730a528c647f83f7f77f59b5b21d7e060e4c9843
Reviewed-on: https://boringssl-review.googlesource.com/12440
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=101
Change-Id: Ia1edbccee535b0bc3a0e18465286d5bcca240035
Reviewed-on: https://boringssl-review.googlesource.com/12470
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>
There is no more derivation step. We just use the resumption secret
directly. This saves us an unnecessary memcpy.
Change-Id: I203bdcc0463780c47cce655046aa1be560bb5b18
Reviewed-on: https://boringssl-review.googlesource.com/12472
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 fine because TLS PRFs only go up to SHA-384, but since
SSL_SESSION::master_key is sized to 48, not EVP_MAX_MD_SIZE, this should
explicitly check the bounds.
Change-Id: I2b1bcaab5cdfc3ce4d7a8b8ed5cc4c6d15d10270
Reviewed-on: https://boringssl-review.googlesource.com/12460
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>
Change-Id: I3a5d949eec9241ea43da40ce23e0e7f2a25e30e5
Reviewed-on: https://boringssl-review.googlesource.com/12381
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 aligns with ec_test which has a ForEachCurve helper and avoids
writing these loops all the time. As a bonus, these tests start working
in DTLS now.
Change-Id: I613fc08b641ddc12a819d8a1268a1e6a29043663
Reviewed-on: https://boringssl-review.googlesource.com/12380
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 change causes SSL_CTX_set_signed_cert_timestamp_list to check the
SCT list for shallow validity before allowing it to be set.
Change-Id: Ib8a1fe185224ff02ed4ce53a0109e60d934e96b3
Reviewed-on: https://boringssl-review.googlesource.com/12401
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
(Otherwise we end up touching potentially unwound stack.)
I looked into why our builders didn't catch this and it appears that, at
least with Clang 3.7, ASAN doesn't notice this. Perhaps Clang at that
version is being lazy about destructing the scoped CBB and so doesn't
actually go wrong.
Change-Id: Ia0f73e7eb662676439f024805fc8287a4e991ce0
Reviewed-on: https://boringssl-review.googlesource.com/12400
Reviewed-by: Adam Langley <agl@google.com>
It is not called outside of t1_enc.c.
Change-Id: Ifd9d109eeb432e931361ebdf456243c490b93ecf
Reviewed-on: https://boringssl-review.googlesource.com/12340
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>
Since the printed format for errors uses colons to separate different
parts of the error message, this was confusing.
Change-Id: I4742becec2bcb56ad8dc2fdb9a3bb23e4452d1b2
Reviewed-on: https://boringssl-review.googlesource.com/12361
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Commit-Queue: Adam Langley <alangley@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
We do not change ALPN on renego, so the value should carry over and not
be cleared.
Change-Id: Id54a083945542b4457d9c2787f0fe7c30239b76f
Reviewed-on: https://boringssl-review.googlesource.com/12306
Reviewed-by: David Benjamin <davidben@google.com>
If the function fails, it's an internal_error.
Change-Id: I4b7cf7a6ca2527f04b708303ab1bc71df762b55b
Reviewed-on: https://boringssl-review.googlesource.com/12312
Reviewed-by: David Benjamin <davidben@google.com>
It doesn't need to be exported out of t1_lib.c.
Change-Id: I000493e1e330457051da1719ca9f8152a4ff845a
Reviewed-on: https://boringssl-review.googlesource.com/12316
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>
Previously the option to retain only the SHA-256 hash of client
certificates could only be set at the |SSL_CTX| level. This change makes
|SSL| objects inherit the setting from the |SSL_CTX|, but allows it to
be overridden on a per-|SSL| basis.
Change-Id: Id435934af3d425d5f008d2f3b9751d1d0884ee55
Reviewed-on: https://boringssl-review.googlesource.com/12182
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>
The former has always worked. The latter is new to the revised
processing order.
Change-Id: I993d29ccaca091725524847695df4d1944b609cf
Reviewed-on: https://boringssl-review.googlesource.com/11848
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This changes our resumption strategy. Before, we would negotiate ciphers
only on fresh handshakes. On resumption, we would blindly use whatever
was in the session.
Instead, evaluate cipher suite preferences on every handshake.
Resumption requires that the saved cipher suite match the one that would
have been negotiated anyway. If client or server preferences changed
sufficiently, we decline the session.
This is much easier to reason about (we always pick the best cipher
suite), simpler, and avoids getting stuck under old preferences if
tickets are continuously renewed. Notably, although TLS 1.2 ticket
renewal does not work in practice, TLS 1.3 will renew tickets like
there's no tomorrow.
It also means we don't need dedicated code to avoid resuming a cipher
which has since been disabled. (That dedicated code was a little odd
anyway since the mask_k, etc., checks didn't occur. When cert_cb was
skipped on resumption, one could resume without ever configuring a
certificate! So we couldn't know whether to mask off RSA or ECDSA cipher
suites.)
Add tests which assert on this new arrangement.
BUG=116
Change-Id: Id40d851ccd87e06c46c6ec272527fd8ece8abfc6
Reviewed-on: https://boringssl-review.googlesource.com/11847
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 in preparation for determining the cipher suite (which, in TLS
1.2, requires the certificate be known) before resumption.
Note this has caller-visible effects:
- cert_cb is now called whether resumption occurs or not. Our only
consumer which uses this as a server is Node which will require a
patch to fix up their mucking about with SSL_get_session. (But the
patch should be quite upstreamable. More 1.1.0-compatible and
generally saner.)
- cert_cb is now called before new_session_cb and dos_protection_cb.
BUG=116
Change-Id: I6cc745757f63281fad714d4548f23880570204b0
Reviewed-on: https://boringssl-review.googlesource.com/11846
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 simplifies a little code around EMS and PSK KE modes, but requires
tweaking the SNI code.
The extensions that are more tightly integrated with the handshake are
still processed inline for now. It does, however, require an extra state
in 1.2 so the asynchronous session callback does not cause extensions to
be processed twice. Tweak a test enforce this.
This and a follow-up to move cert_cb before resumption are done in
preparation for resolving the cipher suite before resumption and only
resuming on match.
Note this has caller-visible effects:
- The legacy SNI callback happens before resumption.
- The ALPN callback happens before resumption.
- Custom extension ClientHello parsing callbacks also cannot depend on
resumption state.
- The DoS protection callback now runs after all the extension callbacks
as it is documented to be called after the resumption decision.
BUG=116
Change-Id: I1281a3b61789b95c370314aaed4f04c1babbc65f
Reviewed-on: https://boringssl-review.googlesource.com/11845
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>
As a client, we must tolerate this to avoid interoperability failures
with allowed server behaviors.
BUG=117
Change-Id: I9c40a2a048282e2e63ab5ee1d40773fc2eda110a
Reviewed-on: https://boringssl-review.googlesource.com/12311
Reviewed-by: David Benjamin <davidben@google.com>
Draft 18 sadly loosens the requirements to only requiring the PRF hash
stay fixed.
BUG=117
Change-Id: Ic94d53fd9cabaee611fcf36b0071558075e10728
Reviewed-on: https://boringssl-review.googlesource.com/12310
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>
This is generally much cleaner and makes it possible to implement the
more lax cipher matching in draft 18.
BUG=117
Change-Id: I595d7619d60bc92e598d75b43945286323c0b72b
Reviewed-on: https://boringssl-review.googlesource.com/12309
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>
This is a no-op because all affected codepaths are either unreachable or
are fine because ssl_hs_error (intentionally, since C doesn't help us
any) aligns with zero. Still, fix these.
Change-Id: Ieba4e3eec3881a56b5ddcd32abdd2c9dda875eda
Reviewed-on: https://boringssl-review.googlesource.com/12313
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>
It doesn't particular matter, but AcceptAnySession should only skip the
things that would cause us to note accept a ticket. ExpectTicketAge is
an assertion, not part of protocol logic. Accordingly, fix the text.
Change-Id: I3bea9c58f4d5f912308252ec8834f183287d632f
Reviewed-on: https://boringssl-review.googlesource.com/12308
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>
The version check should run if AcceptAnyVersion is *not* set.
Change-Id: I4c137564f91a86cb5e6a26e09fd4670cce8f1dcb
Reviewed-on: https://boringssl-review.googlesource.com/12307
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>
When debugging a flaky test, it's useful to be able to run a given test
over and over.
Change-Id: I1a7b38792215550b242eb8238214d873d41becb6
Reviewed-on: https://boringssl-review.googlesource.com/12301
Reviewed-by: David Benjamin <davidben@google.com>
The draft 18 implementation did not compute scts_requested correctly. As
a result, it always believed SCTs were requested. Fix this and add tests
for unsolicited OCSP responses and SCTs at all versions.
Thanks to Daniel Hirche for the report.
Change-Id: Ifc59c5c4d7edba5703fa485c6c7a4055b15954b4
Reviewed-on: https://boringssl-review.googlesource.com/12305
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>
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>
BUG=chromium:659593
Change-Id: I73a4751609b85df7cd40f0f60dc3f3046a490940
Reviewed-on: https://boringssl-review.googlesource.com/11861
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 currently preferentially sign the largest hash available and
advertise such a preference for signatures we accept. We're just as
happy with SHA-256 and, all else equal, a smaller hash would be epsilon
more performant. We also currently claim, in TLS 1.3, we prefer P-384
over P-256 which is off.
Instead order SHA-256 first, next the larger SHA-2 hashes, and leave
SHA-1 at the bottom. Within a hash, order ECDSA > RSA-PSS > RSA-PKCS1.
This has the added consequence that we will preferentially pair P-256
with SHA-256 in signatures we generate instead of larger hashes that get
truncated anyway.
Change-Id: If4aee068ba6829e8c0ef7948f56e67a5213e4c50
Reviewed-on: https://boringssl-review.googlesource.com/11821
Reviewed-by: Adam Langley <agl@google.com>
SSL_write has messy semantics around retries. As a sanity-check, it does
pointer and length checks and requires the original and retry SSL_write
pass the same buffer pointer.
In some cases, buffer addresses may change but still include the
original data as a prefix on the retry. Callers then set
SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER to skip the pointer check. But, in
that case, the pointer may have been freed so doing a comparison is
undefined behavior.
Short-circuiting the pointer equality check avoids this problem.
Change-Id: I76cb8f7d45533504cd95287bc53897ca636af51d
Reviewed-on: https://boringssl-review.googlesource.com/11760
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>
On the client we'll leave it off by default until the change has made it
through Chrome's release process. For TLS 1.3, there is no existing
breakage risk, so always do it. This saves us the trouble of having to
manually turn it on in servers.
See [0] for a data point of someone getting it wrong.
[0] https://hg.mozilla.org/projects/nss/rev/9dbc21b1c3cc
Change-Id: I74daad9e7efd2040e9d66d72d558b31f145e6c4c
Reviewed-on: https://boringssl-review.googlesource.com/11680
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I18cee423675d6a686f83b4ef4b38696cb618392c
Reviewed-on: https://boringssl-review.googlesource.com/11683
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I0bd7fdd276e7461ef08b8055bf3d0387f756739f
Reviewed-on: https://boringssl-review.googlesource.com/11682
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=103
Change-Id: I9a49fbaf66af73978ce264d27926f483e1e44766
Reviewed-on: https://boringssl-review.googlesource.com/11620
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>
Channel ID for TLS 1.3 uses the same digest construction as
CertificateVerify. This message is signed with the Channel ID key and
put in the same handshake message (with the same format) as in TLS 1.2.
BUG=103
Change-Id: Ia5b2dffe5a39c39db0cecb0aa6bdc328e53accc2
Reviewed-on: https://boringssl-review.googlesource.com/11420
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>
{sha1, ecdsa} is virtually nonexistent. {sha512, ecdsa} is pointless
when we only accept P-256 and P-384. See Chromium Intent thread here:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/kWwLfeIQIBM/9chGZ40TCQAJ
This tweaks the signature algorithm logic slightly so that sign and
verify preferences are separate.
BUG=chromium:655318
Change-Id: I1097332600dcaa38e62e4dffa0194fb734c6df3f
Reviewed-on: https://boringssl-review.googlesource.com/11621
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>