Using RSA_PKCS1_PADDING with RSA_private_decrypt is inherently unsafe.
The API requires writing output on success and touching the error queue
on error. Thus, although the padding check itself is constant-time as of
294d1e36c2, and the logic after the
decryption in the SSL code is constant-time as of
adb46dbc6d, the API boundary in the middle
still leaks whether the padding check succeeded, giving us our
much-loved Bleichenbacher padding oracle.
Instead, PKCS#1 padding must be handled by the caller which uses
RSA_NO_PADDING, in timing-sensitive code integrated with the
Bleichenbacher mitigation. Removing PKCS#1 padding in constant time is
actually much simpler when the expected length is a constant (and if
it's not a constant, avoiding a padding oracle seems unlikely), so just
do it inline.
Signed-off-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Rich Salz <rsalz@openssl.org>
GH: #1222
When session tickets are used, it's possible that SNI might swtich the
SSL_CTX on an SSL. Normally, this is not a problem, because the
initial_ctx/session_ctx are used for all session ticket/id processes.
However, when the SNI callback occurs, it's possible that the callback
may update the options in the SSL from the SSL_CTX, and this could
cause SSL_OP_NO_TICKET to be set. If this occurs, then two bad things
can happen:
1. The session ticket TLSEXT may not be written when the ticket expected
flag is set. The state machine transistions to writing the ticket, and
the client responds with an error as its not expecting a ticket.
2. When creating the session ticket, if the ticket key cb returns 0
the crypto/hmac contexts are not initialized, and the code crashes when
trying to encrypt the session ticket.
To fix 1, if the ticket TLSEXT is not written out, clear the expected
ticket flag.
To fix 2, consider a return of 0 from the ticket key cb a recoverable
error, and write a 0 length ticket and continue. The client-side code
can explicitly handle this case.
Fix these two cases, and add unit test code to validate ticket behavior.
Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1098)
The ssl3_init_finished_mac() function can fail, in which case we need to
propagate the error up through the stack.
RT#3198
Reviewed-by: Rich Salz <rsalz@openssl.org>
In the new state machine if using nbio and we get the header of a
handshake message is one record with the body in the next, with an nbio
event in the middle, then the connection was failing. This is because
s->init_num was getting reset. We should only reset it after we have
read the whole message.
RT#4394
Reviewed-by: Andy Polyakov <appro@openssl.org>
ChangeCipherSpec messages have a defined value. They also may not occur
in the middle of a handshake message. The current logic will accept a
ChangeCipherSpec with value 2. It also would accept up to three bytes of
handshake data before the ChangeCipherSpec which it would discard
(because s->init_num gets reset).
Instead, require that s->init_num is 0 when a ChangeCipherSpec comes in.
RT#4391
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
The write BIO for handshake messages is bufferred so that we only write
out to the network when we have a complete flight. There was some
complexity in the buffering logic so that we switched buffering on and
off at various points through out the handshake. The only real reason to
do this was historically it complicated the state machine when you wanted
to flush because you had to traverse through the "flush" state (in order
to cope with NBIO). Where we knew up front that there was only going to
be one message in the flight we switched off buffering to avoid that.
In the new state machine there is no longer a need for a flush state so
it is simpler just to have buffering on for the whole handshake. This
also gives us the added benefit that we can simply call flush after every
flight even if it only has one message in it. This means that BIO authors
can implement their own buffering strategies and not have to be aware of
the state of the SSL object (previously they would have to switch off
their own buffering during the handshake because they could not rely on
a flush being received when they really needed to write data out). This
last point addresses GitHub Issue #322.
Reviewed-by: Andy Polyakov <appro@openssl.org>
Set ctx->error = X509_V_ERR_OUT_OF_MEM when verificaiton cannot
continue due to malloc failure. Also, when X509_verify_cert()
returns <= 0 make sure that the verification status does not remain
X509_V_OK, as a last resort set it it to X509_V_ERR_UNSPECIFIED,
just in case some code path returns an error without setting an
appropriate value of ctx->error.
Reviewed-by: Richard Levitte <levitte@openssl.org>
This involves providing more session ticket key data, for both the cipher and
the digest
Signed-off-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
GH: #515, MR: #2153
The current limit of 2^14 bytes is too low (e.g. RFC 5246 specifies the
maximum size of just the extensions field to be 2^16-1), and may cause
bogus failures.
RT#4063
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/413)
Per RFC 4507, section 3.3:
This message [NewSessionTicket] MUST be sent if the
server included a SessionTicket extension in the ServerHello. This
message MUST NOT be sent if the server did not include a
SessionTicket extension in the ServerHello.
The presence of the NewSessionTicket message should be determined
entirely from the ServerHello without probing.
RT#4389
Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
The V2ClientHello code creates an empty compression list, but the
compression list must explicitly contain the null compression (and later
code enforces this).
RT#4387
Reviewed-by: Stephen Henson <steve@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
The tls_client_key_exchange_post_work() frees the pms on error. It also
calls ssl_generate_master_secret() which also free the pms. If an error
occurs after ssl_generate_master_secret() has been called then a double
free can occur.
Reviewed-by: Andy Polyakov <appro@openssl.org>
If we fail halfway through constructing the peer_tmp EVP_PKEY but we have
already stored it in s->s3->peer_tmp then if anything tries to use it then
it will likely fail. This was causing s_client to core dump in the
sslskewith0p test. s_client was trying to print out the connection
parameters that it had negotiated so far. Arguably s_client should not do
that if the connection has failed...but given it is existing functionality
it's easier to fix libssl.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Move the dh_st structure into an internal header file and provide
relevant accessors for the internal fields.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Suppress CT callbacks with aNULL or PSK ciphersuites that involve
no certificates. Ditto when the certificate chain is validated via
DANE-TA(2) or DANE-EE(3) TLSA records. Also skip SCT processing
when the chain is fails verification.
Move and consolidate CT callbacks from libcrypto to libssl. We
also simplify the interface to SSL_{,CTX_}_enable_ct() which can
specify either a permissive mode that just collects information or
a strict mode that requires at least one valid SCT or else asks to
abort the connection.
Simplified SCT processing and options in s_client(1) which now has
just a simple pair of "-noct" vs. "-ct" options, the latter enables
the permissive callback so that we can complete the handshake and
report all relevant information. When printing SCTs, print the
validation status if set and not valid.
Signed-off-by: Rob Percival <robpercival@google.com>
Reviewed-by: Emilia Käsper <emilia@openssl.org>
The original check for != SSL_VERIFY_NONE can give surprising results
when flags SSL_VERIFY_PEER is not set, but other flags are. Note
that SSL_VERIFY_NONE (0) is not a flag bit, it is rather the absense
of all other flag bits.
Signed-off-by: Rob Percival <robpercival@google.com>
Reviewed-by: Emilia Käsper <emilia@openssl.org>
Free up parsed X509_NAME structure if the CertificateRequest message
contains excess data.
The security impact is considered insignificant. This is a client side
only leak and a large number of connections to malicious servers would
be needed to have a significant impact.
This was found by libFuzzer.
Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Stephen Henson <steve@openssl.org>
Session resumption involves a version check, so version negotiation must
happen first. Currently, the DTLS implementation cannot do session
resumption in DTLS 1.0 because the ssl_version check always checks
against 1.2.
Switching the order also removes the need to fixup ssl_version in DTLS
version negotiation.
Signed-off-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
RT: #4392, MR: #2452
We now send the highest supported version by the client, even if the session
uses an older version.
This fixes 2 problems:
- When you try to reuse a session but the other side doesn't reuse it and
uses a different protocol version the connection will fail.
- When you're trying to reuse a session with an old version you might be
stuck trying to reuse the old version while both sides support a newer
version
Signed-off-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
GH: #852, MR: #2452
Don't have #error statements in header files, but instead wrap
the contents of that file in #ifndef OPENSSL_NO_xxx
This means it is now always safe to include the header file.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Adjust ssl_set_client_hello_version to get both the minimum and maximum and then
make ssl_set_client_hello_version use the maximum version.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
MR: #1595
Disabled by default, but can be enabled by setting the
ct_validation_callback on a SSL or SSL_CTX.
Reviewed-by: Ben Laurie <ben@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
Add -DBIO_DEBUG to --strict-warnings.
Remove comments about outdated debugging ifdef guards.
Remove md_rand ifdef guarding an assert; it doesn't seem used.
Remove the conf guards in conf_api since we use OPENSSL_assert, not assert.
For pkcs12 stuff put OPENSSL_ in front of the macro name.
Merge TLS_DEBUG into SSL_DEBUG.
Various things just turned on/off asserts, mainly for checking non-NULL
arguments, which is now removed: camellia, bn_ctx, crypto/modes.
Remove some old debug code, that basically just printed things to stderr:
DEBUG_PRINT_UNKNOWN_CIPHERSUITES, DEBUG_ZLIB, OPENSSL_RI_DEBUG,
RL_DEBUG, RSA_DEBUG, SCRYPT_DEBUG.
Remove OPENSSL_SSL_DEBUG_BROKEN_PROTOCOL.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Remove support for static ECDH ciphersuites. They require ECDH keys
in certificates and don't support forward secrecy.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
