Commit 9ceb2426b0 (PACKETise ClientHello) broke session tickets by failing
to detect the session ticket extension in an incoming ClientHello. This
commit fixes the bug.
Reviewed-by: Emilia Käsper <emilia@openssl.org>
Enhance the PACKET code readability, and fix a stale comment. Thanks
to Ben Kaduk (bkaduk@akamai.com) for pointing this out.
Reviewed-by: Emilia Käsper <emilia@openssl.org>
This was obsolete in 2001. This is not the same as Gost94 digest.
Thanks to Dmitry Belyavsky <beldmit@gmail.com> for review and advice.
Reviewed-by: Matt Caswell <matt@openssl.org>
When config'd with "sctp" running "make test" causes a seg fault. This is
actually due to the way ssltest works - it dives under the covers and frees
up BIOs manually and so some BIOs are NULL when the SCTP code does not
expect it. The simplest fix is just to add some sanity checks to make sure
the BIOs aren't NULL before we use them.
This problem occurs in master and 1.0.2. The fix has also been applied to
1.0.1 to keep the code in sync.
Reviewed-by: Tim Hudson <tjh@openssl.org>
There are some missing return value checks in the SCTP code. In master this
was causing a compilation failure when config'd with
"--strict-warnings sctp".
Reviewed-by: Tim Hudson <tjh@openssl.org>
If a client receives a ServerKeyExchange for an anon DH ciphersuite with the
value of p set to 0 then a seg fault can occur. This commits adds a test to
reject p, g and pub key parameters that have a 0 value (in accordance with
RFC 5246)
The security vulnerability only affects master and 1.0.2, but the fix is
additionally applied to 1.0.1 for additional confidence.
CVE-2015-1794
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
make errors wants things in a different order to the way things are
currently defined in the header files. The easiest fix is to just let it
reorder it.
Reviewed-by: Richard Levitte <levitte@openssl.org>
--strict-warnings started showing warnings for this today...
Surely an error should be raised if these reads fail?
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Some of the PACKET functions were returning incorrect data. An unfortunate
choice of test data in the unit test was masking the failure.
Reviewed-by: Tim Hudson <tjh@openssl.org>
The move of CCS into the state machine introduced a bug in ssl3_read_bytes.
The value of |recvd_type| was not being set if we are satisfying the request
from handshake fragment storage. This can occur, for example, with
renegotiation and causes the handshake to fail.
Reviewed-by: Tim Hudson <tjh@openssl.org>
Continuing on from the previous commit this moves the processing of DTLS
CCS messages out of the record layer and into the state machine.
Reviewed-by: Tim Hudson <tjh@openssl.org>
The handling of incoming CCS records is a little strange. Since CCS is not
a handshake message it is handled differently to normal handshake messages.
Unfortunately whilst technically it is not a handhshake message the reality
is that it must be processed in accordance with the state of the handshake.
Currently CCS records are processed entirely within the record layer. In
order to ensure that it is handled in accordance with the handshake state
a flag is used to indicate that it is an acceptable time to receive a CCS.
Previously this flag did not exist (see CVE-2014-0224), but the flag should
only really be considered a workaround for the problem that CCS is not
visible to the state machine.
Outgoing CCS messages are already handled within the state machine.
This patch makes CCS visible to the TLS state machine. A separate commit
will handle DTLS.
Reviewed-by: Tim Hudson <tjh@openssl.org>
Provide more robust (inline) functions to replace n2s, n2l, etc. These
functions do the same thing as the previous macros, but also keep track
of the amount of data remaining and return an error if we try to read more
data than we've got.
Reviewed-by: Tim Hudson <tjh@openssl.org>
Commit e481f9b90b removed OPENSSL_NO_TLSEXT from the code.
Previously if OPENSSL_NO_TLSEXT *was not* defined then the server random was
filled during getting of the ClientHello. If it *was* defined then the
server random would be filled in ssl3_send_server_hello(). Unfortunately in
commit e481f9b90b the OPENSSL_NO_TLSEXT guards were removed but *both*
server random fillings were left in. This could cause problems for session
ticket callbacks.
Reviewed-by: Stephen Henson <steve@openssl.org>
Move PSK premaster secret algorithm to ssl_generate_master secret so
existing key exchange code can be used and modified slightly to add
the PSK wrapping structure.
Reviewed-by: Matt Caswell <matt@openssl.org>
Add support for RSAPSK, DHEPSK and ECDHEPSK server side.
Update various checks to ensure certificate and server key exchange messages
are only sent when required.
Update message handling. PSK server key exchange parsing now include an
identity hint prefix for all PSK server key exchange messages. PSK
client key exchange message expects PSK identity and requests key for
all PSK key exchange ciphersuites.
Update flags for RSA, DH and ECDH so they are also used in PSK.
Reviewed-by: Matt Caswell <matt@openssl.org>
Add support for RSAPSK, DHEPSK and ECDHEPSK client side.
Update various checks to ensure certificate and server key exchange messages
are only expected when required.
Update message handling. PSK server key exchange parsing now expects an
identity hint prefix for all PSK server key exchange messages. PSK
client key exchange message requests PSK identity and key for all PSK
key exchange ciphersuites and includes identity in message.
Update flags for RSA, DH and ECDH so they are also used in PSK.
Reviewed-by: Matt Caswell <matt@openssl.org>
The DTLS code is supposed to drop packets if we try to write them out but
the underlying BIO write buffers are full. ssl3_write_pending() contains
an incorrect test for DTLS that controls this. The test only checks for
DTLS1 so DTLS1.2 does not correctly clear the internal OpenSSL buffer which
can later cause an assert to be hit. This commit changes the test to cover
all DTLS versions.
RT#3967
Reviewed-by: Tim Hudson <tjh@openssl.org>
This flag was not set anywhere within the codebase (only read). It could
only be set by an app reaching directly into s->s3->flags and setting it
directly. However that method became impossible when libssl was opaquified.
Even in 1.0.2/1.0.1 if an app set the flag directly it is only relevant to
ssl3_connect(), which calls SSL_clear() during initialisation that clears
any flag settings. Therefore it could take effect if the app set the flag
after the handshake has started but before it completed. It seems quite
unlikely that any apps really do this (especially as it is completely
undocumented).
The purpose of the flag is suppress flushing of the write bio on the client
side at the end of the handshake after the client has written the Finished
message whilst resuming a session. This enables the client to send
application data as part of the same flight as the Finished message.
This flag also controls the setting of a second flag SSL3_FLAGS_POP_BUFFER.
There is an interesting comment in the code about this second flag in the
implementation of ssl3_write:
/* This is an experimental flag that sends the
* last handshake message in the same packet as the first
* use data - used to see if it helps the TCP protocol during
* session-id reuse */
It seems the experiment did not work because as far as I can tell nothing
is using this code. The above comment has been in the code since SSLeay.
This commit removes support for SSL3_FLAGS_DELAY_CLIENT_FINISHED, as well
as the associated SSL3_FLAGS_POP_BUFFER.
Reviewed-by: Rich Salz <rsalz@openssl.org>
Add support for loading verify and chain stores in SSL_CONF.
Commands to set verify mode and client CA names.
Add documentation.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
The PSK identity hint should be stored in the SSL_SESSION structure
and not in the parent context (which will overwrite values used
by other SSL structures with the same SSL_CTX).
Reviewed-by: Matt Caswell <matt@openssl.org>
We always free the handshake buffer when digests are freed so move
it into ssl_free_digest_list()
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Rewrite ssl3_digest_cached_records handling. Only digest cached records
if digest array is NULL: this means it is safe to call
ssl3_digest_cached_records multiple times (subsequent calls are no op).
Remove flag TLS1_FLAGS_KEEP_HANDSHAKE instead only update handshake buffer
if digest array is NULL.
Add additional "keep" parameter to ssl3_digest_cached_records to indicate
if the handshake buffer should be retained after digesting cached records
(needed for TLS 1.2 client authentication).
Reviewed-by: Matt Caswell <matt@openssl.org>
If RSA or DSA is disabled we will never use a ciphersuite with
RSA/DSA authentication as it is already filtered out by the cipher
list logic.
Reviewed-by: Richard Levitte <levitte@openssl.org>
As numerous comments indicate the certificate and key array is not an
appopriate structure to store the peers certificate: so remove it and
just the s->session->peer instead.
Reviewed-by: Richard Levitte <levitte@openssl.org>
This reverts commit d480e182fe.
Commit broke TLS handshakes due to fragility of digest caching: that will be
fixed separately.
Reviewed-by: Rich Salz <rsalz@openssl.org>
While closing RT3588 (Remove obsolete comment) Kurt and I saw that a
few lines to completely clear the SSL cipher state could be moved into
a common function.
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
It is valid for an extension block to be present in a ClientHello, but to
be of zero length.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Fix error handling in ssl_session_dup, as well as incorrect setting up of
the session ticket. Follow on from CVE-2015-1791.
Thanks to LibreSSL project for reporting these issues.
Reviewed-by: Tim Hudson <tjh@openssl.org>
This is a workaround so old that nobody remembers what buggy clients
it was for. It's also been broken in stable branches for two years and
nobody noticed (see
https://boringssl-review.googlesource.com/#/c/1694/).
Reviewed-by: Tim Hudson <tjh@openssl.org>
It should not be possible for DTLS message fragments to span multiple
packets. However previously if the message header fitted exactly into one
packet, and the fragment body was in the next packet then this would work.
Obviously this would fail if packets get re-ordered mid-flight.
Reviewed-by: Tim Hudson <tjh@openssl.org>
The underlying field returned by RECORD_LAYER_get_rrec_length() is an
unsigned int. The return type of the function should match that.
Reviewed-by: Tim Hudson <tjh@openssl.org>
This adds additional checks to the processing of extensions in a ClientHello
to ensure that either no extensions are present, or if they are then they
take up the exact amount of space expected.
With thanks to the Open Crypto Audit Project for reporting this issue.
Reviewed-by: Stephen Henson <steve@openssl.org>
Remove a comment that suggested further clean up was required.
DH_free() performs the necessary cleanup.
With thanks to the Open Crypto Audit Project for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
Ensure OPENSSL_cleanse() is called on the premaster secret value calculated for GOST.
With thanks to the Open Crypto Audit Project for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
The session object on the client side is initially created during
construction of the ClientHello. If the client is DTLS1.2 capable then it
will store 1.2 as the version for the session. However if the server is only
DTLS1.0 capable then when the ServerHello comes back the client switches to
using DTLS1.0 from then on. However the session version does not get
updated. Therefore when the client attempts to resume that session the
server throws an alert because of an incorrect protocol version.
Reviewed-by: Tim Hudson <tjh@openssl.org>
If a NewSessionTicket is received by a multi-threaded client when
attempting to reuse a previous ticket then a race condition can occur
potentially leading to a double free of the ticket data.
CVE-2015-1791
This also fixes RT#3808 where a session ID is changed for a session already
in the client session cache. Since the session ID is the key to the cache
this breaks the cache access.
Parts of this patch were inspired by this Akamai change:
c0bf69a791
Reviewed-by: Rich Salz <rsalz@openssl.org>
dtls1_get_message has an |mt| variable which is the type of the message that
is being requested. If it is negative then any message type is allowed.
However the value of |mt| is not checked in one of the main code paths, so a
peer can send a message of a completely different type and it will be
processed as if it was the message type that we were expecting. This has
very little practical consequences because the current behaviour will still
fail when the format of the message isn't as expected.
Reviewed-by: Andy Polyakov <appro@openssl.org>
Where we called openssl_cleanse, make sure we do it on all error
paths. Be consistent in use of sizeof(foo) when possible.
Reviewed-by: Andy Polyakov <appro@openssl.org>
The new accessors SSL_get_client_random, SSL_get_server_random and
SSL_SESSION_get_master_key should return a size_t to match the type of the
|outlen| parameter.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Change the new SSL_get_client_random(), SSL_get_server_random() and
SSL_SESSION_get_master_key() functions to use size_t for |outlen| instead of
int.
Reviewed-by: Tim Hudson <tjh@openssl.org>
Tor uses these values to implement a low-rent clone of RFC 5705 (which,
in our defense, we came up with before RFC 5705 existed). But now that
ssl_st is opaque, we need another way to get at them.
Includes documentation, with suitable warnings about not actually
using these functions.
Signed-off-by: Nick Mathewson <nickm@torproject.org>
Signed-off-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tim Hudson <tjh@openssl.org>
On the server side, if you want to know which ciphers the client
offered, you had to use session->ciphers. But that field is no
longer visible, so we need a method to get at it.
Signed-off-by: Nick Mathewson <nickm@torproject.org>
Signed-off-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tim Hudson <tjh@openssl.org>
The size of the SRP extension can never be negative (the variable
|size| is unsigned). Therefore don't check if it is less than zero.
RT#3862
Reviewed-by: Richard Levitte <levitte@openssl.org>
If the record received is for a version that we don't support, previously we
were sending an alert back. However if the incoming record already looks
like an alert then probably we shouldn't do that. So suppress an outgoing
alert if it looks like we've got one incoming.
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Version negotiation was broken (one of the late changes in the review
process broke it). The problem is that TLS clients do not set first_packet,
whereas TLS/DTLS servers and DTLS clients do. The simple fix is to set
first_packet for TLS clients too.
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
