This is in preparation for simplifying tls1_check_group_id, called by
tls1_check_ec_cert, which, in turn, is in preparation for moving the
peer group list to SSL_HANDSHAKE.
It also helps with bug #55. Move the key usage check to the certificate
configuration sanity check. There's no sense in doing it late. Also
remove the ECDSA peer curve check as we configure certificates
externally. With only one certificate, there's no sense in trying to
remove it.
BUG=55
Change-Id: I8c116337770d96cc9cfd4b4f0ca7939a4f05a1a9
Reviewed-on: https://boringssl-review.googlesource.com/11524
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 releases memory associated with them after the handshake. Note this
changes the behavior of |SSL_get0_certificate_types| and
|SSL_get_client_CA_list| slightly. Both functions now return NULL
outside of the handshake. But they were already documented to return
something undefined when not called at the CertificateRequest.
A survey of callers finds none that would care. (Note
SSL_get_client_CA_list is used both as a getter for the corresponding
server config setter and to report client handshake properties. Only the
latter is affected.) It's also pretty difficult to imagine why a caller
would wish to query this stuff at any other time, and there are clear
benefits to dropping the CA list after the handshake (some servers send
ABSURDLY large lists).
Change-Id: I3ac3b601ff0cfa601881ce77ae33d99bb5327004
Reviewed-on: https://boringssl-review.googlesource.com/11521
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ifcdbeab9291d1141605a09a1960702c792cffa86
Reviewed-on: https://boringssl-review.googlesource.com/11561
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: I5d4fc0d3204744e93d71a36923469035c19a5b10
Reviewed-on: https://boringssl-review.googlesource.com/11560
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: Ib499b3393962a4d41cf9694e055ed3eb869d91a2
Reviewed-on: https://boringssl-review.googlesource.com/11504
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 server acknowledging a non-existent session is a particularly
interesting case since getting it wrong means a NULL crash.
Change-Id: Iabde4955de883595239cfd8e9d84a7711e60a886
Reviewed-on: https://boringssl-review.googlesource.com/11500
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>
BUG=77
Change-Id: If568412655aae240b072c29d763a5b17bb5ca3f7
Reviewed-on: https://boringssl-review.googlesource.com/10840
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
BUG=77
Change-Id: Id8c45e98c4c22cdd437cbba1e9375239e123b261
Reviewed-on: https://boringssl-review.googlesource.com/10763
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>
EnableAllCiphers is problematic since some (version, cipher)
combinations aren't even defined and crash. Instead, use the
SendCipherSuite bug to mask the true cipher (which is becomes arbitrary)
for failure tests. The shim should fail long before we get further.
This lets us remove a number of weird checks in the TLS 1.3 code.
This also fixes the UnknownCipher tests which weren't actually testing
anything. EnableAllCiphers is now AdvertiseAllConfiguredCiphers and
does not filter out garbage values.
Change-Id: I7102fa893146bb0d096739e768c5a7aa339e51a8
Reviewed-on: https://boringssl-review.googlesource.com/11481
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 is another case where the specification failed to hammer things
down and OpenSSL messed it up as a result. Also fix the SCT test in TLS
1.3.
Change-Id: I47541670447d1929869e1a39b2d9671a127bfba0
Reviewed-on: https://boringssl-review.googlesource.com/11480
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>
The client/server split didn't actually make sense. We're interested in
whether the client will notice the bad version before anything else, so
ignore peer cipher preferences so all combinations work.
Change-Id: I52f84b932509136a9b39d93e46c46729c3864bfd
Reviewed-on: https://boringssl-review.googlesource.com/11413
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>
cURL calls this function if |OPENSSL_VERSION_NUMBER| is in [0x10002003,
0x10002fff], which it now is for BoringSSL after 0aecbcf6.
Change-Id: I3f224f73f46791bd2232a1a96ed926c32740a6f6
Reviewed-on: https://boringssl-review.googlesource.com/11461
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
ConflictingVersionNegotiation really should be about, say 1.1 and 1.2
since those may be negotiated via either mechanism. (Those two cases are
actually kinda weird and we may wish to change the spec. But, in the
meantime, test that we have the expected semantics.)
Also test that we ignore true TLS 1.3's number for now, until we use it,
and that TLS 1.3 suitably ignores ClientHello.version.
Change-Id: I76c660ddd179313fa68b15a6fda7a698bef4d9c9
Reviewed-on: https://boringssl-review.googlesource.com/11407
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
They weren't updated for the new version negotiation. (Though right now
they're just testing that we *don't* implement the downgrade detection
because it's a draft version.)
Change-Id: I4c983ebcdf3180d682833caf1e0063467ea41544
Reviewed-on: https://boringssl-review.googlesource.com/11406
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Otherwise we panic. Thanks to EKR for reporting.
Change-Id: Ie4b6c2e18e1c77c7b660ca5d4c3bafb38a82cb6a
Reviewed-on: https://boringssl-review.googlesource.com/11405
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Rather than clear variables and break out of a loop that just ends up
returning anyway, just return. This makes all the abort points
consistent in this function.
Change-Id: I51d862e7c60a9e967773f15a17480b783af8c456
Reviewed-on: https://boringssl-review.googlesource.com/11422
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>
Breaking from inside the inner loop doesn't do what the code wants.
Instead the outer loop will continue running and it's possible for it to
read off the end of the buffer. (Found with libFuzzer.)
Next change will update the other abort points in this code to match.
Change-Id: I006dca0cd4c31db1c4b5e84b996fe24b2f1e6c13
Reviewed-on: https://boringssl-review.googlesource.com/11421
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I always forget this.
Change-Id: I9fa15cebb6586985ddc48cdbf9d184a49a8bfb02
Reviewed-on: https://boringssl-review.googlesource.com/11402
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This function is used by NGINX to enable specific curves for ECDH from a
configuration file. However when building with BoringSSL, since it's not
implmeneted, it falls back to using EC_KEY_new_by_curve_name() wich doesn't
support X25519.
Change-Id: I533df4ef302592c1a9f9fc8880bd85f796ce0ef3
Reviewed-on: https://boringssl-review.googlesource.com/11382
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>
OpenSSL recently had a regression here (CVE-2016-6309). We're fine,
but so that we stay that way, add some tests.
Change-Id: I244d7ff327b7aad550f86408c5e5e65e6d1babe5
Reviewed-on: https://boringssl-review.googlesource.com/11321
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>
BUG=106
Change-Id: Iaa12aeb67627f3c22fe4a917c89c646cb3dc1843
Reviewed-on: https://boringssl-review.googlesource.com/11325
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: I73f9fd64b46f26978b897409d817b34ec9d93afd
Reviewed-on: https://boringssl-review.googlesource.com/11080
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We were never really testing this.
Change-Id: Ia953870053d16d3994ae48172017d384c7bc3601
Reviewed-on: https://boringssl-review.googlesource.com/11341
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This mirror's 2dc0204603 on the C side.
BUG=90
Change-Id: Iebb72df5a5ae98cb2fd8db519d973cd734ff05ea
Reviewed-on: https://boringssl-review.googlesource.com/11320
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for implementing the version extension and is
probably what we should have done from the beginning as it makes
intolerance bugs simpler.
This means knobs like SendClientVersion and SendServerVersion deal with
the wire values while knobs like NegotiateVersion and MaxVersion deal
with logical versions. (This matches how the bugs have always worked.
SendFoo is just a weird post-processing bit on the handshake messages
while NegotiateVersion actually changes how BoGo behaves.)
BUG=90
Change-Id: I7f359d798d0899fa2742107fb3d854be19e731a4
Reviewed-on: https://boringssl-review.googlesource.com/11300
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>
It didn't clean up |profiles| on error or check for
sk_SRTP_PROTECTION_PROFILE_push failures.
Change-Id: I44d7f64896ad73347fbb0fc79752be4de70d3ab7
Reviewed-on: https://boringssl-review.googlesource.com/11323
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Also tidy up the logic slightly.
Change-Id: I708254406b2df52435ec434ac9806e8eb2cbe928
Reviewed-on: https://boringssl-review.googlesource.com/11322
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
If someone is still using EVP_PKEY_EC (I really should get on converting
Chromium...), don't silently skip the curve match check in TLS 1.3,
otherwise it may work on accident. Refuse to sign anything so this gets
caught.
Change-Id: I4ea46efb0b8f31a656771b9d2e5f882bba64eb99
Reviewed-on: https://boringssl-review.googlesource.com/11244
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 GREASEs cipher suites, groups, and extensions. For now, we'll
always place them in a hard-coded position. We can experiment with more
interesting strategies later.
If we add new ciphers and curves, presumably we prefer them over current
ones, so place GREASE values at the front. This prevents implementations
from parsing only the first value and ignoring the rest.
Add two new extensions, one empty and one non-empty. Place the empty one
in front (IBM WebSphere can't handle trailing empty extensions) and the
non-empty one at the end.
Change-Id: If2e009936bc298cedf2a7a593ce7d5d5ddbb841a
Reviewed-on: https://boringssl-review.googlesource.com/11241
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>
Get us a little bit more room here.
BUG=79
Change-Id: Ifadad94ead7794755a33f02d340111694b3572af
Reviewed-on: https://boringssl-review.googlesource.com/11228
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
That is an extremely confusing name. It should be NPN-Declined-TLS13.
Change-Id: I0e5fa50a3ddb0b80e88a8bc10d0ef87d0fff0a54
Reviewed-on: https://boringssl-review.googlesource.com/11227
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We recently added a three-connection option, but the transcripts were
still assuming just -Normal and -Resume.
Change-Id: I8816bce95dd7fac779af658e3eb86bc78bb95c91
Reviewed-on: https://boringssl-review.googlesource.com/11226
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This allows the fuzzer to discover server-side resumption paths by
simply supplying what we'd like the ticket to decrypt to in the clear.
We also have a natural way to get transcripts out of runner. We record
the runner-side transcripts, so all resumption handshakes will replay
the shim-created unencrypted tickets.
BUG=104
Change-Id: Icf9cbf4af520077d38e2c8c2766b6f8bfa3c9ab5
Reviewed-on: https://boringssl-review.googlesource.com/11224
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Both the C and Go code were sampling the real clock. With this, two
successive iterations of runner transcripts give the same output.
Change-Id: I4d9e219e863881bf518c5ac199dce938a49cdfaa
Reviewed-on: https://boringssl-review.googlesource.com/11222
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We want to ensure -fuzzer passes tests, except for the tests it
intentionally fails on. This ensures that we don't lose our ability to
refresh the fuzzer transcripts.
Change-Id: I761856c30379a3934fd46a24627ef8415b136f93
Reviewed-on: https://boringssl-review.googlesource.com/11221
Reviewed-by: Adam Langley <agl@google.com>
Apparently we never wrote one of those. Also send a decrypt_error alert
to be consistent with all the other signature checks.
Change-Id: Ib5624d098d1e3086245192cdce92f5df26005064
Reviewed-on: https://boringssl-review.googlesource.com/11180
Reviewed-by: David Benjamin <davidben@google.com>
SSL_peek works fine for us, but OpenSSL 1.1.0 regressed this
(https://github.com/openssl/openssl/issues/1563), and we don't have
tests either. Fix this.
SSL_peek can handle all weird events that SSL_read can, so use runner
and tell bssl_shim to do a SSL_peek + SSL_peek + SSL_read instead of
SSL_read. Then add tests for all the events we may discover.
Change-Id: I9e8635e3ca19653a02a883f220ab1332d4412f98
Reviewed-on: https://boringssl-review.googlesource.com/11090
Reviewed-by: Adam Langley <agl@google.com>
Upstream makes 0 mean "min/max supported version". Match that behavior,
although call it "default" instead. It shouldn't get you TLS 1.3 until
we're ready to turn it on everywhere.
BUG=90
Change-Id: I9f122fceb701b7d4de2ff70afbc1ffdf370cb97e
Reviewed-on: https://boringssl-review.googlesource.com/11181
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>
We'd previously been assuming we'd want to predict P-256 and X25519 but,
on reflection, that's nonsense. Although, today, P-256 is widespread and
X25519 is less so, that's not the right question to ask. Those servers
are all 1.2.
The right question is whether we believe enough servers will get to TLS
1.3 before X25519 to justify wasting 64 bytes on all other connections.
Given that OpenSSL has already shipped X25519 and Microsoft was doing
interop testing on X25519 around when we were shipping it, I think the
answer is no.
Moreover, if we are wrong, it will be easier to go from predicting one
group to two rather than the inverse (provided we send a fake one with
GREASE). I anticipate prediction-miss HelloRetryRequest logic across the
TLS/TCP ecosystem will be largely untested (no one wants to pay an RTT),
so taking a group out of the predicted set will likely be a risky
operation.
Only predicting one group also makes things a bit simpler. I haven't
done this here, but we'll be able to fold the 1.2 and 1.3 ecdh_ctx's
together, even.
Change-Id: Ie7e42d3105aca48eb9d97e2e05a16c5379aa66a3
Reviewed-on: https://boringssl-review.googlesource.com/10960
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 old numbers violate a MUST-level requirement in TLS 1.2 to not
advertise anonymous (0x0700 ends in 0x00). The spec has been updated
with new allocations which avoid these.
BUG=webrtc:6342
Change-Id: Ia5663ada98fa1ebf0f8a7f50fe74a0e9206c4194
Reviewed-on: https://boringssl-review.googlesource.com/11131
Reviewed-by: Adam Langley <agl@google.com>
Found by libFuzzer and then one more mistake caught by valgrind. Add a
test for this case.
Change-Id: I92773bc1231bafe5fc069e8568d93ac0df4c8acb
Reviewed-on: https://boringssl-review.googlesource.com/11129
Reviewed-by: David Benjamin <davidben@google.com>
Upstream added these functions after we did but decided to change the
names slightly. I'm not sure why they wanted to add the "proto" in
there, but align with them nonetheless so the ecosystem only has one set
of these functions.
BUG=90
Change-Id: Ia9863c58c9734374092051f02952b112806040cc
Reviewed-on: https://boringssl-review.googlesource.com/11123
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 in preparation for using the supported_versions extension to
experiment with draft TLS 1.3 versions, since we don't wish to restore
the fallback. With versions begin opaque values, we will want
version_from_wire to reject unknown values, not attempt to preserve
order in some way.
This means ClientHello.version processing needs to be separate code.
That's just written out fully in negotiate_version now. It also means
SSL_set_{min,max}_version will notice invalid inputs which aligns us
better with upstream's versions of those APIs.
This CL doesn't replace ssl->version with an internal-representation
version, though follow work should do it once a couple of changes land
in consumers.
BUG=90
Change-Id: Id2f5e1fa72847c823ee7f082e9e69f55e51ce9da
Reviewed-on: https://boringssl-review.googlesource.com/11122
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 will make it a little easier to store the normalized version rather
than the wire version. Also document the V2ClientHello behavior.
Change-Id: I5ce9ccce44ca48be2e60ddf293c0fab6bba1356e
Reviewed-on: https://boringssl-review.googlesource.com/11121
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>
Passing --quiet makes valgrind only print out errors, so we don't need
to suppress things. Combine that with checking valgrind's dedicated exit
code so we notice errors that happen before the "---DONE---" marker.
This makes that marker unnecessary for valgrind. all_tests.go was not
sensitive to this, but still would do well to have valgrind be silent.
Change-Id: I841edf7de87081137e38990e647e989fd7567295
Reviewed-on: https://boringssl-review.googlesource.com/11128
Reviewed-by: Adam Langley <agl@google.com>
If the test failed due to non-ASan reasons but ASan also had errors,
output those too.
Change-Id: Id908fe2a823c59255c6a9585dfaa894a4fcd9f59
Reviewed-on: https://boringssl-review.googlesource.com/11127
Reviewed-by: Adam Langley <agl@google.com>
Runner needs to implement fuzzer mode as well so we can record
transcripts from it. A bunch of tests were failing:
- C and Go disagreed on what fuzzer mode did to TLS 1.3 padding. So we
fuzz more code, align Go with C. Fuzzer mode TLS 1.3 still pads but
just skips the final AEAD.
- The deterministic RNG should be applied per test, not per exchange. It
turns out, if your RNG is deterministic, one tends to pick the same
session ID over and over which confuses clients. (Resumption is
signaled by echoing the session ID.)
Now the only failing tests are the ones one would expect to fail.
BUG=79
Change-Id: Ica23881a6e726adae71e6767730519214ebcd62a
Reviewed-on: https://boringssl-review.googlesource.com/11126
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
