They're not in the duplicated handshake state machines anyway. But we still
shouldn't negotiate them. d1_pkt.c assumes Finished is the only post-CCS
handshake message. An unexpected handshake message in the current epoch may
either be a retransmit/out-of-order message from the previous handshake, or a
message from the next handshake (also potentially out-of-order). In the former
case, we shouldn't spin up another handshake state machine instance.
(This assumption is required due to a protocol bug. DTLS resets sequence
numbers after a handshake, so it is necessary to categorize handshake fragments
by pre-CCS and post-CCS to distinguish between retransmit and renego.)
Change-Id: Ib3c1c7085c729e36a40f7ff14494733156924a24
Reviewed-on: https://boringssl-review.googlesource.com/3028
Reviewed-by: Adam Langley <agl@google.com>
This regressed in e95d20dcb8. EVP_AEAD will push
errors on the error queue (unlike the EVP_CIPHER codepath which checked
everything internally to ssl/ and didn't bother pushing anything). This meant
that a dropped packet would leave junk in the error queue.
Later, when SSL_read returns <= 0 (EOF or EWOULDBLOCK), the non-empty error
queue check in SSL_get_error kicks in and SSL_read looks to have failed.
BUG=https://code.google.com/p/webrtc/issues/detail?id=4214
Change-Id: I1e5e41c77a3e5b71e9eb0c72294abf0da677f840
Reviewed-on: https://boringssl-review.googlesource.com/2982
Reviewed-by: Adam Langley <agl@google.com>
This regressed in fcf25833bc. 0 return code on
unclean shutdown means the underlying BIO returned EOF, didn't push any error
code, but we haven't seen close_notify yet. The intent seems to be that you go
check errno or some BIO-specific equivalent if you care about close_notify.
Make sure test code routes all SSL_read return codes through SSL_get_error
since that's supposed to work in all cases.
(Note that rv == 0 can still give SSL_ERROR_SSL if the error queue is not
empty.)
Change-Id: I45bf9614573f876d93419ce169a4e0d9ceea9052
Reviewed-on: https://boringssl-review.googlesource.com/2981
Reviewed-by: Adam Langley <agl@google.com>
The distinction between publicly and non-publicly invalid is barely acted upon
and slightly silly now that the CBC padding check has been folded into
EVP_AEAD.
Change-Id: Idce4b9b8d29d624e3c95243a147265d071612127
Reviewed-on: https://boringssl-review.googlesource.com/2980
Reviewed-by: Adam Langley <agl@google.com>
This is an initial cut at aarch64 support. I have only qemu to test it
however—hopefully hardware will be coming soon.
This also affects 32-bit ARM in that aarch64 chips can run 32-bit code
and we would like to be able to take advantage of the crypto operations
even in 32-bit mode. AES and GHASH should Just Work in this case: the
-armx.pl files can be built for either 32- or 64-bit mode based on the
flavour argument given to the Perl script.
SHA-1 and SHA-256 don't work like this however because they've never
support for multiple implementations, thus BoringSSL built for 32-bit
won't use the SHA instructions on an aarch64 chip.
No dedicated ChaCha20 or Poly1305 support yet.
Change-Id: Ib275bc4894a365c8ec7c42f4e91af6dba3bd686c
Reviewed-on: https://boringssl-review.googlesource.com/2801
Reviewed-by: Adam Langley <agl@google.com>
SSL_library_init already loads the error strings (unlike upstream). Code which
calls both will end up loading error strings twice. Instead make the second
call a no-op.
Change-Id: Ifd34ab20ed46aabeba14661e58f8dac2bbb29f69
Reviewed-on: https://boringssl-review.googlesource.com/2790
Reviewed-by: Adam Langley <agl@google.com>
The under 32 constraint is silly; it's to check for duplicate curves in
library-supplied configuration. That API is new as of 1.0.2. It doesn't seem
worth bothering; if the caller supplies a repeated value, may as well emit a
repeated one and so be it. (Probably no one will ever call that function
outside of maybe test code anyway.)
While I'm here, remove the 0 constraint too. It's not likely to change, but
removing the return value overload seems easier than keeping comments about it
comments about it.
Change-Id: I01d36dba1855873875bb5a0ec84b040199e0e9bc
Reviewed-on: https://boringssl-review.googlesource.com/2844
Reviewed-by: Adam Langley <agl@google.com>
We only implement four curves (P-224, P-256, P-384, and P-521) and only
advertise the latter three by default. Don't maintain entries corresponding to
all the unimplemented curves.
Change-Id: I1816a10c6f849ca1d9d896bc6f4b64cd6b329481
Reviewed-on: https://boringssl-review.googlesource.com/2843
Reviewed-by: Adam Langley <agl@google.com>
They both happen to be zero, but OBJ_undef is a type error; OBJ_foo expands to
a comma-separated list of integers.
Change-Id: Ia5907dd3bc83240b7cc98af6456115d2efb48687
Reviewed-on: https://boringssl-review.googlesource.com/2842
Reviewed-by: Adam Langley <agl@google.com>
When parsing ClientHello clear any existing extension state from
SRP login and SRTP profile.
(Imported from upstream's 4f605ccb77)
More state that should be systematically reset across handshakes. Add a reset
on the ServerHello end too since that was missed.
Change-Id: Ibb4549acddfd87caf7b6ff853e2adbfa4b7e7856
Reviewed-on: https://boringssl-review.googlesource.com/2838
Reviewed-by: Adam Langley <agl@google.com>
write_quota should only be decremented by 1 in datagram mode, otherwise we'll
underflow and always allow writes through. This does not cause any existing
tests to fail.
(It will be useful once the bug in dtls1_do_write is fixed.)
Change-Id: I42aa001d7264790a3726269890635f679497fb1c
Reviewed-on: https://boringssl-review.googlesource.com/2831
Reviewed-by: Adam Langley <agl@google.com>
The existing comments are not very helpful. This code is also quite buggy.
Document two of them as TODOs.
Change-Id: Idfaf93d9c3b8b1ee92f2fb0d292ef513b5f6d824
Reviewed-on: https://boringssl-review.googlesource.com/2830
Reviewed-by: Adam Langley <agl@google.com>
BIO_ctrls do not have terribly well-defined return values on error. (Though the
existing ones seem to all return 0, not -1, on nonexistant operation.)
Change-Id: I08497f023ce3257c253aa71517a98b2fe73c3f74
Reviewed-on: https://boringssl-review.googlesource.com/2829
Reviewed-by: Adam Langley <agl@google.com>
g_probably_mtu and dtls1_guess_mtu is a bunch of logic for guessing the right
MTU, but it only ever returns the maximum (the input is always zero). Trim that
down to only what it actually does.
Change-Id: If3afe3f68ccb36cbf9c4525372564d16a4bbb73f
Reviewed-on: https://boringssl-review.googlesource.com/2828
Reviewed-by: Adam Langley <agl@google.com>
The retry doesn't actually work when we're sending a non-initial fragment; the
s->init_off != 0 block will get re-run each iteration through and continually
prepend headers. It can also infinite loop if the BIO reports
BIO_CTRL_DGRAM_MTU_EXCEEDED but either fails to report an MTU or reports an MTU
that always rounds up to the minimum. See upstream's
d3d9eef316.
WebRTC doesn't participate in any of the MTU logic and inherits the default
MTU, so just remove it for now.
Change-Id: Ib2ed2ba016b7c229811741fb7369c015ba0b551f
Reviewed-on: https://boringssl-review.googlesource.com/2827
Reviewed-by: Adam Langley <agl@google.com>
That setting means that the MTU is provided externally via SSL_set_mtu.
(Imported from upstream's 001235778a)
Change-Id: I4e5743a9dee734ddd0235f080aefe98a7365aaf6
Reviewed-on: https://boringssl-review.googlesource.com/2826
Reviewed-by: Adam Langley <agl@google.com>
Based in part on upstream's cf75017bfd. This
situation really shouldn't be able to happen, but between no static asserts
that the minimum MTU is always large enough and a bug in reseting the MTU later
(to be fixed be a follow-up import from upstream), check these and return a
useful error code.
Change-Id: Ie853e5d35a6a7bc9c0032e74ae71529d490f4fe2
Reviewed-on: https://boringssl-review.googlesource.com/2825
Reviewed-by: Adam Langley <agl@google.com>
BoringSSL currently retransmits non-deterministically on an internal timer
(rather than one supplied externally), so the tests currently fail flakily
depending on timing. Valgrind is a common source for this. We still assume an
in-order and reliable channel, but drop retransmits silently:
- Handshake messages may arrive with old sequence numbers.
- Retransmitted CCS records arrive from the previous epoch.
- We may receive a retransmitted Finished after we believe the handshake has
completed. (Aside: even in a real implementation, only Finished is possible
here. Even with out-of-order delivery, retransmitted or reordered messages
earlier in the handshake come in under a different epoch.)
Note that because DTLS renego and a Finished retransmit are ambiguous at the
record layer[*], this precludes us writing tests for DTLS renego. But DTLS
renego should get removed anyway. As BoringSSL currently implements renego,
this ambiguity is also a source of complexity in the real implementation. (See
the SSL3_MT_FINISHED check in dtls1_read_bytes.)
[*] As a further fun aside, it's also complex if dispatching renego vs Finished
after handshake message reassembly. The spec doesn't directly say the sequence
number is reset across renegos, but it says "The first message each side
transmits in /each/ handshake always has message_seq = 0". This means that such
an implementation needs the handshake message reassembly logic be aware that a
Finished fragment with high sequence number is NOT an out-of-order fragment for
the next handshake.
Change-Id: I35d13560f82bcb5eeda62f4de1571d28c818cc36
Reviewed-on: https://boringssl-review.googlesource.com/2770
Reviewed-by: Adam Langley <agl@google.com>
As of our 82b7da271ff13a2878e921f0d262812d111a7b84, an SSL_SESSION created
externally always has a cipher set. Unknown ciphers are rejected early. Prior
to that, an SSL_SESSION would only have a valid cipher or valid cipher_id
depending on whether it came from an internal or external session cache.
See upstream's 6a8afe2201 and
c566205319 for more context.
Since we don't get ourselves into this strange situation and s->cipher is now
always valid for established SSL_SESSION objects (the existence of
unestablished SSL_SESSION objects during a handshake is awkward, but something
to deal with later), do away with s->cipher_id altogether. An application
should be able to handle failing to parse an SSL_SESSION instead of parsing it
successfuly but rejecting all resumptions.
Change-Id: I2f064a815e0db657b109c7c9269ac6c726d1ffed
Reviewed-on: https://boringssl-review.googlesource.com/2703
Reviewed-by: Adam Langley <agl@google.com>
This CL removes the last of the EVP_CIPHER codepath in ssl/. The dead code is
intentionally not pruned for ease of review, except in DTLS-only code where
adding new logic to support both, only to remove half, would be cumbersome.
Fixes made:
- dtls1_retransmit_state is taught to retain aead_write_ctx rather than
enc_write_ctx.
- d1_pkt.c reserves space for the variable-length nonce when echoed into the
packet.
- dtls1_do_write sizes the MTU based on EVP_AEAD max overhead.
- tls1_change_cipher_state_cipher should not free AEAD write contexts in DTLS.
This matches the (rather confused) ownership for the EVP_CIPHER contexts.
I've added a TODO to resolve this craziness.
A follow-up CL will remove all the resultant dead code.
Change-Id: I644557f4db53bbfb182950823ab96d5e4c908866
Reviewed-on: https://boringssl-review.googlesource.com/2699
Reviewed-by: Adam Langley <agl@google.com>
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 7c6a3cf237).
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>
David Benjamin
Adam Langley
Nick Harper