Commit aea145e removed some error codes that are generated
algorithmically: mapping alerts to error texts. Found by
Andreas Karlsson. This restores them, and adds two missing ones.
Reviewed-by: Matt Caswell <matt@openssl.org>
The SSL_load_client_CA_file() failed to load any CAs due to an
inccorrect assumption about the return value of lh_*_insert(). The
return value when inserting into a hash is the old value of the key.
The bug was introduced in 3c82e437bb.
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1279)
We calculate the size required for the ServerKeyExchange message and then
call BUF_MEM_grow_clean() on the buffer. However we fail to take account of
2 bytes required for the signature algorithm and 2 bytes for the signature
length, i.e. we could overflow by 4 bytes. In reality this won't happen
because the buffer is pre-allocated to a large size that means it should be
big enough anyway.
Addresses an OCAP Audit issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1264)
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1264)
In some situations (such as when we receive a fragment of an alert)
we try to get the next packet but did not mark the current one as read,
meaning that we got the same record back again - leading to an infinite
loop.
Found using the BoringSSL test suite.
Reviewed-by: Andy Polyakov <appro@openssl.org>
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
ssl_session_hash() always looks at the first 4 bytes, regardless of the length.
A client can send a session id that's shorter, and the callback could also
generate one that's shorter. So we make sure that the rest of the buffer is
initliazed to 0 so that we always calculate the same hash.
Found by tis-interpreter, also previously reported as RT #2871
Reviewed-by: Rich Salz <rsalz@openssl.org>
MR: #2911
SSL_clear() and SSL_free() will remove a session from the cache if it is
considered "bad". However SSL_set_session() does not do this for the session
it is replacing.
SSL_clear() clears an SSL object ready for reuse. It does not clear the
session though. This means that:
SSL_clear(s)
SSL_set_session(s, sess);
and
SSL_set_session(s, sess);
SSL_clear(s);
do not do the same thing, although logically you would expect that they
would.
The failure of SSL_set_session() to remove bad sessions from the cache
should be considered a bug, so this commit fixes it.
RT#597
Reviewed-by: Rich Salz <rsalz@openssl.org>
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)
Sessions are stored on the session_ctx, which doesn't change after
SSL_set_SSL_CTX().
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Previously if we received an empty record we just threw it away and
ignored it. Really though if we get an empty record of a different content
type to what we are expecting then that should be an error, i.e. we should
reject out of context empty records. This commit makes the necessary changes
to achieve that.
RT#4395
Reviewed-by: Andy Polyakov <appro@openssl.org>
The tlsext_status_type field in SSL is used by e.g. OpenResty to determine
if the client requested the certificate status, but SSL is now opaque.
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
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>
When setting the certificate chain, if a certificate doesn't pass
security checks, then chain may point to a freed STACK_OF(X509)
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Windows was complaining about a unary minus operator being applied to an
unsigned type. It did seem to go on and do the right thing anyway, but the
code does look a little suspect. This fixes it.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
The ssl3_digest_cached_records() function was failing to handle errors
that might be returned from EVP_DigestSignInit() and
EVP_DigestSignUpdate().
RT#4180
Reviewed-by: Stephen Henson <steve@openssl.org>
Rename sk_xxx to OPENSSL_sk_xxx and _STACK to OPENSSL_STACK
Rename lh_xxx API to OPENSSL_LH_xxx and LHASH_NODE to OPENSSL_LH_NODE
Make lhash stuff opaque.
Use typedefs for function pointers; makes the code simpler.
Remove CHECKED_xxx macros.
Add documentation; remove old X509-oriented doc.
Add API-compat names for entire old API
Reviewed-by: Dr. Stephen Henson <steve@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>
Since with SSL_VERIFY_NONE, the connection may continue and the
session may even be cached, we should save some evidence that the
chain was not sufficiently verified and would have been rejected
with SSL_VERIFY_PEER. To that end when a CT callback returs failure
we set the verify result to X509_V_ERR_NO_VALID_SCTS.
Note: We only run the CT callback in the first place if the verify
result is still X509_V_OK prior to start of the callback.
RT #4502
Reviewed-by: Tim Hudson <tjh@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>
In the SSLV2ClientHello processing code in ssl3_get_record, the value of
|num_recs| will always be 0. This isn't obvious from the code so a comment
is added to explain it.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
The function ssl3_get_record() can obtain multiple records in one go
as long as we are set up for pipelining and all the records are app
data records. The logic in the while loop which reads in each record is
supposed to only continue looping if the last record we read was app data
and we have an app data record waiting in the buffer to be processed. It
was actually checking that the first record had app data and we have an
app data record waiting. This actually amounts to the same thing so wasn't
wrong - but it looks a bit odd because it uses the |rr| array without an
offset.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Pipelining introduced the concept of multiple records being read in one
go. Therefore we work with an array of SSL3_RECORD objects. The pipelining
change erroneously made a change in ssl3_get_record() to apply the current
record offset to the SSL3_BUFFER we are using for reading. This is wrong -
there is only ever one read buffer. This reverts that change. In practice
this should make little difference because the code block in question is
only ever used when we are processing a single record.
Reviewed-by: Viktor Dukhovni <viktor@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
Since 50932c4 "PACKETise ServerHello processing",
ssl_next_proto_validate() incorrectly allows empty protocol name.
draft-agl-tls-nextprotoneg-04[1] says "Implementations MUST ensure that
the empty string is not included and that no byte strings are
truncated."
This patch restores the old correct behavior.
[1] https://tools.ietf.org/html/draft-agl-tls-nextprotoneg-04
Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Add a status return value instead of void.
Add some sanity checks on reference counter value.
Update the docs.
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
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>
Per RFC 5246,
Note: this extension is not meaningful for TLS versions prior to 1.2.
Clients MUST NOT offer it if they are offering prior versions.
However, even if clients do offer it, the rules specified in [TLSEXT]
require servers to ignore extensions they do not understand.
Although second sentence would suggest that there would be no interop
problems in always offering the extension, WebRTC has reported issues
with Bouncy Castle on < TLS 1.2 ClientHellos that still include
signature_algorithms. See also
https://bugs.chromium.org/p/webrtc/issues/detail?id=4223
RT#4390
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Stephen Henson <steve@openssl.org>
When setting a new SRTP connection profile using
SSL_CTX_set_tlsext_use_srtp() or SSL_set_tlsext_use_srtp() we should
free any existing profile first to avoid a memory leak.
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
If the application has limited the size of the async pool using
ASYNC_init_thread() then we could run out of jobs while trying to start a
libssl io operation. However libssl was failing to handle this and treating
it like a fatal error. It should not be fatal...we just need to retry when
there are jobs available again.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Based on an orignal commit by GitHub user BertramScharpf. Rebased and
updated to take account of all the updates since this was first raised.
GH PR#62
Reviewed-by: Rich Salz <rsalz@openssl.org>
If no serverinfo extension is found in some cases, do not abort the handshake,
but simply omit/skip that extension.
Check for already-registered serverinfo callbacks during serverinfo
registration.
Update SSL_CTX_use_serverinfo() documentation to mention the need to reload the
same serverinfo per certificate, for servers with multiple server certificates.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
Building with -DCHARSET_EBCDIC and using --strict-warnings resulted in
lots of miscellaneous errors. This fixes it.
Reviewed-by: Andy Polyakov <appro@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>
The variables in the BIO weren't being duplicated properly.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
The i2d_X509() function can return a negative value on error. Therefore
we should make sure we check it.
Issue reported by Yuan Jochen Kang.
Reviewed-by: Emilia Käsper <emilia@openssl.org>
It is up to the caller of SSL_dane_tlsa_add() to take appropriate
action when no records are added successfully or adding some records
triggers an internal error (negative return value).
With this change the caller can continue with PKIX if desired when
none of the TLSA records are usable, or take some appropriate action
if DANE is required.
Also fixed the internal ssl_dane_dup() function to properly initialize
the TLSA RR stack in the target SSL handle. Errors in ssl_dane_dup()
are no longer ignored.
Reviewed-by: Rich Salz <rsalz@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>
The Unix build was the last to retain the classic build scheme. The
new unified scheme has matured enough, even though some details may
need polishing.
Reviewed-by: Rich Salz <rsalz@openssl.org>
Make OBJ_name_cmp internal
Rename idea_xxx to IDEA_xxx
Rename get_rfc_xxx to BN_get_rfc_xxx
Rename v3_addr and v3_asid functions to X509v3_...
Reviewed-by: Richard Levitte <levitte@openssl.org>
Make X509_OBJECT, X509_STORE_CTX, X509_STORE, X509_LOOKUP,
and X509_LOOKUP_METHOD opaque.
Remove unused X509_CERT_FILE_CTX
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Dr. Stephen Henson <steve@openssl.org>
There is a preference for suffixes to indicate that a function is internal
rather than prefixes. Note: the suffix is only required to disambiguate
internal functions and public symbols with the same name (but different
case)
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
There was a lot of naming inconsistency, so we try and standardise on
one form.
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
SSL_COMP_free_compression_methods() should not be called expicitly - we
should leave auto-deinit to clean this up instead.
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
ERR_free_strings() should not be called expicitly - we should leave
auto-deinit to clean this up instead.
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
Add an accessor for SSL_CTX.
Since libssl was made opaque, there is no way for users to access the
cipher_list, while users can set the cipher_list by
SSL_CTX_set_cipher_list().
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
The function pqueue_print is not exported and is never called. Therefore
we should delete it.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
