This introduces another knob into SSL_AEAD_CTX to omit the version from the ad
parameter. It also allows us to fold a few more SSL3_ENC_METHOD hooks together.
Change-Id: I6540d410d4722f734093554fb434dab6e5217d4f
Reviewed-on: https://boringssl-review.googlesource.com/2698
Reviewed-by: Adam Langley <agl@google.com>
This lets us fold away the SSLv3-specific generate_master_secret. Once SSLv3
uses AEADs, others will fold away as well.
Change-Id: I27c1b75741823bc6db920d35f5dd5ce71b6fdbb3
Reviewed-on: https://boringssl-review.googlesource.com/2697
Reviewed-by: Adam Langley <agl@google.com>
Fix up the generate_master_secret parameter while we're here.
Change-Id: I1c80796d1f481be0c3eefcf3222f2d9fc1de4a51
Reviewed-on: https://boringssl-review.googlesource.com/2696
Reviewed-by: Adam Langley <agl@google.com>
size_t all the parameters. Also explicitly label label as label. This is in
preparation for pulling the PRF out into SSL3_ENC_METHOD so more of the
SSL3_ENC_METHOD hooks may be shared between SSLv3 and TLS once SSLv3 uses
stateful AEADs.
Also port away from EVP_PKEY_HMAC and use HMAC_CTX directly. The abstraction
doesn't buy much and is different from all the other EVP_DigestSign* functions.
There are few enough users within BoringSSL and Google that we can probably
deprecate and eventually remove it altogether.
Change-Id: I5d4529438c8a2a992fc199388a0c9e73bd6d2e06
Reviewed-on: https://boringssl-review.googlesource.com/2695
Reviewed-by: Adam Langley <agl@google.com>
The EVP_CIPHER codepath should no longer be used with TLS. It still exists for
DTLS and SSLv3. The AEAD construction in TLS does not allow for
variable-overhead AEADs, so stateful AEADs do not include the length in the ad
parameter. Rather the AEADs internally append the unpadded length once it is
known. EVP_aead_rc4_md5_tls is modified to account for this.
Tests are added (and RC4-MD5's regenerated) for each of the new AEADs. The
cipher tests are all moved into crypto/cipher/test because there's now a lot of
them and they clutter the directory listing.
In ssl/, the stateful AEAD logic is also modified to account for stateful AEADs
with a fixed IV component, and for AEADs which use a random nonce (for the
explicit-IV CBC mode ciphers).
The new implementation fixes a bug/quirk in stateless CBC mode ciphers where
the fixed IV portion of the keyblock was generated regardless. This is at the
end, so it's only relevant for EAP-TLS which generates a MSK from the end of
the key block.
Change-Id: I2d8b8aa11deb43bde2fd733f4f90b5d5b8cb1334
Reviewed-on: https://boringssl-review.googlesource.com/2692
Reviewed-by: Adam Langley <agl@google.com>
It's not worth saving the extra mallocs. This is preparation for moving SSLv3
to stateful AEADs; it'll share code TLS's SSL3_ENC_METHOD, but
tls1_generate_key_block is different, so that'll be pulled out into its own
hook.
Change-Id: I3f2136600758465c66ce23736041bb47f74efa6d
Reviewed-on: https://boringssl-review.googlesource.com/2690
Reviewed-by: Adam Langley <agl@google.com>
More modern versions of GCC (at least with aarch64) are warning about an
unused value in these locations. It's incorrect, but I guess that the
macro is confusing it.
Using a (void) tag is a little ugly but solves the problem.
Change-Id: If6ba5083ab6e501c81e7743ae1ed99a89565e57c
Reviewed-on: https://boringssl-review.googlesource.com/2810
Reviewed-by: Adam Langley <agl@google.com>
The minimum MTU (not consistently enforced) is just under 256, so it's
difficult to test everything, but this is a basic test. (E.g., without renego,
the only handshake message with encryption is Finished which fits in the MTU.)
It tests the server side because the Certificate message is large enough to
require fragmentation.
Change-Id: Ida11f1057cebae2b800ad13696f98bb3a7fbbc5e
Reviewed-on: https://boringssl-review.googlesource.com/2824
Reviewed-by: Adam Langley <agl@google.com>
This should have been removed with its dtls1_clear cousin in
8c88153465.
Change-Id: Ibf4ee67348f603285b26766568cbb92183b62cee
Reviewed-on: https://boringssl-review.googlesource.com/2823
Reviewed-by: Adam Langley <agl@google.com>
State on s3 gets freed in both ssl3_clear and ssl3_free. Considate to just
ssl3_free. This replaces the (SSL,ssl,ssl3)_clear calls in (SSL,ssl,ssl3)_new
with the state that was initialized. This results in a little code duplication
between SSL_new and SSL_clear because state is on the wrong object. I've just
left TODOs for now; some of it will need disentangling.
We're far from it, but going forward, separate state between s and s->s3 as:
- s contains configuration state, DTLS or TLS. It is initialized from SSL_CTX,
configurable directly afterwards, and preserved across SSL_clear calls.
(Including when it's implicitly set as part of a handshake callback.)
- Connection state hangs off s->s3 (TLS) and s->d1 (DTLS). It is reset across
SSL_clear. This should happen naturally out of a ssl_free/ssl_new pair.
The goal is to avoid needing separate initialize and reset code for anything;
the point any particular state is reset is the point its owning context is
destroyed and recreated.
Change-Id: I5d779010778109f8c339c07433a0777feaf94d1f
Reviewed-on: https://boringssl-review.googlesource.com/2822
Reviewed-by: Adam Langley <agl@google.com>
Configuration data inherited from the ctx happens in SSL_new. (This also gets
in the way of using ssl3_free/ssl3_new to implement SSL_clear.)
Change-Id: I2773af91abf4e1edc0c1a324bc1e94088d7c2274
Reviewed-on: https://boringssl-review.googlesource.com/2821
Reviewed-by: Adam Langley <agl@google.com>
A memory leak can occur in dtls1_buffer_record if either of the calls to
ssl3_setup_buffers or pqueue_insert fail. The former will fail if there
is a malloc failure, whilst the latter will fail if attempting to add a
duplicate record to the queue. This should never happen because
duplicate records should be detected and dropped before any attempt to
add them to the queue. Unfortunately records that arrive that are for
the next epoch are not being recorded correctly, and therefore replays
are not being detected. Additionally, these "should not happen" failures
that can occur in dtls1_buffer_record are not being treated as fatal and
therefore an attacker could exploit this by sending repeated replay
records for the next epoch, eventually causing a DoS through memory
exhaustion.
Thanks to Chris Mueller for reporting this issue and providing initial
analysis and a patch. Further analysis and the final patch was performed
by Matt Caswell from the OpenSSL development team.
CVE-2015-0206
(Imported from upstream's 7c6a3cf2375f5881ef3f3a58ac0fbd0b4663abd1).
Change-Id: I765fe61c75bc295bcc4ab356b8a5ce88c8964764
Reviewed-on: https://boringssl-review.googlesource.com/2782
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Add a dedicated error code to the queue for a handshake_failure alert in
response to ClientHello. This matches NSS's client behavior and gives a better
error on a (probable) failure to negotiate initial parameters.
BUG=https://crbug.com/446505
Change-Id: I34368712085a6cbf0031902daf2c00393783d96d
Reviewed-on: https://boringssl-review.googlesource.com/2751
Reviewed-by: Adam Langley <agl@google.com>
The |skip_message| variable was overly complex and, since we have at
least 32-bit ints, we know that a 24-bit value doesn't overflow an int.
Change-Id: I5c16fa979e1716f39cc47882c033bcf5bce3284c
Reviewed-on: https://boringssl-review.googlesource.com/2610
Reviewed-by: Adam Langley <agl@google.com>
RAND_pseudo_bytes just calls RAND_bytes now and only returns 0 or 1. Switch all
callers within the library call the new one and use the simpler failure check.
This fixes a few error checks that no longer work (< 0) and some missing ones.
Change-Id: Id51c79deec80075949f73fa1fbd7b76aac5570c6
Reviewed-on: https://boringssl-review.googlesource.com/2621
Reviewed-by: Adam Langley <agl@google.com>
No current use of ssl_cert_type passes a NULL EVP_PKEY, so it can be simplified
a little.
Change-Id: I2052cc3b6069cd30e4685ba8a6d0014016a4d712
Reviewed-on: https://boringssl-review.googlesource.com/2620
Reviewed-by: Adam Langley <agl@google.com>
Currently we don't express an opinion. Most sites aren't likely to have a
choice since it depends on what certificates they have available. But we may as
well order them.
Change-Id: I4fffa5e392f42e19823cb8faa2e9e15a6bb91086
Reviewed-on: https://boringssl-review.googlesource.com/2607
Reviewed-by: Adam Langley <agl@google.com>
Turns out the EVP_CIPH_FLAG_CUSTOM_CIPHER ciphers (i.e. legacy EVP_CIPHER
AES-GCM) have a completely different return value setup than the normal ones
which are the standard one/zero. (Except that they never return zero; see
TODO.)
Fix checks in ssl/ and remove remnants of EVP_CIPH_FLAG_CUSTOM_CIPHER in ssl/
as we're using EVP_AEAD now.
See CHANGES entry added in upstream's 3da0ca796cae6625bd26418afe0a1dc47bf5a77f.
Change-Id: Ia4d0ff59b03c35fab3a08141c60b9534cb7172e2
Reviewed-on: https://boringssl-review.googlesource.com/2606
Reviewed-by: Adam Langley <agl@google.com>
Those version checks are if renego tried to change the version, but at that
point we're out of the initial null cipher and should leave the version fixed.
(On the server end, the code in question was dead after the version negotiation
rewrite anyway.)
Change-Id: I3242ba11bc9981ccf7fdb867176d59846cc49dd9
Reviewed-on: https://boringssl-review.googlesource.com/2605
Reviewed-by: Adam Langley <agl@google.com>
This avoids needing a should_add_to_finished_hash boolean on do_write. The
logic in do_write was a little awkward because do_write would be called
multiple times if the write took several iterations. This also gets complex if
DTLS retransmits are involved. (At a glance, it's not obvious the
BIO_CTRL_DGRAM_MTU_EXCEEDED case actually works.)
Doing it as the handshake message is being prepared avoids this concern. It
also gives a natural point for the extended master secret logic which needs to
do work after the finished hash has been sampled.
As a bonus, we can remove s->d1->retransmitting which was only used to deal
with this issue.
Change-Id: Ifedf23ee4a6c5e08f960d296a6eb1f337a16dc7a
Reviewed-on: https://boringssl-review.googlesource.com/2604
Reviewed-by: Adam Langley <agl@google.com>
That comment is wrong as of TLS 1.2.
Change-Id: I900d5efc09d7468f2601d85f867833e43d046f6a
Reviewed-on: https://boringssl-review.googlesource.com/2603
Reviewed-by: Adam Langley <agl@google.com>
(Or should we just drop this? It only matters for servers trying to use client
auth.)
Change-Id: I50b6999375dc8f9246bf617f17929ae304503c57
Reviewed-on: https://boringssl-review.googlesource.com/2602
Reviewed-by: Adam Langley <agl@google.com>
Some of the messages did the computation manually which would bite us if we
tried to transplant them between DTLS and TLS. More importantly, it precludes
moving the handshake hash computation from ssl_do_write to
ssl_set_handshake_header.
Change-Id: I9d400deb0720e62cb1ab905242eb0679ad650a46
Reviewed-on: https://boringssl-review.googlesource.com/2600
Reviewed-by: Adam Langley <agl@google.com>
The frag_off/frag_len parameters are always zero, and the return value is never
used.
Change-Id: If7487b23c55f2a996e411b25b76a8e1651f25d8b
Reviewed-on: https://boringssl-review.googlesource.com/2601
Reviewed-by: Adam Langley <agl@google.com>
This change has no semantic effect (I hope!). It's just a reformatting
of a few files in ssl/. This is just a start – the other files in ssl/
should follow in the coming days.
Change-Id: I5eb3f4b18d0d46349d0f94d3fe5ab2003db5364e
David is heading out so I didn't want to block the previous batch of
changes for weeks. Thus I landed them as-is and this change tweaks a
couple of things that would normally have been addressed in code-review.
Change-Id: I2579dbc43d93fea34a52b4041f5511d70217aaf7
Ensure that both the client and the server emit a protocol_version alert
(except in SSLv3 where it doesn't exist) with a record-layer version which the
peer will recognize.
Change-Id: I31650a64fe9b027ff3d51e303711910a00b43d6f
This makes SSLv23_method go through DTLS_ANY_VERSION's version negotiation
logic. This allows us to get rid of duplicate ClientHello logic. For
compatibility, SSL_METHOD is now split into SSL_PROTOCOL_METHOD and a version.
The legacy version-locked methods set min_version and max_version based this
version field to emulate the original semantics.
As a bonus, we can now handle fragmented ClientHello versions now.
Because SSLv23_method is a silly name, deprecate that too and introduce
TLS_method.
Change-Id: I8b3df2b427ae34c44ecf972f466ad64dc3dbb171
Tested manually by replacing SSLv23_method() with TLSv1_2_method() in
bssl_shim. This is a large chunk of code which is not run in SSLv23_method(),
but it will be run after unification. It's split out separately to ease review.
Change-Id: I6bd241daca17aa0f9b3e36e51864a29755a41097
Match the DTLS code. Rather than sniffing the handshake state, use the
have_version bit.
Change-Id: I40e92f187647417c34b4cfdc3ad258f5562e781b
Reviewed-on: https://boringssl-review.googlesource.com/2588
Reviewed-by: Adam Langley <agl@google.com>
These tests use both APIs. This also modifies the inline version negotiation's
error codes (currently only used for DTLS) to align with SSLv23's error codes.
Note: the peer should send a protocol_version alert which is currently untested
because it's broken.
Upstream would send such an alert if TLS 1.0 was supported but not otherwise,
which is somewhat bizarre. We've actually regressed and never send the alert in
SSLv23. When version negotiation is unified, we'll get the alerts back.
Change-Id: I4c77bcef3a3cd54a039a642f189785cd34387410
Reviewed-on: https://boringssl-review.googlesource.com/2584
Reviewed-by: Adam Langley <agl@google.com>
Amend the version negotiation tests to test this new spelling of max_version.
min_version will be tested in a follow-up.
Change-Id: Ic4bfcd43bc4e5f951140966f64bb5fd3e2472b01
Reviewed-on: https://boringssl-review.googlesource.com/2583
Reviewed-by: Adam Langley <agl@google.com>
DTLS_method() can now negotiate versions without switching methods.
Change-Id: I0655b3221b6e7e4b3ed4acc45f1f41c594447021
Reviewed-on: https://boringssl-review.googlesource.com/2582
Reviewed-by: Adam Langley <agl@google.com>
SSL3_ENC_METHOD will remain version-specific while SSL_METHOD will become
protocol-specific. This finally removes all the version-specific portions of
SSL_METHOD but the version tag itself.
(SSL3_ENC_METHOD's version-specific bits themselves can probably be handled by
tracking a canonicalized protocol version. It would simplify version
comparisons anyway. The one catch is SSLv3 has a very different table. But
that's a cleanup for future. Then again, perhaps a version-specific method
table swap somewhere will be useful later for TLS 1.3.)
Much of this commit was generated with sed invocation:
s/method->ssl3_enc/enc_method/g
Change-Id: I2b192507876aadd4f9310240687e562e56e6c0b1
Reviewed-on: https://boringssl-review.googlesource.com/2581
Reviewed-by: Adam Langley <agl@google.com>
Now SSLv23 and DTLS_ANY_VERSION share version-related helper functions.
ssl3_get_method is temporary until the method switch is no longer necessary.
Put them all together so there's one place to refactor them when we add a new
version or implement min_version/max_version controls.
Change-Id: Ic28a145cad22db08a87fdb854480b22886c451c6
Reviewed-on: https://boringssl-review.googlesource.com/2580
Reviewed-by: Adam Langley <agl@google.com>
Missed this one. It requires that we be able to change an SSL_METHOD after the
after, which complicates compiling the version locking into min_version /
max_version configurations.
Change-Id: I24ba54b7939360bbfafe3feb355a65840bda7611
Reviewed-on: https://boringssl-review.googlesource.com/2579
Reviewed-by: Adam Langley <agl@google.com>
SSL_ST_BEFORE isn't a possible state anymore. It seems this state meant the
side wasn't known, back in the early SSLeay days. Now upstream guesses
(sometimes incorrectly with generic methods), and we don't initialize until
later. SSL_shutdown also doesn't bother to call ssl3_shutdown at all if the
side isn't initialized and SSL_ST_BEFORE isn't the uninitialized state, which
seems a much more sensible arrangement.
Likewise, because bare SSL_ST_BEFOREs no longer exist, SSL_in_init implies
SSL_in_before and there is no need to check both.
Change-Id: Ie680838b2f860b895073dabb4d759996e21c2824
Reviewed-on: https://boringssl-review.googlesource.com/2564
Reviewed-by: Adam Langley <agl@google.com>
These may as well be replaced with assertions. Get them out of the way of the
initialization.
Change-Id: Ie4ab8bdc018e4a1def7d3f6b3b172a77896bfc0a
Reviewed-on: https://boringssl-review.googlesource.com/2563
Reviewed-by: Adam Langley <agl@google.com>
They're always known now. Also fix the SSLv23_{client,server}_method
definitions still had their own macro invocations.
Change-Id: Ia13f29a27f2331d25a4051e83f2d5abc62fab981
Reviewed-on: https://boringssl-review.googlesource.com/2562
Reviewed-by: Adam Langley <agl@google.com>
There's an undefined one not used anywhere. The others ought to be const. Also
move the forward declaration to ssl.h so we don't have to use the struct name.
Change-Id: I76684cf65255535c677ec19154cac74317c289ba
Reviewed-on: https://boringssl-review.googlesource.com/2561
Reviewed-by: Adam Langley <agl@google.com>
else block got lost in a rewrite of this code.
Change-Id: I51f1655474ec8bbd4eccb4297124e8584329444e
Reviewed-on: https://boringssl-review.googlesource.com/2560
Reviewed-by: Adam Langley <agl@google.com>
The client_version needs to be preserved, both for the RSA key exchange and
(when this codepath is used for TLS) for the SChannel renego workaround. Fix
the tests to enforce this so the cipher suite version tests catch this.
Change-Id: I0c42dc3ec4830f3724026b400e5066e7a7f1ee97
Reviewed-on: https://boringssl-review.googlesource.com/2551
Reviewed-by: Adam Langley <agl@google.com>
There's no need to make that conditional.
Change-Id: Idac1aba42b22e3fe8e7731ae4ecb5ebc4183336c
Reviewed-on: https://boringssl-review.googlesource.com/2550
Reviewed-by: Adam Langley <agl@google.com>
Comparing data is a much easier idiom than CBS_skip + a CBS_len check.
Change-Id: I3efe925734c76f3494cad682445291ae83750a7e
Reviewed-on: https://boringssl-review.googlesource.com/2500
Reviewed-by: Adam Langley <agl@google.com>
The record-layer version of the ServerHello should match the final version. The
record-layer version of the ClientHello should be the advertised version, but
clamped at TLS 1.0. This is to ensure future rewrites do not regress this.
Change-Id: I96f1f0674944997ff38b562453a322ce61652635
Reviewed-on: https://boringssl-review.googlesource.com/2540
Reviewed-by: Adam Langley <agl@google.com>
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>
(Imported from upstream's 7a04b854d655785798d471df25ffd5036f3cc46b.)
This does not affect BoringSSL as ssl3_get_client_hello advances to yet another
state immediately after reading the message. But the state advance is correct.
It matches the normal exit for this function.
Change-Id: I8a664f2ad5f80beacbaf3e17a7786a5c9e8ef30e
Reviewed-on: https://boringssl-review.googlesource.com/2480
Reviewed-by: Piotr Sikora <piotr@cloudflare.com>
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 4b87706d20f0a2fdf2e8f1b90256e141c487ef47 and
eceef8fb865eb5de329b27ea472d4fdea4c290fe.)
Dead code.
Change-Id: I58120c3a9c42cb9db27f404774778222c3bb642a
Reviewed-on: https://boringssl-review.googlesource.com/2479
Reviewed-by: Adam Langley <agl@google.com>
PR#1767
(Imported from upstream's fe78f08d1541211566a5656395186bfbdc61b6f8)
Not sure this is reachable (upstream's PR references custom engines), but
better be tidy. Note this is slightly different from upstream's: EVP_Cipher is
documented to return -1 on failure, not 0.
Change-Id: I836f12b73c6912a8ae8cbd37cfd3d33466acbc9e
Reviewed-on: https://boringssl-review.googlesource.com/2478
Reviewed-by: Adam Langley <agl@google.com>
PR#3613
(Imported from upstream's fc3968a25ce0c16cab8730ec0d68a59856158029)
We don't care about GOST, but removing redundant code is reasonable. Also
switch that CRYPTO_add to EVP_PKEY_dup. Missed a spot.
Change-Id: I768ec546d987fb3d8bc3decf7ebf1a5590fbb6c2
Reviewed-on: https://boringssl-review.googlesource.com/2477
Reviewed-by: Adam Langley <agl@google.com>
Some code predated the RFCs themselves, but the RFCs now exist. Also remove
now obsolete comments and some unused #defines.
See upstream's cffeacd91e70712c99c431bf32a655fa1b561482. (Though this predates
it; I just remembered I never uploaded it.)
Change-Id: I5e56f0ab6b7f558820f72e84dfdbc71a8c23cb91
Reviewed-on: https://boringssl-review.googlesource.com/2475
Reviewed-by: Adam Langley <agl@google.com>
The ClientHello record is padded to 1024 bytes when
fastradio_padding is enabled. As a result, the 3G cellular radio
is fast forwarded to DCH (high data rate) state. This mechanism
leads to a substantial redunction in terms of TLS handshake
latency, and benefits mobile apps that are running on top of TLS.
Change-Id: I3d55197b6d601761c94c0f22871774b5a3dad614
MSVC doesn't like it when you compare the two.
Change-Id: I03c5ff2e2668ac2e536de8278e3a7c98a3dfd117
Reviewed-on: https://boringssl-review.googlesource.com/2460
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>
Don't link with dl, except on Linux where we have malloc tests.
Change-Id: I7b23acc854172e64628a55acecfaa9a661f74f77
Reviewed-on: https://boringssl-review.googlesource.com/2453
Reviewed-by: Adam Langley <agl@google.com>
The comment has it right, but the rewritten code was wrong.
Change-Id: I450193c39fb62eae32aae090a3834dd83db53421
Reviewed-on: https://boringssl-review.googlesource.com/2444
Reviewed-by: Adam Langley <agl@google.com>
Replace the comment with a clearer one and reimplement it much more tidily. The
mask thing was more complicated than was needed.
This slightly changes behavior on the DTLS_ANY_VERSION side in that, if only
one method is enabled, we no longer short-circuit to the version-locked method
early. This "optimization" seems unnecessary.
Change-Id: I571c8b60ed16bd4357c67d65df0dd1ef9cc5eb57
Reviewed-on: https://boringssl-review.googlesource.com/2451
Reviewed-by: Adam Langley <agl@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 SSL_clear is called before SSL_set_{connect,accept}_state (as SSL_new does
internally), s->state will get set prematurely. Likewise, s->server is set
based on the method's ssl_accept hook, but client SSL's may be initialized from
a generic SSL_METHOD too.
Since we can't easily get rid of the generic SSL_METHODs, defer s->state and
s->server initialization until the side is known.
Change-Id: I0972e17083df22a3c09f6f087011b54c699a22e7
Reviewed-on: https://boringssl-review.googlesource.com/2439
Reviewed-by: Adam Langley <agl@google.com>
It's unused. Also per the previous commit message, it historically had a bug
anyway.
Change-Id: I5868641e7938ddebbc0ffd72d218c81cd17c7739
Reviewed-on: https://boringssl-review.googlesource.com/2437
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_clear sets s->state and dtls1_clear sets cookie_len on the server. Setting
cookie_len on the server seems to serve no purpose but to let the callback know
how large the buffer is. This can be done just before calling the callback.
It also avoids a bug where the cookie check can be bypassed, should the server
not specify an app_verify_cookie_cb, by supplying a cookie of all zeros of the
maximum size. (Zero is fine because an empty cookie is rejected.)
The goal here is to avoid needing the SSL_clear calls in the handshake
functions. They are currently needed to fix the cookie_len setting when using
the generic method. (They get set wrong and then flipped back.)
Change-Id: I5095891bc0f7df62d83a9c84312fcf0b84826faa
Reviewed-on: https://boringssl-review.googlesource.com/2435
Reviewed-by: Adam Langley <agl@google.com>
s->server's value isn't final until SSL_connect or SSL_accept is called when
using the generic SSLv23_method or DTLS_method rather than the version-locked
ones. This makes the tests pass if bssl_shim uses those methods.
It would be nicer if the generic methods were gone and an SSL* could know from
creation which half it's destined for. Unfortunately, there's a lot of code
that uses those generic methods, so we probably can't get rid of them. If they
have to stay, it seems better to standardize on only having those, rather than
support both, even if standardizing on the side-specific ones would be
preferable.
Change-Id: I40e65a8842cd6706da92263a263f664336a7f3b3
Reviewed-on: https://boringssl-review.googlesource.com/2434
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 comment is no longer true. It dates from OpenSSL's initial commit, but
stopped being true in upstream's 413c4f45ed0508d2242638696b7665f499d68265.
Change-Id: I47377d992a00e3d57c795fef893e19e109dd6945
Reviewed-on: https://boringssl-review.googlesource.com/2431
Reviewed-by: Adam Langley <agl@google.com>
We intend to deprecate the version-locked methods and unify them. Don't expose
that there's a method swap. (The existing version-locked methods will merely be
a shorthand for configuring minimum/maximum versions.)
There is one consumer of SSL_get_ssl_method in internal code, but it's just
some logging in test-only code. All it's doing is getting the version as a
string which should be SSL_get_version instead.
While here, also remove dead ssl_bad_method function. Also the bogus
ssl_crock_st forward-declaration. The forward declaration in base.h should be
perfectly sufficient.
Change-Id: I50480808f51022e05b078a285f58ec85d5ad7c8e
Reviewed-on: https://boringssl-review.googlesource.com/2408
Reviewed-by: Adam Langley <agl@google.com>
When not offering to resume a session, the client populates s->session with a
fresh SSL_SESSION before the ServerHello is processed and, in DTLS_ANY_VERSION,
before the version is even determined. Don't create a fresh SSL_SESSION until
we know we are doing a full handshake.
This brings ssl3_send_client_hello closer to ssl23_client_hello in behavior. It
also fixes ssl_version in the client in DTLS_ANY_VERSION.
SSLv23_client_method is largely unchanged. If no session is offered, s->session
continues to be NULL until the ServerHello is received. The one difference is
that s->session isn't populated until the entire ServerHello is received,
rather than just the first half, in the case of a fragmented ServerHello. Apart
from info_callback, no external hooks get called between those points, so this
shouldn't expose new missing NULL checks.
The other client methods change significantly to match SSLv23_client_method's
behavior. For TLS, any exposed missing NULL checks are also in
SSLv23_client_method (and version-specific methods are already weird), so that
should be safe. For DTLS, I've verified that accesses in d1_*.c either handle
NULL or are after the ServerHello.
Change-Id: Idcae6bd242480e28a57dbba76ce67f1ac1ae1d1d
Reviewed-on: https://boringssl-review.googlesource.com/2404
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>
We forgot to add those when we implemented the features. (Also relevant because
they will provide test coverage later for configuring features when using the
generic method tables rather than *_client_method.)
Change-Id: Ie08b27de893095e01a05a7084775676616459807
Reviewed-on: https://boringssl-review.googlesource.com/2410
Reviewed-by: Adam Langley <agl@google.com>
Although the comment suggests this was added with an s->session check to
account for SSL_set_session switching methods (which we will remove in the next
commit) and to account for SSLv23_method switching methods (which we hope to
remove after a long tower of cleanup), the current codepath never runs and
can't work:
If it is called prior to handshaking or setting a session, no method switch has
happened so that codepath is dead. If it is called after setting a session, the
s->session check will keep it from running. If it is called after a handshake,
we will have established a session so that check will again keep it from
running. (Finally, if it is called during the handshake, the in_handshake check
will stop; that there is an SSL_clear call in the handshake state machine at
all is a bug that will be addressed once more things are disentangled. See
upstream's 979689aa5cfa100ccbc1f25064e9398be4b7b05c.)
Were that code to ever run, the SSL* would be in an inconsistent state. It
switches the method, but not the handshake_func. The handshake_func isn't
switched to NULL, so that will keep the SSL_connect and SSL_accept code from fixing it.
It seems the intent was that the caller would always call
SSL_set_{connect,accept}_state to fix this. But as of upstream's
b31b04d951e9b65bde29657e1ae057b76f0f0a73, this is not necessary and indeed
isn't called by a lot of consumer code.
Change-Id: I710652b1d565b77bc26f913c2066ce749a9025c9
Reviewed-on: https://boringssl-review.googlesource.com/2430
Reviewed-by: Adam Langley <agl@google.com>
The data is owned by the SSL_SESSION, so the caller should not modify it. This
will require changes in Chromium, but they should be trivial.
Change-Id: I314718530c7d810f7c7b8852339b782b4c2dace1
Reviewed-on: https://boringssl-review.googlesource.com/2409
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 implements session IDs in client and server in runner.go.
Change-Id: I26655f996b7b44c7eb56340ef6a415d3f2ac3503
Reviewed-on: https://boringssl-review.googlesource.com/2350
Reviewed-by: Adam Langley <agl@google.com>
The ex_data index may fail to be allocated. Also don't leave a dangling pointer
in handshake_dgst if EVP_DigestInit_ex fails and check a few more init function
failures.
Change-Id: I2e99a89b2171c9d73ccc925a2f35651af34ac5fb
Reviewed-on: https://boringssl-review.googlesource.com/2342
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>
CERT_PKEY_SIGN isn't meaningful since, without strict mode, we always fall back
to SHA-1 anyway. So the digest is never NULL when CERT_PKEY_SIGN is computed.
The entire valid_flags is now back to it's pre-1.0.2 check of seeing if the
certificate and key are configured.
This finally removes the sensitivity between valid_flags and selecting the
digest, so we can defer choosing the digest all we like.
Change-Id: I9f9952498f512d7f0cc799497f7c5b52145a48af
Reviewed-on: https://boringssl-review.googlesource.com/2288
Reviewed-by: Adam Langley <agl@google.com>
It doesn't depend on the cipher now that export ciphers are gone. It need only
be called once. Also remove the valid bit; nothing ever reads it. Its output is
also only used within a function, so make mask_k and mask_a local variables.
So all the configuration-based checks are in one place, change the input
parameter from CERT to SSL and move the PSK and ECDHE checks to the mask
computation. This avoids having to evaluate the temporary EC key for each
cipher.
The remaining uses are on the client which uses them differently (disabled
features rather than enabled ones). Those too may as well be local variables,
so leave a TODO.
Change-Id: Ibcb574341795d4016ea749f0290a793eed798874
Reviewed-on: https://boringssl-review.googlesource.com/2287
Reviewed-by: Adam Langley <agl@google.com>
Many are now unused. Only two are currently considered in cipher selection:
CERT_PKEY_VALID and CERT_PKEY_SIGN. (As per previous commits, this is either
bizarre due to limited slots or redundant with ssl_early_callback_ctx. We can
probably prune this too.)
This also fixes a bug where DTLS 1.0 went through a TLS 1.2 codepath. As the
DTLS code is currently arranged, all version comparisons must be done via
macros like SSL_USE_SIGALGS. (Probably we should add functions to map from DTLS
to TLS versions and slowly move the library to using the TLS version as
in-memory representation.)
Change-Id: I89bcf5b7b9ea5cdecf54f4445156586377328fe0
Reviewed-on: https://boringssl-review.googlesource.com/2286
Reviewed-by: Adam Langley <agl@google.com>
It's new in OpenSSL 1.0.2 so it's never set by existing code. This removes gobs
and gobs of complexity from tls1_check_chain. It only checks the local
certificate, not the peer certificate. The uses appear to be:
- Sanity-check configuration. Not worth the complexity.
- Guide in selecting ciphers based on ClientHello parameters and which
certificates in the CERT_PKEY are compatible. This isn't very useful one its
own since the CERT_PKEY array only stores one slot per type (e.g. you cannot
configure RSA/SHA-1 and RSA/SHA-256).
- For the (currently removed) SSL_check_chain to return more information based
on ClientHello parameters and guide selecting a certificate. This is
potentially useful but, as noted in the commit which removed it, redundant
with ssl_early_callback_ctx.
This CL is largely mechanical removing of dead codepaths. The follow-up will
clean up the now unnecessary parts of this function.
Change-Id: I2ebfa17e4f73e59aa1ee9e4ae7f615af2c6cf590
Reviewed-on: https://boringssl-review.googlesource.com/2285
Reviewed-by: Adam Langley <agl@google.com>
Get rid of now dead codepaths.
Change-Id: I3b5d49097cba70c5698a230cc6c1d79bdd0f0880
Reviewed-on: https://boringssl-review.googlesource.com/2284
Reviewed-by: Adam Langley <agl@google.com>
Both of these are newly-exported in OpenSSL 1.0.2, so they cannot be used by
current consumers.
This was added in upstream's 18d7158809c9722f4c6d2a8af7513577274f9b56 to
support custom selection of certificates. The intent seems to be that you
listen to cert_cb and use SSL_check_chain to lean on OpenSSL to process
signature algorithms list for you.
Unfortunately, the implementation is slightly suspect: it uses the same
function as the codepath which mutates and refers to the CERT_PKEY of the
matching type. Some access was guarded by check_flags, but this is too
complex. Part of it is also because the matching digest is selected early and
we intend to connect this to EVP_PKEY_supports_digest so it is no longer a
property of just the key type.
Let's remove the hook for now, to unblock removing a lot of complexity. After
cleaning up this area, a function like this could be cleaner to support, but
we already have a version of this: select_certificate_cb and
ssl_early_callback_ctx.
Change-Id: I3add425b3996e5e32d4a88e14cc607b4fdaa5aec
Reviewed-on: https://boringssl-review.googlesource.com/2283
Reviewed-by: Adam Langley <agl@google.com>
This is maintained just to distinguish whether the digest was negotiated or we
simply fell back to assuming SHA-1 support. No code is sensitive to this flag
and it adds complexity because it is set at a different time, for now, from the
rest of valid_flags.
The flag is new in OpenSSL 1.0.2, so nothing external could be sensitive to it.
Change-Id: I9304e358d56f44d912d78beabf14316d456bf389
Reviewed-on: https://boringssl-review.googlesource.com/2282
Reviewed-by: Adam Langley <agl@google.com>
This is new in OpenSSL 1.0.2 so it isn't used anywhere. Cuts down slightly on
connection-global state associated with signature algorithm processing.
Repurposing the digest field to mean both "the digest we choose to sign with
this key" and "the digest the last signature we saw happened to use" is
confusing.
Change-Id: Iec4d5078c33e271c8c7b0ab221c356ee8480b89d
Reviewed-on: https://boringssl-review.googlesource.com/2281
Reviewed-by: Adam Langley <agl@google.com>
