Bruce Dawson pointed out that the shadowing of |ret| in |s3_srvr.c|
looked dodgy. It was actually deliberate (we don't want to reset the
default value of the function's |ret| variable with a successful return
from the callback) but it does look dodgy.
This change adds -Wshadow to ban variable shadowing and fixes all
current instances.
Change-Id: I1268f88b9f26245c7d16d6ead5bb9014ea471c01
Reviewed-on: https://boringssl-review.googlesource.com/2520
Reviewed-by: Adam Langley <agl@google.com>
It just inserts extra flushes everywhere and isn't used.
Change-Id: I082e4bada405611f4986ba852dd5575265854036
Reviewed-on: https://boringssl-review.googlesource.com/2456
Reviewed-by: Adam Langley <agl@google.com>
Use it in ssl3_cert_verify_hash so signing a pre-TLS-1.2 handshake hash can go
through RSA_sign and be intercepted via RSA_METHOD appropriately. This avoids
Windows needing to intercept sign_raw. (CAPI keys cannot provide sign_raw,
unless the input size happens to be that of NID_md5_sha1.)
Also use it in processing ServerKeyExchange to avoid special-casing RSA.
BUG=crbug.com/437023
Change-Id: Ia07433f468b75fdf7bfc8fa90c9751639b2478e6
Reviewed-on: https://boringssl-review.googlesource.com/2420
Reviewed-by: David Benjamin <davidben@google.com>
It should be set correctly prior to entering the handshake. Don't mask bugs by
assigning it.
Change-Id: Ib9bca8fad68916b3b242aad8819e3760e59e777a
Reviewed-on: https://boringssl-review.googlesource.com/2443
Reviewed-by: Adam Langley <agl@google.com>
first_packet is a temporary connection-global flag set for the duration of some
call and then queried from other code. This kind of logic is too difficult to
reason through. It also incorrectly treats renegotiate ClientHellos as
pre-version-negotiation records. This eliminates the need to query
enc_write_ctx (which wasn't EVP_AEAD-aware anyway).
Instead, take a leaf from Go TLS's book and add a have_version bit. This is
placed on s->s3 as it is connection state; s->s3 automatically gets reset on
SSL_clear while s doesn't.
This new flag will also be used to determine whether to do the V2ClientHello
sniff when the version-locked methods merge into SSLv23_method. It will also
replace needing to condition s->method against a dummy DTLS_ANY_VERSION value
to determine whether DTLS version negotiation has happened yet.
Change-Id: I5c8bc6258b182ba4ab175a48a84eab6d3a001333
Reviewed-on: https://boringssl-review.googlesource.com/2442
Reviewed-by: Adam Langley <agl@google.com>
Supporting both schemes seems pointless. Now that s->server and s->state are
set appropriately late and get_ssl_method is gone, the only difference is that
the client/server ones have non-functional ssl_accept or ssl_connect hooks. We
can't lose the generic ones, so let's unify on that.
Note: this means a static linker will no longer drop the client or server
handshake code if unused by a consumer linking statically. However, Chromium
needs the server half anyway for DTLS and WebRTC, so that's probably a lost
cause. Android also exposes server APIs.
Change-Id: I290f5fb4ed558f59fadb5d1f84e9d9c405004c23
Reviewed-on: https://boringssl-review.googlesource.com/2440
Reviewed-by: Adam Langley <agl@google.com>
If the state is SSL_ST_BEFORE, the SSL* was just initialized. Otherwise, we
don't want to call SSL_clear. The one case I found where we do is if a
handshake message is received and someone sets
SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS. This is apparently intended for external
consumers to set, but I see no code in Google that does.
Which is fortunate because it'll trigger SSL_clear. This retains the BIOs but
drops all connection state, including the record. If the client just initiated
renego, that's the ClientHello that's lost. The connection then hangs: the now
reset SSL* wants a ClientHello (under the null cipher because that too's been
dropped) while the peer wants an encrypted ServerHello.
Change-Id: Iddb3e0bb86d39d98155b060f9273a0856f2d1409
Reviewed-on: https://boringssl-review.googlesource.com/2436
Reviewed-by: Adam Langley <agl@google.com>
SSL_ST_BEFORE is never standalone. As of upstream's
413c4f45ed0508d2242638696b7665f499d68265, SSL_ST_BEFORE is only ever set paired
with SSL_ST_ACCEPT or SSL_ST_CONNECT.
Conversely, SSL_ST_OK is never paired with SSL_ST_ACCEPT or SSL_ST_CONNECT. As
far as I can tell, this combination has never been possible.
Change-Id: Ifbc8f147be821026cf59f3d5038f0dbad3b0a1d2
Reviewed-on: https://boringssl-review.googlesource.com/2433
Reviewed-by: Adam Langley <agl@google.com>
It should already be assigned, as of upstream's
b31b04d951e9b65bde29657e1ae057b76f0f0a73. I believe these assignments are part
of the reason it used to appear to work. Replace them with assertions. So the
assertions are actually valid, check in SSL_connect / SSL_accept that they are
never called if the socket had been placed in the opposite state. (Or we'd be
in another place where it would have appeared to work with the handshake
functions fixing things afterwards.)
Now the only places handshake_func is set are in SSL_set_{connect,accept}_state
and the method switches.
Change-Id: Ib249212bf4aa889b94c35965a62ca06bdbcf52e1
Reviewed-on: https://boringssl-review.googlesource.com/2432
Reviewed-by: Adam Langley <agl@google.com>
This fixes bugs that kept the tests from working:
- Resolve DTLS version and cookie before the session.
- In DTLS_ANY_VERSION, ServerHello should be read with first_packet = 1. This
is a regression from f2fedefdca. We'll want to
do the same for TLS, but first let's change this to a boolean has_version in a
follow-up.
Things not yet fixed:
- DTLS code is not EVP_AEAD-aware. Those ciphers are disabled for now.
- On the client, DTLS_ANY_VERSION creates SSL_SESSIONs with the wrong
ssl_version. The tests pass because we no longer enforce the match as of
e37216f56009fbf48c3a1e733b7a546ca6dfc2af. (In fact, we've gone from the server
ignoring ssl_version and client enforcing to the client mostly ignoring
ssl_version and the server enforcing.)
- ssl3_send_client_hello's ssl_version check checks for equality against
s->version rather than >.
Change-Id: I5a0dde221b2009413df9b9443882b9bf3b29519c
Reviewed-on: https://boringssl-review.googlesource.com/2403
Reviewed-by: Adam Langley <agl@google.com>
This is a bit of cleanup that probably should have been done at the same time
as 30ddb434bf.
For now, version negotiation is implemented with a method swap. It also
performs this swap on SSL_set_session, but this was neutered in
30ddb434bf. Rather than hackishly neuter it,
remove it outright. In addition, remove SSL_set_ssl_method. Now all method
swaps are internal: SSLv23_method switch to a version-specific method and
SSL_clear undoing it.
Note that this does change behavior: if an SSL* is created with one
version-specific method and we SSL_set_session to a session from a /different/
version, we would switch to the /other/ version-specific method. This is
extremely confusing, so it's unlikely anyone was actually expecting it.
Version-specific methods in general don't work well.
Change-Id: I72a5c1f321ca9aeb1b52ebe0317072950ba25092
Reviewed-on: https://boringssl-review.googlesource.com/2390
Reviewed-by: Adam Langley <agl@google.com>
This is only used for EAP-FAST which we apparently don't need to support.
Remove it outright. We broke it in 9eaeef81fa by
failing to account for session misses.
If this changes and we need it later, we can resurrect it. Preferably
implemented differently: the current implementation is bolted badly onto the
handshake. Ideally use the supplied callbacks to fabricate an appropriate
SSL_SESSION and resume that with as much of the normal session ticket flow as
possible.
The one difference is that EAP-FAST seems to require the probing mechanism for
session tickets rather than the sane session ID echoing version. We can
reimplement that by asking the record layer to probe ahead for one byte.
Change-Id: I38304953cc36b2020611556a91e8ac091691edac
Reviewed-on: https://boringssl-review.googlesource.com/2360
Reviewed-by: Adam Langley <agl@google.com>
This commit fixes a number of crashes caused by malloc failures. They
were found using the -malloc-test=0 option to runner.go which runs tests
many times, causing a different allocation call to fail in each case.
(This test only works on Linux and only looks for crashes caused by
allocation failures, not memory leaks or other errors.)
This is not the complete set of crashes! More can be found by collecting
core dumps from running with -malloc-test=0.
Change-Id: Ia61d19f51e373bccb7bc604642c51e043a74bd83
Reviewed-on: https://boringssl-review.googlesource.com/2320
Reviewed-by: Adam Langley <agl@google.com>
tls1_process_sigalgs now only determines the intersection between the peer
algorithms and those configured locally. That list is queried later to
determine the hash algorithm to use when signing CertificateVerify or
ServerKeyExchange.
This is needed to support client auth on Windows where smartcards or CAPI may
not support all hash functions.
As a bonus, this does away with more connection-global state. This avoids the
current situation where digests are chosen before keys are known (for
CertificateVerify) or for slots that don't exist.
Change-Id: Iec3619a103d691291d8ebe08ef77d574f2faf0e8
Reviewed-on: https://boringssl-review.googlesource.com/2280
Reviewed-by: Adam Langley <agl@google.com>
All of NSS, upstream OpenSSL, SChannel, and Secure Transport require, on the
client, that the ServerHello version match the session's version on resumption.
OpenSSL's current behavior is incompatible with all of these. Fall back to a
full handshake on the server instead of mismatch.
Add a comment on the client for why we are, as of
30ddb434bf, not currently enforcing the same in
the client.
Change-Id: I60aec972d81368c4ec30e2fd515dabd69401d175
Reviewed-on: https://boringssl-review.googlesource.com/2244
Reviewed-by: Adam Langley <agl@google.com>
The limit increased from 32 to 255 between DTLS 1.0 and DTLS 1.2.
Change-Id: I329a59f9ba2bccc70282e2b47679c57b67e5ed43
Reviewed-on: https://boringssl-review.googlesource.com/2242
Reviewed-by: Adam Langley <agl@google.com>
There's not much point in retaining the identity hint in the SSL_SESSION. This
avoids the complexity around setting psk_identity hint on either the SSL or the
SSL_SESSION. Introduce a peer_psk_identity_hint for the client to store the one
received from the server.
This changes the semantics of SSL_get_psk_identity_hint; it now only returns
the value configured for the server. The client learns the hint through the
callback. This is compatible with the one use of this API in conscrypt (it
pulls the hint back out to pass to a callback).
Change-Id: I6d9131636b47f13ac5800b4451436a057021054a
Reviewed-on: https://boringssl-review.googlesource.com/2213
Reviewed-by: Adam Langley <agl@google.com>
(Imported form upstream's 455b65dfab0de51c9f67b3c909311770f2b3f801 and
0d6a11a91f4de238ce533c40bd9507fe5d95f288)
Change-Id: Ia195c7fe753cfa3a7f8c91d2d7b2cd40a547be43
s->s3 is never NULL if an ssl3_* function is called, and we'll crash later
anyway. (This also makes scan-build stop believing it can be NULL.)
Change-Id: Ibf8433bd4d945f9bf5416d72946102a9e50d2787
Reviewed-on: https://boringssl-review.googlesource.com/2206
Reviewed-by: Adam Langley <agl@google.com>
Minor change, but they're the users of the old API left within
BoringSSL.
Change-Id: Ic24e0d006c97fa5265abc3373d3f98aa8d2f8b1e
Reviewed-on: https://boringssl-review.googlesource.com/2100
Reviewed-by: Adam Langley <agl@google.com>
If generating the master secret or applying the PSK post-processing fails,
we'll double-free all the ECDH state.
Change-Id: Id52931af73bdef5eceb06f7e64d32fdda629521e
Reviewed-on: https://boringssl-review.googlesource.com/2063
Reviewed-by: Adam Langley <agl@google.com>
Deprecate the old two-pass version of the function. If the ticket is too long,
replace it with a placeholder value but keep the connection working.
Change-Id: Ib9fdea66389b171862143d79b5540ea90a9bd5fb
Reviewed-on: https://boringssl-review.googlesource.com/2011
Reviewed-by: Adam Langley <agl@google.com>
There's only one caller and it doesn't use that feature. While I'm here, tidy
that function a little. Don't bother passing FALLBACK_SCSV into
ssl3_get_cipher_by_value.
Change-Id: Ie71298aeaaab6e24401e0a6c2c0d2281caa93ba4
Reviewed-on: https://boringssl-review.googlesource.com/2030
Reviewed-by: Adam Langley <agl@google.com>
This removes the need to track the client cipher list in the SSL_SESSION. It
also eliminates a field in SSL_SESSION that wasn't serialized by
i2d_SSL_SESSION. It's only used to implement SSL_get_shared_ciphers which is
only used by debug code.
Moreover, it doesn't work anyway. The SSLv2 logic pruned that field to the
common ciphers, but the SSLv3+ logic just stores the client list as-is. I found
no internal callers that were actually compiled (if need be we can stub in
something that always returns the empty string or so).
Change-Id: I55ad45964fb4037fd623f7591bc574b2983c0698
Reviewed-on: https://boringssl-review.googlesource.com/1866
Reviewed-by: Adam Langley <agl@google.com>
This resolves a pile of MSVC warnings in Chromium.
Change-Id: Ib9a29cb88d8ed8ec4118d153260f775be059a803
Reviewed-on: https://boringssl-review.googlesource.com/1865
Reviewed-by: Adam Langley <agl@google.com>
Remove one more difference to worry about switching between TLS and SSLv3
method tables.
Although this does change the get_ssl_method hook for the version-specific
tables (before TLS and SSLv3 would be somewhat partitioned), it does not appear
to do anything. get_ssl_method is only ever called in SSL_set_session for
client session resumption. Either you're using the version-specific method
tables and don't know about other versions anyway or you're using SSLv23 and
don't partition TLS vs SSL3 anyway.
BUG=chromium:403378
Change-Id: I8cbdf02847653a01b04dbbcaf61fcb3fa4753a99
Reviewed-on: https://boringssl-review.googlesource.com/1842
Reviewed-by: Adam Langley <agl@google.com>
The TLS-specific hooks have been removed. We aim to no longer perform version
negotiation as a pre-processing step, so ensure the only differences to worry
about are the version, get_method hook, and the enc_data.
BUG=chromium:403378
Change-Id: I628ec6f4c50ceed01d7af8f4110b6dc95cfbe023
Reviewed-on: https://boringssl-review.googlesource.com/1841
Reviewed-by: Adam Langley <agl@google.com>
Remove the old implementation which was excessively general. This mirrors the
SCT support and adds a single boolean flag to request an OCSP response with no
responder IDs, extensions, or frills. The response, if received, is stored on
the SSL_SESSION so that it is available for (re)validation on session
resumption; Chromium revalidates the saved auth parameters on resume.
Server support is unimplemented for now. This API will also need to be adjusted
in the future if we implement RFC 6961.
Change-Id: I533c029b7f7ea622d814d05f934fdace2da85cb1
Reviewed-on: https://boringssl-review.googlesource.com/1671
Reviewed-by: Adam Langley <agl@google.com>
Get all this stuff out of the way.
- OPENSSL_NO_MD5
- OPENSSL_NO_SHA
- OPENSSL_NO_EC
- OPENSSL_NO_ECDSA
- OPENSSL_NO_ECDH
- OPENSSL_NO_NEXTPROTONEG
- OPENSSL_NO_DH
- OPENSSL_NO_SSL3
- OPENSSL_NO_RC4
- OPENSSL_NO_RSA
Also manually removed a couple instances of OPENSSL_NO_DSA that seemed to be
confused anyway. Did some minor manual cleanup. (Removed a few now-pointless
'if (0)'s.)
Change-Id: Id540ba97ee22ff2309ab20ceb24c7eabe766d4c4
Reviewed-on: https://boringssl-review.googlesource.com/1662
Reviewed-by: Adam Langley <agl@google.com>
This moves CertificateVerify digest processing to the new
SSL_GET_MESSAGE_DONT_HASH_MESSAGE flag. It also refactors it similarly to
ssl3_send_cert_verify and moves that logic to a common ssl3_cert_verify_hash
function to compute the handshake hash.
This removes a large chunk of duplicate (and divergent!) logic between TLS and
DTLS. It also removes TLS1_FLAGS_KEEP_HANDSHAKE.
Change-Id: Ia63c94f7d76d901bc9c4c33454fbfede411adf63
Reviewed-on: https://boringssl-review.googlesource.com/1633
Reviewed-by: Adam Langley <agl@google.com>
This avoids needing the save the hash on the SSL* (and use some field for two
purposes). Instead, use the new SSL_GET_MESSAGE_DONT_HASH_MESSAGE flag (which
actually was already used here, but at the time, pointlessly). Also fix a minor
bug where the hash would be recomputed in non-blocking mode because init_num
may stay zero for a few state machine iterations.
Change-Id: I3d8331cf3134c5f9a3eda9e988bba5bcebe40933
Reviewed-on: https://boringssl-review.googlesource.com/1631
Reviewed-by: Adam Langley <agl@google.com>
This replaces the special-case in ssl3_get_message for Channel ID. Also add
ssl3_hash_current_message to hash the current message, taking TLS vs DTLS
handshake header size into account.
One subtlety with this flag is that a message intended to be processed with
SSL_GET_MESSAGE_DONT_HASH_MESSAGE cannot follow an optional message
(reprocessed with reuse_message, etc.). There is an assertion to that effect.
If need be, we can loosen it to requiring that the preceeding optional message
also pass SSL_GET_MESSAGE_DONT_HASH_MESSAGE and then maintain some state to
perform the more accurate assertion, but this is sufficient for now.
Change-Id: If8c87342b291ac041a35885b9b5ee961aee86eab
Reviewed-on: https://boringssl-review.googlesource.com/1630
Reviewed-by: Adam Langley <agl@google.com>
Now that only RSA and ECDSA certificates are supported, the server should just
reject non-signing ones outright, rather than allowing them to skip
CertificateVerify.
Change-Id: I7fe5ed3adde14481016ee841ed241faba18c26f0
Reviewed-on: https://boringssl-review.googlesource.com/1609
Reviewed-by: Adam Langley <agl@google.com>
The return values are now 1/0, not 1/0/-1.
Change-Id: If65bb08a229c7944cb439ec779df461904d0ec19
Reviewed-on: https://boringssl-review.googlesource.com/1607
Reviewed-by: Adam Langley <agl@google.com>
It doesn't appear to have ever been implemented on the client. The server code
stopped working anyway because it now skips the ssl_get_message call, so we
never cash in on the reuse_message, attempt to reprocess the repeated
ClientHello, and reject it thinking it's a second MS SGC restart.
Change-Id: Id536846e08460143f6fc0a550bdcc1b26b506b04
Reviewed-on: https://boringssl-review.googlesource.com/1580
Reviewed-by: Adam Langley <agl@google.com>
DSA is not connected up to EVP, so it wouldn't work anyway. We shouldn't
advertise a cipher suite we don't support. Chrome UMA data says virtually no
handshakes end up negotiating one of these.
Change-Id: I874d934432da6318f05782ebd149432c1d1e5275
Reviewed-on: https://boringssl-review.googlesource.com/1566
Reviewed-by: Adam Langley <agl@google.com>
These are the variants where the CA signs a Diffie-Hellman keypair. They are
not supported by Chrome on NSS.
Change-Id: I569a7ac58454bd3ed1cd5292d1f98499012cdf01
Reviewed-on: https://boringssl-review.googlesource.com/1564
Reviewed-by: Adam Langley <agl@google.com>
In the fixed_ecdh case, it wasn't even implemented, but there was stub code for
it. It complicates the ClientKeyExchange (the client parameters become implicit
in the certificate) and isn't used.
Change-Id: I3627a37042539c90e05e59cd0cb3cd6c56225561
Reviewed-on: https://boringssl-review.googlesource.com/1563
Reviewed-by: Adam Langley <agl@google.com>
Just use the normal API for them.
Change-Id: Ibb5988611a86e8d39abda1e02087523d98defb51
Reviewed-on: https://boringssl-review.googlesource.com/1555
Reviewed-by: Adam Langley <agl@google.com>
I see no internal users and the existence of a THIRD version encoding
complicates all version-checking logic. Also convert another version check to
SSL_IS_DTLS that was missed earlier.
Change-Id: I60d215f57d44880f6e6877889307dc39dbf838f7
Reviewed-on: https://boringssl-review.googlesource.com/1550
Reviewed-by: Adam Langley <agl@google.com>
This lets us put the SSL_CIPHER table in the data section. For type-checking,
make STACK_OF(SSL_CIPHER) cast everything to const SSL_CIPHER*.
Note that this will require some changes in consumers which weren't using a
const SSL_CIPHER *.
Change-Id: Iff734ac0e36f9e5c4a0f3c8411c7f727b820469c
Reviewed-on: https://boringssl-review.googlesource.com/1541
Reviewed-by: Adam Langley <agl@google.com>
It was added in OpenSSL 1.0.2, so nothing can be depending on it yet. If we
really want a Suite B profile, it seems better to generate a configuration for
the rest of the system rather than pepper the codebase with checks.
Change-Id: I1be3ebed0e87cbfe236ade4174dcf5bbc7e10dd5
Reviewed-on: https://boringssl-review.googlesource.com/1517
Reviewed-by: Adam Langley <agl@google.com>
It's not part of SSL_OP_ALL and is unused, so remove it. Add a test that
asserts the version check works.
Change-Id: I917516594ec5a4998a8316782f035697c33d99b0
Reviewed-on: https://boringssl-review.googlesource.com/1418
Reviewed-by: Adam Langley <agl@google.com>
Any ssl3_get_* function that takes ownership of something before the
ssl_get_message call can't early-return without cleanup work.
This fixes valgrind on ClientAuth-Server-Async.
Change-Id: Ie7f0b37cac4d4bb7e06c00bae091fee0386c22da
Reviewed-on: https://boringssl-review.googlesource.com/1413
Reviewed-by: Adam Langley <agl@google.com>
- DTLS server code didn't account for the new ClientHello state. This looks
like it only matters if a DTLS server uses select_certificate_cb and returns
asynchronously.
- State A transitions immediately to B and is redundant. No code distinguishes
A and B.
- The ssl_get_message call transitions to the second state (originally C). This
makes the explicit transition to C a no-op. More of a problem,
ssl_get_message may return asynchronously and remain in its second state if the
handshake body had not completed yet. Fix this by splitting state C in two.
Combined with the above change, this results in only the top few states getting
reshuffled.
This fixes the server async tests.
Change-Id: I46703bcd205988b118217b6424ba4f88e731be5a
Reviewed-on: https://boringssl-review.googlesource.com/1412
Reviewed-by: Adam Langley <agl@google.com>
