1.1.0 is not impacted by CVE-2019-1559, but this commit is a follow on
from that. That CVE was a result of applications calling SSL_shutdown
after a fatal alert has occurred. By chance 1.1.0 is not vulnerable to
that issue, but this change is additional hardening to prevent other
similar issues.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Thanks to GitHub user YuDudysheva for reporting this.
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from https://github.com/openssl/openssl/pull/6853)
We should validate that the various fields we put into the
CertificateRequest are not too long. Otherwise we will construct an
invalid message.
Fixes#6609
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6628)
If the remove_session_cb accesses the session's data (for instance,
via SSL_SESSION_get_protocol_version), a potential use after free
can occur. For this, consider the following scenario when adding
a new session via SSL_CTX_add_session:
- The session cache is full
(SSL_CTX_sess_number(ctx) > SSL_CTX_sess_get_cache_size(ctx))
- Only the session cache has a reference to ctx->session_cache_tail
(that is, ctx->session_cache_tail->references == 1)
Since the cache is full, remove_session_lock is called to remove
ctx->session_cache_tail from the cache. That is, it
SSL_SESSION_free()s the session, which free()s the data. Afterwards,
the free()d session is passed to the remove_session_cb. If the callback
accesses the session's data, we have a use after free.
The free before calling the callback behavior was introduced in
commit e4612d02c5 ("Remove sessions
from external cache, even if internal cache not used.").
CLA: trivial
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6222)
(cherry picked from commit c0a58e034d)
NULL-check for cipher is redundant, instead check if cipher->name is NULL
While here fix formatting of BIO_printf calls as suggested by Andy Polyakov.
CLA: trivial
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6282)
(cherry picked from commit d61e6040a0)
The TLS code marks records as read when its finished using a record. The DTLS code did
not do that. However SSL_has_pending() relies on it. So we should make DTLS consistent.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6160)
In DTLS if we have buffered a fragment for a zero length message (e.g.
ServerHelloDone) then, when we unbuffered the fragment, we were attempting
to memcpy the contents of the fragment which is zero length and a NULL
pointer. This is undefined behaviour. We should check first whether we
have a zero length fragment.
Fixes a travis issue.
[extended tests]
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6224)
During a full handshake the server is the last one to "speak". The timer
should continue to run until we know that the client has received our last
flight (e.g. because we receive some application data).
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6196)
The ciphers field in a session contains the stack of ciphers offered by
the client.
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6114)
The function SSL_get_shared_ciphers() is supposed to return ciphers shared
by the client and the server. However it only ever returned the client
ciphers.
Fixes#5317
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6114)
Commit d316cdcf6d introduced some extra
checks into the session-cache update procedure, intended to prevent
the caching of sessions whose resumption would lead to a handshake
failure, since if the server is authenticating the client, there needs to
be an application-set "session id context" to match up to the authentication
context. While that change is effective for its stated purpose, there
was also some collatoral damage introduced along with the fix -- clients
that set SSL_VERIFY_PEER are not expected to set an sid_ctx, and so
their usage of session caching was erroneously denied.
Fix the scope of the original commit by limiting it to only acting
when the SSL is a server SSL.
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5967)
(cherry picked from commit c4fa1f7fc0)
The MAX_CURVELIST macro defines the total number of in-built SSL/TLS curves
that we support. However it has not been updated as new curves are added.
Fixes#5232
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
(Merged from https://github.com/openssl/openssl/pull/6065)
(cherry picked from commit ca50cd911c)
In 1.0.2 and below we always send the same client_version in a reneg
ClientHello that we sent the first time around, regardless of what
version eventually gets negotiated. According to a comment in
statem_clnt.c this is a workaround for some buggy servers that choked if
we changed the version used in the RSA encrypted premaster secret.
In 1.1.0+ this behaviour no longer occurs. This restores the original
behaviour.
Fixes#1651
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6060)
In 1.1.0 and above we check the digest algorithm used to create signatures
in intermediate CA certs. If it is not sufficiently strong then we reject
the cert. To work out what digest was used we look at the OID for the
signature. This works for most signatures, but not for RSA PSS where the
digest is stored as parameter of the SignatureAlgorithmIdentifier. This
results in the digest look up routines failing and the cert being rejected.
PR #3301 added support for doing this properly in master. So in that
branch this all works as expected. It also works properly in 1.0.2 where we
don't have the digest checks at all. So the only branch where this fails is
1.1.0.
PR #3301 seems too significant to backport to 1.1.0. Instead we simply skip
the signature digest algorithm strength checks if we detect RSA PSS.
Fixes#3558.
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
(Merged from https://github.com/openssl/openssl/pull/6052)
In 1.0.2 you could configure automatic ecdh params by using the
ECDHParameters config directive and setting it to the value
"+Automatic" or just "Automatic". This is no longer required in 1.1.0+
but we still recognise the "+Automatic" keyword for backwards compatibility.
However we did not recognise just "Automatic" without the leading "+" which
is equally valid. This commit fixes that omission.
Fixes#4113
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6036)
DTLS was not correctly returning the number of pending bytes left in
a call to SSL_pending(). This makes the detection of truncated packets
almost impossible.
Fixes#5478
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6021)
We were sending illegal parameter. This isn't correct. The parameters are
legal, we just don't have an overlap. A more appropriate alert is
handshake failure.
Fixes#2919
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6010)
If SSL_set_bio() is called with a NULL wbio after a failed connection then
this can trigger an assertion failure. This should be valid behaviour and
the assertion is in fact invalid and can simply be removed.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5966)
(cherry picked from commit bd7775e14a)
We cannot provide a certificate status on a resumption so we should
ignore this extension in that case.
Fixes#1662
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/5897)
The GOST engine needs to be loaded before we initialise libssl. Otherwise
the GOST ciphersuites are not enabled. However the SSL conf module must
be loaded before we initialise libcrypto. Otherwise we will fail to read
the SSL config from a config file properly.
Another problem is that an application may make use of both libcrypto and
libssl. If it performs libcrypto stuff first and OPENSSL_init_crypto()
is called and loads a config file it will fail if that config file has
any libssl stuff in it.
This commit separates out the loading of the SSL conf module from the
interpretation of its contents. The loading piece doesn't know anything
about SSL so this can be moved to libcrypto. The interpretation of what it
means remains in libssl. This means we can load the SSL conf data before
libssl is there and interpret it when it later becomes available.
Fixes#5809
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5879)
If a server has been configured to use an ECDSA certificate, we should
allow it regardless of whether the server's own supported groups list
includes the certificate's group.
Fixes#2033
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
(Merged from https://github.com/openssl/openssl/pull/5607)
For DTLS/SCTP we were waiting for a dry event during the call to
tls_finish_handshake(). This function just tidies up various internal
things, and after it completes the handshake is over. I can find no good
reason for waiting for a dry event here, and nothing in RFC6083 suggests
to me that we should need to. More importantly though it seems to be
wrong. It is perfectly possible for a peer to send app data/alerts/new
handshake while we are still cleaning up our handshake. If this happens
then we will never get the dry event and so we cannot continue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5085)
At a couple of points in a DTLS/SCTP handshake we need to wait for a dry
event before continuing. However if an alert has been sent by the peer
then we will never receive that dry event and an infinite loop results.
This commit changes things so that we attempt to read a message if we
are waiting for a dry event but haven't got one yet. This should never
succeed, but any alerts will be processed.
Fixes#4763
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5085)
The sid_ctx is something of a "certificate request context" or a
"session ID context" -- something from the application that gives
extra indication of what sort of thing this session is/was for/from.
Without a sid_ctx, we only know that there is a session that we
issued, but it could have come from a number of things, especially
with an external (shared) session cache. Accordingly, when resuming,
we will hard-error the handshake when presented with a session with
zero-length sid_ctx and SSL_VERIFY_PEER is set -- we simply have no
information about the peer to verify, so the verification must fail.
In order to prevent these future handshake failures, proactively
decline to add the problematic sessions to the session cache.
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5175)
(cherry picked from commit d316cdcf6d)
This could in theory result in an overread - but due to the over allocation
of the underlying buffer does not represent a security issue.
Thanks to Fedor Indutny for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5415)
In <= TLSv1.2 a Finished message always comes immediately after a CCS
except in the case of NPN where there is an additional message between
the CCS and Finished. Historically we always calculated the Finished MAC
when we processed the CCS. However to deal with NPN we also calculated it
when we receive the Finished message. Really this should only have been
done if we hand negotiated NPN.
This simplifies the code to only calculate the MAC when we receive the
Finished. In 1.1.1 we need to do it this way anyway because there is no
CCS (except in middlebox compat mode) in TLSv1.3.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5286)
This is based on a heavily modified version of commit db0f35dda by Todd
Short from the master branch.
We are adding this because it used to be possible to disable reneg using
the flag SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS in 1.0.2. This is no longer
possible because of the opacity work.
A point to note about this is that if an application built against new
1.1.0 headers (that know about the new option SSL_OP_NO_RENEGOTIATION
option) is run using an older version of 1.1.0 (that doesn't know about
the option) then the option will be accepted but nothing will happen, i.e.
renegotiation will not be prevented. There's probably not much we can do
about that.
Fixes#4739
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4901)
In TLS we have a check to make sure an incoming reneg ClientHello is
acceptable. The equivalent check is missing in the DTLS code. This means
that if a client does not signal the ability to handle secure reneg in the
initial handshake, then a subsequent reneg handshake should be rejected by
the server. In the DTLS case the reneg was being allowed if the the 2nd
ClientHello had a renegotiation_info extension. This is incorrect.
While incorrect, this does not represent a security issue because if
the renegotiation_info extension is present in the second ClientHello it
also has to be *correct*. Therefore this will only work if both the client
and server believe they are renegotiating, and both know the previous
Finished result. This is not the case in an insecure rengotiation attack.
I have also tidied up the check in the TLS code and given a better check
for determining whether we are renegotiating or not.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5191)
The data argument of SSL_dane_tlsa_add is used read-only, so it
should be const.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5056)
(cherry picked from commit a41a6120cd)
In the case of a protocol version alert being sent by a peer the record
version number may not be what we are expecting. In DTLS records with an
unexpected version number are silently discarded. This probably isn't
appropriate for alerts, so we tolerate a mismatch in the minor version
number.
This resolves an issue reported on openssl-users where an OpenSSL server
chose DTLS1.0 but the client was DTLS1.2 only and sent a protocol_version
alert with a 1.2 record number. This was silently ignored by the server.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5018)
(cherry picked from commit 08455bc9b0)
This commit adds comments to bio_method_st definitions where the
function pointers are defined as NULL. Most of the structs have comments
but some where missing and not all consitent.
CLA: trivial
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4881)
(cherry picked from commit b4ff66223b)
SSL_trace() has a case which was inadvertently falling through.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4888)
