Fixes failed compile with [-Werror=implicit-fallthrough=], which is
default on gcc-7.x on distributions like fedora.
Enabling no implicit fallthrough for more than just clang as well to
catch this going forward.
Change-Id: I6cd880dac70ec126bd7812e2d9e5ff804d32cadd
Signed-off-by: Vincent Batts <vbatts@redhat.com>
Reviewed-on: https://boringssl-review.googlesource.com/20564
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The Java client implementation of the 3SHAKE mitigation incorrectly
rejects initial handshakes when all of the following are true:
1. The ClientHello offered a session.
2. The session was successfully resumed previously.
3. The server declines the session.
4. The server sends a certificate with a different SAN list than in the
previous session.
(Note the 3SHAKE mitigation is to reject certificates changes on
renegotiation, while Java's logic applies to initial handshakes as
well.)
The end result is long-lived Java clients break on some certificate
rotations. Fingerprint Java clients and decline all offered sessions.
This avoids (2) while still introducing new sessions to clear any
existing problematic sessions.
See also b/65323005.
Change-Id: Ib2b84c69b5ecba285ffb8c4d03de5626838d794e
Reviewed-on: https://boringssl-review.googlesource.com/20184
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Right now we report the per-connection value during the handshake and
the per-session value after the handshake. This also trims our tickets
slightly by removing a largely unused field from SSL_SESSION.
Putting it on SSL_HANDSHAKE would be better, but sadly a number of
bindings-type APIs expose it after the handshake.
Change-Id: I6a1383f95da9b1b141b9d6adadc05ee1e458a326
Reviewed-on: https://boringssl-review.googlesource.com/20064
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This fixes a regression in Conscrypt added by
https://boringssl-review.googlesource.com/19144. SSL_get_session
otherwise attempts to return hs->new_session, but that has been released
at this point.
Change-Id: I55b41cbefb65b3ae3cfbfad72f6338bd66db3341
Reviewed-on: https://boringssl-review.googlesource.com/19904
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
That's the last of it!
Change-Id: I93d1f5ab7e95b2ad105c34b24297a0bf77625263
Reviewed-on: https://boringssl-review.googlesource.com/19784
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Bug: 128
Change-Id: Ief3779b1c43dd34a154a0f1d2f94d0da756bc07a
Reviewed-on: https://boringssl-review.googlesource.com/19144
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I messed up https://boringssl-review.googlesource.com/8883 and caused
both sides to believe they had sent the final Finished. Use next_message
to detect whether our last flight had a reply.
Change-Id: Ia4d8c8eefa818c9a69acc94d63c9c863293c3cf5
Reviewed-on: https://boringssl-review.googlesource.com/19604
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
SSL_state_string_long and SSL_state_string are often used for debugging
purposes. The latter's 6-letter codes are absurd, but
SSL_state_string_long is plausible. So we don't lose this when
converging state machines or switching to TLS 1.3, add this to TLS 1.3.
Bug: 128
Change-Id: Iec6529a4d9eddcf08bc9610137b4ccf9ea2681a6
Reviewed-on: https://boringssl-review.googlesource.com/19524
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The ticket encryption key is rotated automatically once every 24 hours,
unless a key has been configured manually (i.e. using
|SSL_CTX_set_tlsext_ticket_keys|) or one of the custom ticket encryption
methods is used.
Change-Id: I0dfff28b33e58e96b3bbf7f94dcd6d2642f37aec
Reviewed-on: https://boringssl-review.googlesource.com/18924
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This loosens the earlier restriction to match Channel ID. Both may be
configured and offered, but the server is obligated to select only one
of them. This aligns with the current tokbind + 0-RTT draft where the
combination is signaled by a separate extension.
Bug: 183
Change-Id: I786102a679999705d399f0091f76da236be091c2
Reviewed-on: https://boringssl-review.googlesource.com/19124
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Use SSL_SESSION_get_digest instead of the lower level function where
applicable. Also, remove the failure case (Ivan Maidanski points out in
https://android-review.googlesource.com/c/337852/1/src/ssl/t1_enc.c that
this unreachable codepath is a memory leak) by passing in an SSL_CIPHER
to make it more locally obvious that other values are impossible.
Change-Id: Ie624049d47ab0d24f32b405390d6251c7343d7d6
Reviewed-on: https://boringssl-review.googlesource.com/19024
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Rather than init_msg/init_num, there is a get_message function which
either returns success or try again. This function does not advance the
current message (see the previous preparatory change). It only completes
the current one if necessary.
Being idempotent means it may be freely placed at the top of states
which otherwise have other asychronous operations. It also eases
converting the TLS 1.2 state machine. See
https://docs.google.com/a/google.com/document/d/11n7LHsT3GwE34LAJIe3EFs4165TI4UR_3CqiM9LJVpI/edit?usp=sharing
for details.
The read_message hook (later to be replaced by something which doesn't
depend on BIO) intentionally does not finish the handshake, only "makes
progress". A follow-up change will align both TLS and DTLS on consuming
one handshake record and always consuming the entire record (so init_buf
may contain trailing data). In a few places I've gone ahead and
accounted for that case because it was more natural to do so.
This change also removes a couple pointers of redundant state from every
socket.
Bug: 128
Change-Id: I89d8f3622d3b53147d69ee3ac34bb654ed044a71
Reviewed-on: https://boringssl-review.googlesource.com/18806
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
WebRTC will need this (probably among other things) to lose crypto/x509
at some point.
Bug: chromium:706445
Change-Id: I988e7300c4d913986b6ebbd1fa4130548dde76a4
Reviewed-on: https://boringssl-review.googlesource.com/18904
Reviewed-by: David Benjamin <davidben@google.com>
With on_handshake_complete, this can be managed internally by the TLS
code. The next commit will add a ton more calls to this function.
Change-Id: I91575d3e4bfcccbbe492017ae33c74b8cc1d1340
Reviewed-on: https://boringssl-review.googlesource.com/18865
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Instead, the DTLS driver can detect these states implicitly based on
when we write flights and when the handshake completes. When we flush a
new flight, the peer has enough information to send their reply, so we
start a timer. When we begin assembling a new flight, we must have
received the final message in the peer's flight. (If there are
asynchronous events between, we may stop the timer later, but we may
freely stop the timer anytime before we next try to read something.)
The only place this fails is if we were the last to write a flight,
we'll have a stray timer. Clear it in a handshake completion hook.
Change-Id: I973c592ee5721192949a45c259b93192fa309edb
Reviewed-on: https://boringssl-review.googlesource.com/18864
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This would only come up if the peer didn't pack records together, but
it's free to handle. Notably OpenSSL has a bug where it does not pack
retransmits together.
Change-Id: I0927d768f6b50c62bacdd82bd1c95396ed503cf3
Reviewed-on: https://boringssl-review.googlesource.com/18724
Reviewed-by: David Benjamin <davidben@google.com>
There are still a ton of them, almost exclusively complaints that
function declaration and definitions have different parameter names. I
just fixed a few randomly.
Change-Id: I1072f3dba8f63372cda92425aa94f4aa9e3911fa
Reviewed-on: https://boringssl-review.googlesource.com/18706
Reviewed-by: Steven Valdez <svaldez@google.com>
Bug: 132
Change-Id: I75d6ce5a2256a4b464ca6a9378ac6b63a9bd47e2
Reviewed-on: https://boringssl-review.googlesource.com/18644
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
placement new requires operator new (size_t, void*) to be defined, which
requires pulling in the <new> header.
Change-Id: Ibaa8f3309b03129958f201d32de8afcfafed70f6
Reviewed-on: https://boringssl-review.googlesource.com/18664
Reviewed-by: David Benjamin <davidben@google.com>
The first line of bssl::New is invalid in LLVM CFI as we are casting a
pointer to T before the object is constructed. Instead, we should leave
it as void* and only use it as a T* afterward being constructed.
Bug: chromium:750445
Change-Id: I0ae60c2a7e541b45bc0155dd8f359b662f561dcc
Reviewed-on: https://boringssl-review.googlesource.com/18684
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Similarly, add EVP_AEAD_CTX_tag_len which computes the exact tag length
for required by EVP_AEAD_CTX_seal_scatter.
Change-Id: I069b0ad16fab314fd42f6048a3c1dc45e8376f7f
Reviewed-on: https://boringssl-review.googlesource.com/18324
Reviewed-by: Adam Langley <agl@google.com>
It returns false for incomplete types (or is undefined prior to C++14),
so other instantiations can get confused. Instead, require an explicit
kAllowUniquePtr toggle.
I tried using sizeof(T) to SFINAE-detect an incomplete type but ran into
MSVC issues, I think
https://connect.microsoft.com/VisualStudio/feedback/details/820390/vc-sizeof-doesnt-work-as-expected-in-sfinae-context
Though it seems this also may cause ODR violations if different
compilation units disagree on whether a type is complete. This is all a
mess, so just do the boring thing.
Bug: 132
Change-Id: I6f2d47499f16e75f62629c76f43a5329e91c6daf
Reviewed-on: https://boringssl-review.googlesource.com/18464
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
This allows us to avoid omitting all the silly abort() flags in
reasonable downstreams like Chromium, while the holdouts are fixed. It
also means that we still get the compiler checking that we've
implemented all pure virtuals in some build configurations, which we'll
put on a bot somewhere.
Bug: 132
Change-Id: If500749f7100bb22bb8e828e8ecf38a992ae9fe5
Reviewed-on: https://boringssl-review.googlesource.com/18406
Reviewed-by: Steven Valdez <svaldez@google.com>
SSL_get0_peer_certificates is documented to return NULL if the peer was
anonymous, but it actually returns a non-NULL empty list (except in SSL
3.0 where the Certificate message and thus ssl_parse_cert_chain is
skipped).
Make the implementation match the documentation.
Change-Id: Ib3e25d2155f316cc5e9eb3ab7f74b78e08b8a86b
Reviewed-on: https://boringssl-review.googlesource.com/18226
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Short-term, we will need to use these macros and build without RTTI when
defining any virtual base class. Long-term, it would be good to remove
these constraints, but it will require some downstream work.
Bug: 132
Change-Id: I3bc65bb12d7653978612b7d1bf06f772a2f3b1cd
Reviewed-on: https://boringssl-review.googlesource.com/18344
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
SSLECDHContext has the acronyms problem, so I went with SSLKeyShare to
match the TLS 1.3 terminology. It's also a little shorter. Accept and
Finish, for now, take raw output pointers in anticipation of some
bssl::Array and maybe bssl::CleansedArray types.
Bug: 132
Change-Id: I427c7c0eac95704f3ad093676c504c2848f5acb9
Reviewed-on: https://boringssl-review.googlesource.com/18265
Reviewed-by: Steven Valdez <svaldez@google.com>
I started by switching a couple fields to SSL_HANDSHAKE and then kept
following transitive bits.
Bug: 132
Change-Id: I640dadd3558615fa38c7e8498d4efe7449b0658f
Reviewed-on: https://boringssl-review.googlesource.com/18245
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
SSL_HANDSHAKE is large so I have not attempted to fully switch it to
scopers in this CL. This is just a preparatory step so that we can start
switching its fields to scopers.
(I also anticipate we'll want a bssl::Array<uint8_t> to replace the
pointer/length pairs.)
Bug: 132
Change-Id: I1538d3fc7f9c7385cd8c44a7b99b5c76e8a8768c
Reviewed-on: https://boringssl-review.googlesource.com/18244
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
The previous attempt around the 'struct ssl_st' compatibility mess
offended OSS-Fuzz and UBSan because one compilation unit passed a
function pointer with ssl_st* and another called it with
bssl::SSLConnection*.
Linkers don't retain such types, of course, but to silence this alert,
instead make C-visible types be separate from the implementation and
subclass the public type. This does mean we risk polluting the symbol
namespace, but hopefully the compiler is smart enough to inline the
visible struct's constructor and destructor.
Bug: 132
Change-Id: Ia75a89b3a22a202883ad671a630b72d0aeef680e
Reviewed-on: https://boringssl-review.googlesource.com/18224
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
This adds several utilities as replacements for new and delete and makes
bssl::UniquePtr work with our private types.
Later work can convert more incrementally. I did this one more
aggressively to see how it'd work. Unfortunately, in doing so, I needed
to remove the NULL SSL_AEAD_CTX "method" receiver trick to appease
clang. The null cipher is now represented by a concrete SSL_AEAD_CTX.
The long-lived references to SSL_AEAD_CTX are not yet in types with
constructors, so they still bare Delete rather than UniquePtr for now.
Though this does mean we may be able to move the sequence number into
SSLAEADContext later which is one less object for DTLS to carry around.
Bug: 132
Change-Id: I506b404addafb692055d5709b0ca6d5439a4e6be
Reviewed-on: https://boringssl-review.googlesource.com/18164
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This is horrible, but everything else I tried was worse. The goal with
this CL is to take the extern "C" out of ssl/internal.h and move most
symbols to namespace bssl, so we can start using C++ helpers and
destructors without worry.
Complications:
- Public API functions must be extern "C" and match their declaration in
ssl.h, which is unnamespaced. C++ really does not want you to
interleave namespaced and unnamespaced things. One can actually write
a namespaced extern "C" function, but this means, from C++'s
perspective, the function is namespaced. Trying to namespace the
public header would worked but ended up too deep a rabbithole.
- Our STACK_OF macros do not work right in namespaces.
- The typedefs for our exposed but opaque types are visible in the
header files and copied into consuming projects as forward
declarations. We ultimately want to give SSL a destructor, but
clobbering an unnamespaced ssl_st::~ssl_st seems bad manners.
- MSVC complains about ambiguous names if one typedefs SSL to bssl::SSL.
This CL opts for:
- ssl/*.cc must begin with #define BORINGSSL_INTERNAL_CXX_TYPES. This
informs the public headers to create forward declarations which are
compatible with our namespaces.
- For now, C++-defined type FOO ends up at bssl::FOO with a typedef
outside. Later I imagine we'll rename many of them.
- Internal functions get namespace bssl, so we stop worrying about
stomping the tls1_prf symbol. Exported C functions are stuck as they
are. Rather than try anything weird, bite the bullet and reorder files
which have a mix of public and private functions. I expect that over
time, the public functions will become fairly small as we move logic
to more idiomatic C++.
Files without any public C functions can just be written normally.
- To avoid MSVC troubles, some bssl types are renamed to CPlusPlusStyle
in advance of them being made idiomatic C++.
Bug: 132
Change-Id: Ic931895e117c38b14ff8d6e5a273e868796c7581
Reviewed-on: https://boringssl-review.googlesource.com/18124
Reviewed-by: David Benjamin <davidben@google.com>
This also serves as a certificate verification callback for
CRYPTO_BUFFER-based consumers. Remove the silly
SSL_CTX_i_promise_to_verify_certs_after_the_handshake placeholder.
Bug: 54, chromium:347402
Change-Id: I4c6b445cb9cd7204218acb2e5d1625e6f37aff6f
Reviewed-on: https://boringssl-review.googlesource.com/17964
Reviewed-by: David Benjamin <davidben@google.com>
In some configurations, Clang will warn about all unannotated
fall-throughs in C++. This change adds the needed annotation for Clang
in the single place where we appear to have this.
Change-Id: I25a9069e659ce278d3cd24bf46f667324b3d5146
Reviewed-on: https://boringssl-review.googlesource.com/18024
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This plumbs EVP_AEAD_CTX_seal_scatter all the way through to
tls_record.c, so we can add a new zero-copy record sealing method on top
of the existing code.
Change-Id: I01fdd88abef5442dc16605ea31b29b4b1231c073
Reviewed-on: https://boringssl-review.googlesource.com/17684
Reviewed-by: Adam Langley <agl@google.com>
ssl_cipher required fixing the types of the cipher masks.
Bug: 132
Change-Id: I0428d853b25fe4674ac3cad87a8eb92c6c8659e3
Reviewed-on: https://boringssl-review.googlesource.com/17746
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: I3de3c48a1de59c2b8de348253ce62a648aa6d6eb
Reviewed-on: https://boringssl-review.googlesource.com/17724
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
TLS 1.3 deployment is currently blocked by buggy middleboxes
throughout the ecosystem. As an experiment to better understand these bugs
and the problems they are causing, implement TLS 1.3 variants with
alternate encodings. These are still the same protocol, only encoded
slightly differently. We will use what we learn from these experiments to
guide the TLS 1.3 deployment strategy and proposals to the IETF, if any.
These experiments only target the basic 1-RTT TLS 1.3 handshake. Based on
what we learn from this experiment, we may try future variations to
explore 0-RTT and HelloRetryRequest.
When enabled, the server supports all TLS 1.3 variants while the client
is configured to use a particular variant.
Change-Id: I532411d1abc41314dc76acce0246879b754b4c61
Reviewed-on: https://boringssl-review.googlesource.com/17327
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for upcoming experiments which will require
supporting multiple experimental versions of TLS 1.3 with, on the
server, the ability to enable multiple variants at once. This means the
version <-> wire bijection no longer exists, even when limiting to a
single SSL*. Thus version_to_wire is removed and instead we treat the
wire version as the canonical version value.
There is a mapping from valid wire versions to protocol versions which
describe the high-level handshake protocol in use. This mapping is not
injective, so uses of version_from_wire are rewritten differently.
All the version-munging logic is moved to ssl_versions.c with a master
preference list of all TLS and DTLS versions. The legacy version
negotiation is converted to the new scheme. The version lists and
negotiation are driven by the preference lists and a
ssl_supports_version API.
To simplify the mess around SSL_SESSION and versions, version_from_wire
is now DTLS/TLS-agnostic, with any filtering being done by
ssl_supports_version. This is screwy but allows parsing SSL_SESSIONs to
sanity-check it and reject all bogus versions in SSL_SESSION. This
reduces a mess of error cases.
As part of this, the weird logic where ssl->version is set early when
sending the ClientHello is removed. The one place where we were relying
on this behavior is tweaked to query hs->max_version instead.
Change-Id: Ic91b348481ceba94d9ae06d6781187c11adc15b0
Reviewed-on: https://boringssl-review.googlesource.com/17524
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
