Sessions must be immutable once they can be shared with multiple threads.
We were breaking that rule by writing the ticket index into it during the
handshake. This can lead to incorrect behaviour, including failed
connections in multi-threaded environments.
Reported by David Benjamin.
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/8383)
(cherry picked from commit c96ce52ce2)
Prior to this commit we were keeping a count of how many KeyUpdates we
have processed and failing if we had had too many. This simplistic approach
is not sufficient for long running connections. Since many KeyUpdates
would not be a particular good DoS route anyway, the simplest solution is
to simply remove the key update count.
Fixes#8068
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
(Merged from https://github.com/openssl/openssl/pull/8299)
(cherry picked from commit 3409a5ff8a)
The original 1.1.1 design was to use SSL_CB_HANDSHAKE_START and
SSL_CB_HANDSHAKE_DONE to signal start/end of a post-handshake message
exchange in TLSv1.3. Unfortunately experience has shown that this confuses
some applications who mistake it for a TLSv1.2 renegotiation. This means
that KeyUpdate messages are not handled properly.
This commit removes the use of SSL_CB_HANDSHAKE_START and
SSL_CB_HANDSHAKE_DONE to signal the start/end of a post-handshake
message exchange. Individual post-handshake messages are still signalled in
the normal way.
This is a potentially breaking change if there are any applications already
written that expect to see these TLSv1.3 events. However, without it,
KeyUpdate is not currently usable for many applications.
Fixes#8069
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8096)
(cherry picked from commit 4af5836b55)
When computing the end-point shared secret, don't take the
terminating NULL character into account.
Please note that this fix breaks interoperability with older
versions of OpenSSL, which are not fixed.
Fixes#7956
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7957)
(cherry picked from commit 09d62b336d)
This commit erroneously kept the DTLS timer running after the end of the
handshake. This is not correct behaviour and shold be reverted.
This reverts commit f7506416b1.
Fixes#7998
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/8047)
(cherry picked from commit bcc1f3e2ba)
We were setting a limit of SSL3_RT_MAX_PLAIN_LENGTH on the size of the
ClientHello. AFAIK there is nothing in the standards that requires this
limit.
The limit goes all the way back to when support for extensions was first
added for TLSv1.0. It got converted into a WPACKET max size in 1.1.1. Most
likely it was originally added to avoid the complexity of having to grow
the init_buf in the middle of adding extensions. With WPACKET this is
irrelevant since it will grow automatically.
This issue came up when an attempt was made to send a very large
certificate_authorities extension in the ClientHello.
We should just remove the limit.
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7424)
(cherry picked from commit 7835e97b6f)
The cryptopro extension is supposed to be unsolicited and appears in the
ServerHello only. Additionally it is unofficial and unregistered - therefore
we should really treat it like any other unknown extension if we see it in
the ClientHello.
Fixes#7747
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/7984)
(cherry picked from commit 23fed8ba0e)
Fix some issues in tls13_hkdf_expand() which impact the above function
for TLSv1.3. In particular test that we can use the maximum label length
in TLSv1.3.
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7755)
(cherry picked from commit 0fb2815b87)
If compile OpenSSL with SSL_DEBUG macro, some test cases will cause the
process crashed in the debug code.
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/7707)
(cherry picked from commit 5a4481f0e0)
SSL(_CTX)?_set_client_CA_list() was a server side only function in 1.1.0.
If it was called on the client side then it was ignored. In 1.1.1 it now
makes sense to have a CA list defined for both client and server (the
client now sends it the the TLSv1.3 certificate_authorities extension).
Unfortunately some applications were using the same SSL_CTX for both
clients and servers and this resulted in some client ClientHellos being
excessively large due to the number of certificate authorities being sent.
This commit seperates out the CA list updated by
SSL(_CTX)?_set_client_CA_list() and the more generic
SSL(_CTX)?_set0_CA_list(). This means that SSL(_CTX)?_set_client_CA_list()
still has no effect on the client side. If both CA lists are set then
SSL(_CTX)?_set_client_CA_list() takes priority.
Fixes#7411
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7503)
(cherry picked from commit 9873297900)
TLSv1.3 is more restrictive about the curve used. There must be a matching
sig alg defined for that curve. Therefore if we are using some other curve
in our certificate then we should not negotiate TLSv1.3.
Fixes#7435
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7442)
(cherry picked from commit de4dc59802)
use_ecc() was always returning 1 because there are default (TLSv1.3)
ciphersuites that use ECC - even if those ciphersuites are disabled by
other options.
Fixes#7471
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
(Merged from https://github.com/openssl/openssl/pull/7479)
(cherry picked from commit 589b6227a8)
Commit 9ef9088c15 switched the SSL/SSL_CTX
statistics counters to using Thread-Sanitizer-friendly primitives.
However, it erroneously converted an addition of -1
(for s->session_ctx->stats.sess_accept) to an addition of +1, since that
is the only counter API provided by the internal tsan_assist.h header
until the previous commit. This means that for each accepted (initial)
connection, the session_ctx's counter would get doubly incremented, and the
(switched) ctx's counter would also get incremented.
Restore the counter decrement so that each accepted connection increments
exactly one counter exactly once (in net effect).
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/7464)
(cherry picked from commit 2aaa0b146b)
In TLSv1.2 and below a CertificateRequest is sent after the Certificate
from the server. This means that by the time the client_cert_cb is called
on receipt of the CertificateRequest a call to SSL_get_peer_certificate()
will return the server certificate as expected. In TLSv1.3 a
CertificateRequest is sent before a Certificate message so calling
SSL_get_peer_certificate() returns NULL.
To workaround this we delay calling the client_cert_cb until after we
have processed the CertificateVerify message, when we are doing TLSv1.3.
Fixes#7384
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/7413)
(cherry picked from commit e45620140f)
PR #3783 introduce coded to reset the server side SNI state in
SSL_do_handshake() to ensure any erroneous config time SNI changes are
cleared. Unfortunately SSL_do_handshake() can be called mid-handshake
multiple times so this is the wrong place to do this and can mean that
any SNI data is cleared later on in the handshake too.
Therefore move the code to a more appropriate place.
Fixes#7014
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/7149)
Commit 1c4aa31d79 modified the state machine
to clean up stale ext.hostname values from SSL objects in the case when
SNI was not negotiated for the current handshake. This is natural from
the TLS perspective, since this information is an extension that the client
offered but we ignored, and since we ignored it we do not need to keep it
around for anything else.
However, as documented in https://github.com/openssl/openssl/issues/7014 ,
there appear to be some deployed code that relies on retrieving such an
ignored SNI value from the client, after the handshake has completed.
Because the 1.1.1 release is on a stable branch and should preserve the
published ABI, restore the historical behavior by retaining the ext.hostname
value sent by the client, in the SSL structure, for subsequent retrieval.
[extended tests]
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7115)
The is_tls13_capable() function should not return 0 if no certificates
are configured directly because a certificate callback is present.
Fixes#7140
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/7141)
If we've sent a close_notify then we are restricted about what we can do
in response to handshake messages that we receive. However we can sensibly
process NewSessionTicket messages. We can also process a KeyUpdate message
as long as we also ignore any request for us to update our sending keys.
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7114)
Treat a connection using an external PSK like we would a resumption and
send a single NewSessionTicket afterwards.
Fixes#6941
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7097)
If a client sends data to a server and then immediately closes without
waiting to read the NewSessionTickets then the server can receive EPIPE
when trying to write the tickets and never gets the opportunity to read
the data that was sent. Therefore we ignore EPIPE when writing out the
tickets in TLSv1.3
Fixes#6904
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6944)
We need to ensure that the min-max version range we use when constructing
the ClientHello is the same range we use when we validate the version
selected by the ServerHello. Otherwise this may appear as a fallback or
downgrade.
Fixes#6964
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7013)
Having post handshake auth automatically switched on breaks some
applications written for TLSv1.2. This changes things so that an explicit
function call is required for a client to indicate support for
post-handshake auth.
Fixes#6933.
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6938)
A client that has fallen back could detect an inappropriate fallback if
the TLSv1.3 downgrade protection sentinels are present.
Fixes#6756
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6894)
At certain points in the handshake we could receive either a plaintext or
an encrypted alert from the client. We should tolerate both where
appropriate.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6887)
If we sent early_data and then received back an HRR, the enc_write_ctx
was stale resulting in errors if an alert needed to be sent.
Thanks to Quarkslab for reporting this.
In any case it makes little sense to encrypt alerts using the
client_early_traffic_secret, so we add special handling for alerts sent
after early_data. All such alerts are sent in plaintext.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6887)
Under certain error conditions a call to SSLfatal could accidently be
missed.
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Andy Polyakov <appro@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6872)
In some scenarios the connection could fail without an alert being sent.
This causes a later assertion failure.
Thanks to Quarkslab for reporting this.
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/6852)
In particular, adhere to the rule that we must not modify any
property of an SSL_SESSION object once it is (or might be) in
a session cache. Such modifications are thread-unsafe and have
been observed to cause crashes at runtime.
To effect this change, standardize on the property that
SSL_SESSION->ext.hostname is set only when that SNI value
has been negotiated by both parties for use with that session.
For session resumption this is trivially the case, so only new
handshakes are affected.
On the client, the new semantics are that the SSL->ext.hostname is
for storing the value configured by the caller, and this value is
used when constructing the ClientHello. On the server, SSL->ext.hostname
is used to hold the value received from the client. Only if the
SNI negotiation is successful will the hostname be stored into the
session object; the server can do this after it sends the ServerHello,
and the client after it has received and processed the ServerHello.
This obviates the need to remove the hostname from the session object
in case of failed negotiation (a change that was introduced in commit
9fb6cb810b in order to allow TLS 1.3
early data when SNI was present in the ClientHello but not the session
being resumed), which was modifying cached sessions in certain cases.
(In TLS 1.3 we always produce a new SSL_SESSION object for new
connections, even in the case of resumption, so no TLS 1.3 handshakes
were affected.)
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6378)
These tiny functions only read from the input SSL, and we are
about to use them from functions that only have a const SSL* available,
so propagate const a bit further.
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6378)
The spec says that a client MUST set legacy_version to TLSv1.2, and
requires servers to verify that it isn't SSLv3.
Fixes#6600
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6747)
If we issue new tickets due to post-handshake authentication there is no
reason to remove previous tickets from the cache. The code that did that
only removed the last session anyway - so if more than one ticket got
issued then those other tickets are still valid.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6722)
Check that we are either configured for PSK, or that we have a TLSv1.3
capable certificate type. DSA certs can't be used in TLSv1.3 and we
don't (currently) allow GOST ones either (owing to the lack of standard
sig algs).
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6650)
