The server-side ChangeCipherState processing stores the new cipher
in the SSL_SESSION object, so that the new state can be used if
this session gets resumed. However, writing to the session is only
thread-safe for initial handshakes, as at other times the session
object may be in a shared cache and in use by another thread at the
same time. Reflect this invariant in the code by only writing to
s->session->cipher when it is currently NULL (we do not cache sessions
with no cipher). The code prior to this change would never actually
change the (non-NULL) cipher value in a session object, since our
server enforces that (pre-TLS-1.3) resumptions use the exact same
cipher as the initial connection, and non-abbreviated renegotiations
have produced a new session object before we get to this point.
Regardless, include logic to detect such a condition and abort the
handshake if it occurs, to avoid any risk of inadvertently using
the wrong cipher on a connection.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)
(cherry picked from commit 2e3ec2e1578977fca830a47fd7f521e290540e6d)
TLS 1.3 maintains a separate keys chedule in the SSL object, but
was writing to the 'master_key_length' field in the SSL_SESSION
when generating the per-SSL master_secret. (The generate_master_secret
SSL3_ENC_METHOD function needs an output variable for the master secret
length, but the TLS 1.3 implementation just uses the output size of
the handshake hash function to get the lengths, so the only natural-looking
thing to use as the output length was the field in the session.
This would potentially involve writing to a SSL_SESSION object that was
in the cache (i.e., resumed) and shared with other threads, though.
The thread-safety impact should be minimal, since TLS 1.3 requires the
hash from the original handshake to be associated with the resumption
PSK and used for the subsequent connection. This means that (in the
resumption case) the value being written would be the same value that was
previously there, so the only risk would be on architectures that can
produce torn writes/reads for aligned size_t values.
Since the value is essentially ignored anyway, just provide the
address of a local dummy variable to generate_master_secret() instead.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)
(cherry picked from commit d74014c4b8740f28a54b562f799ad1e754b517b9)
Use a space after a comma.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)
(cherry picked from commit 1866a0d380fc361d9be2ca0509de0f2281505db5)
If we hit an EOF while reading in libssl then we will report an error
back to the application (SSL_ERROR_SYSCALL) but errno will be 0. We add
an error to the stack (which means we instead return SSL_ERROR_SSL) and
therefore give a hint as to what went wrong.
Contains a partial fix for #10880
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/10882)
Allow for encryption overhead in early DTLS size check
and send overflow if validated record is too long
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/11096)
(cherry picked from commit cc0663f697b05ed121a728241f0502250429802d)
Signature algorithms not using an MD weren't checked that they're
allowed by the security level.
Reviewed-by: Tomáš Mráz <tmraz@fedoraproject.org>
GH: #11062
If the servername cb decides to send back a warning alert then the
handshake continues, but we should not signal to the client that the
servername has been accepted.
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/10018)
(cherry picked from commit cd624ccd41ac3ac779c1c7a7a1e63427ce9588dd)
The SNI behaviour for TLSv1.3 and the behaviour of SSL_get_servername()
was not quite right, and not entirely consistent with the RFC.
The TLSv1.3 RFC explicitly says that SNI is negotiated on each handshake
and the server is not required to associate it with the session. This was
not quite reflected in the code so we fix that.
Additionally there were some additional checks around early_data checking
that the SNI between the original session and this session were
consistent. In fact the RFC does not require any such checks, so they are
removed.
Finally the behaviour of SSL_get_servername() was not quite right. The
behaviour was not consistent between resumption and normal handshakes,
and also not quite consistent with historical behaviour. We clarify the
behaviour in various scenarios and also attempt to make it match historical
behaviour as closely as possible.
Fixes#8822
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/10018)
(cherry picked from commit 7955c1f16e72dc944677fd1dbf4b1300e75f1c84)
TLS < 1.2 has fixed signature algorithms: MD5+SHA1 for RSA and SHA1 for the
others. TLS 1.2 sends a list of supported ciphers, but allows not sending
it in which case SHA1 is used. TLS 1.3 makes sending the list mandatory.
When we didn't receive a list from the client, we always used the
defaults without checking that they are allowed by the configuration.
Reviewed-by: Paul Dale <paul.dale@oracle.com>
GH: #10784
(cherry picked from commit b0031e5dc2c8c99a6c04bc7625aa00d3d20a59a5)
Fix double + in hkdflabel declaration (FIXES#10675)
CLA: trivial
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from https://github.com/openssl/openssl/pull/10700)
(cherry picked from commit 2de5a5fbdd14f514e962cccfe90482c37786c183)
Large GOST ClientKeyExchange messages are sent by VipNet CSP, one of
Russian certified products implementing GOST TLS, when a server
certificate contains 512-bit keys.
This behaviour was present in 1.0.2 branch and needs to be restored.
Backport of #10376
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/10376)
As was done for ciphers, supported groups, and EC point formats in
https://github.com/openssl/openssl/pull/9162, only write the negotiated
SNI hostname value to the session object when not resuming, even for
TLS 1.3 resumptions. Otherwise, when using a stateful session cache
(as is done by default when 0-RTT data is enabled), we can have multiple
SSLs active using the same in-memory session object, which leads to
double-frees and similar race conditions in the SNI handler prior
to this commit.
Fortunately, since draft-ietf-tls-tls13-22, there is no requirement
that the SNI hostname be preserved across TLS 1.3 resumption, and thus
not a need to continually update the session object with the "current"
value (to be used when producing session tickets, so that the subsequent
resumption can be checked against the current value). So we can just
relax the logic and only write to the session object for initial handshakes.
This still leaves us in a somewhat inconsistent state, since if the SNI value
does change across handshakes, the session object will continue to record
the initial handshake's value, even if that bears no relation to the
current handshake. The current SSL_get_servername() implementation
prefers the value from the session if s->hit, but a more complete fix
for that and related issues is underway in
https://github.com/openssl/openssl/pull/10018; there is no need to wait
for the complete fix for SNI name handling in order to close the
race condition and avoid runtime crashes.
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/10441)
(cherry picked from commit 2a5385511051d33be8d2b20d7669d8b1862fe510)
This also removes the incorrect documentation comments by those
functions.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10403)
The resumption_label variable when CHARSET_EBCDIC was enabled, was misspelled.
Instead of evaluating to 'res binder' as expected, it evaluated to 'red binder'.
CLA: trivial
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from https://github.com/openssl/openssl/pull/10396)
(cherry picked from commit 6ed12cec7216c3e81b58f5cafa41775e456feaee)
Many Windows-based GOST TLS implementations are unable to extend the
list of supported SignatureAlgorithms because of lack of the necessary
callback in Windows. So for TLS 1.2 it makes sense to imply the support
of GOST algorithms in case when the GOST ciphersuites are present.
This is a backport of #10377 to 1.1.1 branch
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/10378)
Fixes#9869
CLA:trivial
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9878)
The decryption failed alert was deprecated a long time ago. It can
provide an attacker too much information to be able to distinguish between
MAC failures and decryption failures and can lead to oracle attacks.
Instead we should always use the bad_record_mac alert for these issues.
This fixes one instance that still exists. It does not represent a
security issue in this case because it is only ever sent if the record is
publicly invalid, i.e. we have detected it is invalid without using any
secret material.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10093)
(cherry picked from commit 37133290832ac2d1389926eba7325125fdacbe8d)
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from https://github.com/openssl/openssl/pull/9653)
(cherry picked from commit e7c27a6c3716843f8412fd96311b70ac84b785f9)
According to RFC8446 CertificateEntry in Certificate message contains
extensions that were not present in the Certificate message in RFC5246.
CLA: trivial
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/9994)
(cherry picked from commit 65c76cd2c9e8da9468dd490b334e56c51dbef582)
Make the include guards consistent by renaming them systematically according
to the naming conventions below
The public header files (in the 'include/openssl' directory) are not changed
in 1.1.1, because it is a stable release.
For the private header files files, the guard names try to match the path
specified in the include directives, with all letters converted to upper case
and '/' and '.' replaced by '_'. An extra 'OSSL_' is added as prefix.
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9681)
Apart from public and internal header files, there is a third type called
local header files, which are located next to source files in the source
directory. Currently, they have different suffixes like
'*_lcl.h', '*_local.h', or '*_int.h'
This commit changes the different suffixes to '*_local.h' uniformly.
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9681)
If a TLSv1.3 server configured to respond to the status_request extension
also attempted to send a CertificateRequest then it was incorrectly
inserting a non zero length status_request extension into that message.
The TLSv1.3 RFC does allow that extension in that message but it must
always be zero length.
In fact we should not be sending the extension at all in that message
because we don't support it.
Fixes#9767
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9780)
(cherry picked from commit debb64a0ca43969eb3f043aa8895a4faa7f12b6e)
In commit 6aca8d1a5 ("Honour mandatory digest on private key in
has_usable_cert()") I added two checks for the capabilities of the
EVP_PKEY being used. One of them was wrong, as it should only be
checking the signature of the X.509 cert (by its issuer) against the
sigalgs given in a TLS v1.3 signature_algorithms_cert extension.
Remove it.
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9705)
The function SSL_check_chain() can be used by applications to check that
a cert and chain is compatible with the negotiated parameters. This could
be useful (for example) from the certificate callback. Unfortunately this
function was applying TLSv1.2 sig algs rules and did not work correctly if
TLSv1.3 was negotiated.
We refactor tls_choose_sigalg to split it up and create a new function
find_sig_alg which can (optionally) take a certificate and key as
parameters and find an appropriate sig alg if one exists. If the cert and
key are not supplied then we try to find a cert and key from the ones we
have available that matches the shared sig algs.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9443)
Fix a few places where calling ossl_isdigit does the wrong thing on
EBCDIC based systems.
Replaced with ascii_isdigit.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9556)
(cherry picked from commit 48102247ff)
At some point in the past do_ssl3_write() used to return the number of
bytes written, or a value <= 0 on error. It now just returns a success/
error code and writes the number of bytes written to |tmpwrit|.
The SSL_MODE_RELEASE_BUFFERS code was still looking at the return code
for the number of bytes written rather than |tmpwrit|. This has the effect
that the buffers are not released when they are supposed to be.
Fixes#9490
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/9505)
(cherry picked from commit 8bbf63e48f)
tls_parse_stoc_key_share was generating a new EVP_PKEY public/private
keypair and then overrides it with the server public key, so the
generation was a waste anyway. Instead, it should create a
parameters-only EVP_PKEY.
(This is a consequence of OpenSSL using the same type for empty key,
empty key with key type, empty key with key type + parameters, public
key, and private key. As a result, it's easy to mistakenly mix such
things up, as happened here.)
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
(Merged from https://github.com/openssl/openssl/pull/9445)
(cherry picked from commit 166c0b98fd)
Also, use define rather than sizeof
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9377)
(cherry picked from commit fe9edc9d39)
It was only ever in cert_st because ssl_st was a public structure
and could not be modified without breaking the API. However, both
structures are now opaque, and thus we can freely change their layout
without breaking applications. In this case, keeping the shared
sigalgs in the SSL object prevents complications wherein they would
inadvertently get cleared during SSL_set_SSL_CTX() (e.g., as run
during a cert_cb).
Fixes#9099
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9157)
(cherry picked from commit 29948ac80c)
This reverts commit 524006dd1b.
While this change did prevent the sigalgs from getting inadvertently
clobbered by SSL_set_SSL_CTX(), it also caused the sigalgs to not be
set when the cert_cb runs. This, in turn, caused significant breakage,
such as SSL_check_chain() failing to find any valid chain. An alternate
approach to fixing the issue from #7244 will follow.
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9157)
(cherry picked from commit 6f34d7bc7d)
The previous 2 commits moved supported groups and ciphers out of the
session object to avoid race conditions. We now also move ecpointformats
for consistency. There does not seem to be a race condition with access
to this data since it is only ever set in a non-resumption handshake.
However, there is no reason for it to be in the session.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9176)
Similarly to the previous commit we were storing the peer offered list
of ciphers in the session. In practice there is no need for this
information to be avilable from one resumption to the next since this
list is specific to a particular handshake. Since the session object is
supposed to be immutable we should not be updating it once we have decided
to resume. The solution is to remove the session list out of the session
object.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9176)
In TLSv1.3 the supported groups can be negotiated each time a handshake
occurs, regardless of whether we are resuming or not. We should not store
the supported groups information in the session because session objects
can be shared between multiple threads and we can end up with race
conditions. For most users this won't be seen because, by default, we
use stateless tickets in TLSv1.3 which don't get shared. However if you
use SSL_OP_NO_TICKET (to get stateful tickets in TLSv1.3) then this can
happen.
The answer is to move the supported the supported group information into
the SSL object instead.
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9176)
If we receive a KeyUpdate message (update requested) from the peer while
we are in the middle of a write, we should defer sending the responding
KeyUpdate message until after the current write is complete. We do this
by waiting to send the KeyUpdate until the next time we write and there is
no pending write data.
This does imply a subtle change in behaviour. Firstly the responding
KeyUpdate message won't be sent straight away as it is now. Secondly if
the peer sends multiple KeyUpdates without us doing any writing then we
will only send one response, as opposed to previously where we sent a
response for each KeyUpdate received.
Fixes#8677
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/8773)
(cherry picked from commit feb9e31c40)
This function only returns a status and does not modify the parameter.
Since similar function are already taking const parameters, also
change this function to have a const parameter.
Fixes#8934
CLA: trivial
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/8945)
(cherry picked from commit c04b66b18d)
Reviewed-by: Paul Yang <yang.yang@baishancloud.com>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8756)
(cherry picked from commit 6fda11ae5a)
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)
