CBC modes in SSLv3 are bust already with POODLE and we're moving away from it.
Align all the names from 'ssl3' and 'tls1' to 'tls', to match the names of the
TLS-only AEADs.
Change-Id: If742296a8e2633ef42a484e4d873b4a83558b6aa
Reviewed-on: https://boringssl-review.googlesource.com/2693
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>
These helper functions will be used in the implementation of the legacy CBC
mode AEADs. The file is copied as-is and then modified to remove the dependency
on ssl/. Notably explicit IV logic is removed (that's a side effect of how
explicit IVs are currently implemented) and the padding length is returned
directly rather than smuggled into rec->type.
(Diffing tls_cbc.c and s3_cbc.c is probably the easiest for a review.)
The helpers are currently unused.
Change-Id: Ib703f4d3620196c9f2921cb3b8bf985f2d1777db
Reviewed-on: https://boringssl-review.googlesource.com/2691
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>
The extra free in ex_data_impl.c is fixing a mistake: when calling
|CRYPTO_cleanup_all_ex_data| the |EX_CLASS_ITEM| itself wouldn't be
freed.
The change in err_impl.c is to free the thread-id hash also. This allows
programs to free absolutely all memory allocated by BoringSSL, which
allows fuzz testing to find any memory leaks.
Change-Id: I1e518adf2b3e0efa7d7f00f7ab4e65e1dc70161e
Reviewed-on: https://boringssl-review.googlesource.com/2670
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>
We aren't targeting pure C89 any longer and it also upsets GCC on
AArch64 because asm() isn't part of C89.
Change-Id: I0ba299160e2f0c40d9a99ea8df13b4bb33c08163
Reviewed-on: https://boringssl-review.googlesource.com/2800
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>
DSA_verify and DSA_check_signature didn't share a codepath, so the fix was only
applied to the former. Implement verify in terms of check_signature and add
tests for bad DER variants.
Change-Id: I6577f96b13b57fc89a5308bd8a7c2318defa7ee1
Reviewed-on: https://boringssl-review.googlesource.com/2820
Reviewed-by: Adam Langley <agl@google.com>
By using non-DER or invalid encodings outside the signed portion of a
certificate the fingerprint can be changed without breaking the signature.
Although no details of the signed portion of the certificate can be changed
this can cause problems with some applications: e.g. those using the
certificate fingerprint for blacklists.
1. Reject signatures with non zero unused bits.
If the BIT STRING containing the signature has non zero unused bits reject the
signature. All current signature algorithms require zero unused bits.
2. Check certificate algorithm consistency.
Check the AlgorithmIdentifier inside TBS matches the one in the certificate
signature. NB: this will result in signature failure errors for some broken
certificates.
3. Check DSA/ECDSA signatures use DER.
Reencode DSA/ECDSA signatures and compare with the original received signature.
Return an error if there is a mismatch.
This will reject various cases including garbage after signature (thanks to
Antti Karjalainen and Tuomo Untinen from the Codenomicon CROSS program for
discovering this case) and use of BER or invalid ASN.1 INTEGERs (negative or
with leading zeroes).
CVE-2014-8275
(Imported from upstream's 85cfc188c06bd046420ae70dd6e302f9efe022a9 and
4c52816d35681c0533c25fdd3abb4b7c6962302d)
Change-Id: Ic901aea8ea6457df27dc542a11c30464561e322b
Reviewed-on: https://boringssl-review.googlesource.com/2783
Reviewed-by: David Benjamin <davidben@chromium.org>
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>
Some parts of Android can't be updated yet so this change adds
declarations (only) for some functions that will be stubbed in
Android-specific code. (That Android-specific code will live in the
Android repo, not the BoringSSL repo.)
Trying to use these functions outside of Android will result in a link
error.
Change-Id: Iaa9b956e6408d21cd8fc34d90d9c15657e429877
Reviewed-on: https://boringssl-review.googlesource.com/2760
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Write the array literal of all zeros as {0} rather than {}.
Change-Id: If15330d96d019be671d3bcbbdea60c2b3ecc2128
Reviewed-on: https://boringssl-review.googlesource.com/2740
Reviewed-by: Adam Langley <agl@google.com>
I typoed this word and then auto-complete duplicated it all over the
place. This change fixes all the comments.
This change has no semantic effect (comment only).
Change-Id: I8952e9e71302043574757cd74a05e66500008432
Using OPENSSL_WINDOWS for this is inaccurate because it's really a
feature of the compiler, not the platform. I think it's only MSVC that
uses the UI64 suffix.
Change-Id: I4a95961b94e69e72b93f5ed1e0457661b74242c8
Reviewed-on: https://boringssl-review.googlesource.com/2730
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>
ERR_get_error returns the least recent error, not the most recent error.
Nothing in err_test was actually asserting on that.
Change-Id: Ia49e29c231de4bbec77d037860ad1ffa8cce4779
Reviewed-on: https://boringssl-review.googlesource.com/2750
Reviewed-by: Adam Langley <agl@google.com>
One about a possible uninitialised variable (incorrect, but it's easier
to keep the compiler happy) and one warning about "const static" being
backwards.
Change-Id: Ic5976a5f0b48f32e09682e31b65d8ea1c27e5b88
Reviewed-on: https://boringssl-review.googlesource.com/2632
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Since this is C89 we need to maintain this ancient practice.
Change-Id: I7223e7c38a35cf551b6e3c9159d2e21ebf7e62be
Reviewed-on: https://boringssl-review.googlesource.com/2631
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
It's a static function anyway so it doesn't affect anything and it's
colliding with a debugging function on one platform.
Change-Id: Iae0595cce7cb2bdd4c56217f6f1de51ff3134a8b
Reviewed-on: https://boringssl-review.googlesource.com/2630
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The expectation when calling HMAC with key=NULL and keylen=0 is to compute
HMAC on the provided data with a key of length 0 instead of using the
"previous" key, which in the case of HMAC() is whatever bytes happen to be
left on the stack when the HMAC_CTX struct is allocated.
Change-Id: I52a95e262ee4e15f1af3136cb9c07f42f40ce122
Reviewed-on: https://boringssl-review.googlesource.com/2660
Reviewed-by: Adam Langley <agl@google.com>
It doesn't retain partial blocks but it DOES update internal cipher state. ssl/
depends on this property.
Change-Id: I1e44b612c2e1549e096de8b71726007dcbc68de3
Reviewed-on: https://boringssl-review.googlesource.com/2640
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
