When Makefile/opensslconf.h is unchanged, don't write it at all.
Currently every time Configure is executed, these files are overwritten.
Makefile leads to regeneration of buildinf.h, and opensslconf.h is itself
a central header.
As a result, Configure triggers full rebuild, even if nothing is changed.
Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1370)
Rich Salz [Tue, 15 Nov 2016 23:54:28 +0000 (18:54 -0500)]
Check return value of some BN functions.
Factorise multiple bn_get_top(group->field) calls
Add missing checks on some conditional BN_copy return value
Add missing checks on some BN_copy return value
Add missing checks on a few bn_wexpand return value
Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1626)
(cherry picked from commit 78e09b53a40729f5e99829ccc733b592bd22fea1)
Rich Salz [Fri, 4 Nov 2016 14:27:47 +0000 (10:27 -0400)]
Missed a mention of RT
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1849)
(cherry picked from commit 1e62cc12f35408508594be254f40bf9b65d2a3a9)
Matt Caswell [Wed, 2 Nov 2016 22:26:17 +0000 (22:26 +0000)]
Fail if an unrecognised record type is received
TLS1.0 and TLS1.1 say you SHOULD ignore unrecognised record types, but
TLS 1.2 says you MUST send an unexpected message alert. We swap to the
TLS 1.2 behaviour for all protocol versions to prevent issues where no
progress is being made and the peer continually sends unrecognised record
types, using up resources processing them.
Matt Caswell [Wed, 2 Nov 2016 15:36:06 +0000 (15:36 +0000)]
Fix heartbeat_test
The heartbeat_test reaches into the internals of libssl and calls some
internal functions. It then checks the return value to check its what it
expected. However commit fa4c37457 changed the return value of these
internal functions, and now the test is failing.
The solution is to update the test to look for the new return value.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Benjamin Kaduk [Mon, 26 Sep 2016 20:30:42 +0000 (15:30 -0500)]
Fix grammar-o in CONTRIBUTING
Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1625)
(cherry picked from commit e4d94269a5a41594852dc60716500580f1d47cef)
Matt Caswell [Fri, 14 Oct 2016 12:07:00 +0000 (13:07 +0100)]
Ensure we have length checks for all extensions
The previous commit inspired a review of all the length checks for the
extension adding code. This adds more robust checks and adds checks where
some were missing previously. The real solution for this is to use WPACKET
which is currently in master - but that cannot be applied to release
branches.
Matt Caswell [Fri, 14 Oct 2016 10:49:06 +0000 (11:49 +0100)]
Fix length check writing status request extension
The status request extension did not correctly check its length, meaning
that writing the extension could go 2 bytes beyond the buffer size. In
practice this makes little difference because, due to logic in buffer.c the
buffer is actually over allocated by approximately 5k!
Matt Caswell [Fri, 21 Oct 2016 13:49:33 +0000 (14:49 +0100)]
A zero return from BIO_read/BIO_write() could be retryable
A zero return from BIO_read()/BIO_write() could mean that an IO operation
is retryable. A zero return from SSL_read()/SSL_write() means that the
connection has been closed down (either cleanly or not). Therefore we
should not propagate a zero return value from BIO_read()/BIO_write() back
up the stack to SSL_read()/SSL_write(). This could result in a retryable
failure being treated as fatal.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Rich Salz [Sat, 22 Oct 2016 07:53:47 +0000 (03:53 -0400)]
Correctly find all critical CRL extensions
Unhandled critical CRL extensions were not detected if they appeared
after the handled ones. (GitHub issue 1757). Thanks to John Chuah
for reporting this.
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1769)
Richard Levitte [Wed, 28 Sep 2016 22:40:20 +0000 (00:40 +0200)]
If an engine comes up explicitely, it must also come down explicitely
In apps/apps.c, one can set up an engine with setup_engine().
However, we freed the structural reference immediately, which means
that for engines that don't already have a structural reference
somewhere else (because it has registered at least one cipher or digest
algorithm method, and therefore gets a functional reference through the
ENGINE_set_default() call), we end up returning an invalid reference.
Instead, the function release_engine() is added, and called at the end
of the routines that call setup_engine().
Originally, the ENGINE API wasn't designed for this to happen, an
engine had to register at least one algorithm method, and was
especially expected to register the algorithms corresponding to the
key types that could be stored and hidden in hardware. However, it
turns out that some engines will not register those algorithms with
the ENGINE_set_{algo}, ENGINE_set_cipher or ENGINE_set_digest
functions, as they only want the methods to be used for keys, not as
general crypto accelerator methods. That may cause ENGINE_set_default()
to do nothing, and no functional reference is therefore made, leading
to a premature deallocation of the engine and it thereby becoming
unavailable when trying to fetch a key.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1644)
Patrick Steuer [Mon, 17 Oct 2016 08:30:33 +0000 (10:30 +0200)]
Fix strict-warnings build
crypto/evp/e_aes.c: Types of inp and out parameters of AES_xts_en/decrypt
functions need to be changed from char to unsigned char to avoid build
error due to -Werror=incompatible-pointer-types.
crypto/aes/asm/aes-s390x.pl: Comments need to reflect the above change.
Signed-off-by: Patrick Steuer <psteuer@mail.de> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org>
CLA: trivial
Patrick Steuer [Mon, 17 Oct 2016 08:24:49 +0000 (10:24 +0200)]
Fix strict-warnings build
crypto/s390xcap.c: cryptlib.h needs to be included for OPENSSL_cpuid_setup
function prototype is located there to avoid build error due to
-Werror=missing-prototypes.
Signed-off-by: Patrick Steuer <psteuer@mail.de> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org>
CLA: trivial
Matt Caswell [Wed, 12 Oct 2016 15:43:03 +0000 (16:43 +0100)]
Ensure we handle len == 0 in ERR_err_string_n
If len == 0 in a call to ERR_error_string_n() then we can read beyond the
end of the buffer. Really applications should not be calling this function
with len == 0, but we shouldn't be letting it through either!
Thanks to Agostino Sarubbo for reporting this issue. Agostino's blog on
this issue is available here:
https://blogs.gentoo.org/ago/2016/10/14/openssl-libcrypto-stack-based-buffer-overflow-in-err_error_string_n-err-c/
Rich Salz [Wed, 12 Oct 2016 19:49:06 +0000 (15:49 -0400)]
RT is put out to pasture
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1702)
(cherry picked from commit 7954dced19a7e59e7055eab95a981fa943c7d100)
Richard Levitte [Wed, 28 Sep 2016 19:28:00 +0000 (21:28 +0200)]
apps/apps.c: initialize and de-initialize engine around key loading
Before loading a key from an engine, it may need to be initialized.
When done loading the key, we must de-initialize the engine.
(if the engine is already initialized somehow, only the reference
counter will be incremented then decremented)
David Woodhouse [Wed, 28 Sep 2016 13:31:22 +0000 (14:31 +0100)]
Call ENGINE_init() before trying to use keys from engine
Things like 'openssl s_client' only ever worked with keys from an engine
which provided a default generic method for some key type — because it
called ENGINE_set_default() and that ended up being an implicit
initialisation and functional refcount.
But an engine which doesn't provide generic methods doesn't get
initialised, and then when you try to use it you get an error:
cannot load client certificate private key file from engine 140688147056384:error:26096075:engine routines:ENGINE_load_private_key:not initialised:crypto/engine/eng_pkey.c:66:
unable to load client certificate private key file
cf. https://github.com/OpenSC/libp11/issues/107 (in which we discover
that engine_pkcs11 *used* to provide generic methods that OpenSSL would
try to use for ephemeral DH keys when negotiating ECDHE cipher suites in
TLS, and that didn't work out very well.)
Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1640)
Russian GOST ciphersuites are vulnerable to the KCI attack because they use
long-term keys to establish the connection when ssl client authorization is
on. This change brings the GOST implementation into line with the latest
specs in order to avoid the attack. It should not break backwards
compatibility.
Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org>
Matt Caswell [Fri, 9 Sep 2016 09:53:39 +0000 (10:53 +0100)]
Fix a mem leak in NPN handling
If a server sent multiple NPN extensions in a single ClientHello then a
mem leak can occur. This will only happen where the client has requested
NPN in the first place. It does not occur during renegotiation. Therefore
the maximum that could be leaked in a single connection with a malicious
server is 64k (the maximum size of the ServerHello extensions section). As
this is client side, only occurs if NPN has been requested and does not
occur during renegotiation this is unlikely to be exploitable.
Matt Caswell [Fri, 9 Sep 2016 09:08:45 +0000 (10:08 +0100)]
Fix OCSP Status Request extension unbounded memory growth
A malicious client can send an excessively large OCSP Status Request
extension. If that client continually requests renegotiation,
sending a large OCSP Status Request extension each time, then there will
be unbounded memory growth on the server. This will eventually lead to a
Denial Of Service attack through memory exhaustion. Servers with a
default configuration are vulnerable even if they do not support OCSP.
Builds using the "no-ocsp" build time option are not affected.
I have also checked other extensions to see if they suffer from a similar
problem but I could not find any other issues.
Matt Caswell [Wed, 21 Sep 2016 13:48:16 +0000 (14:48 +0100)]
Don't allow too many consecutive warning alerts
Certain warning alerts are ignored if they are received. This can mean that
no progress will be made if one peer continually sends those warning alerts.
Implement a count so that we abort the connection if we receive too many.
Grow TLS/DTLS 16 bytes more than strictly necessary as a precaution against
OOB reads. In most cases this will have no effect because the message buffer
will be large enough already.
There were some unexpected side effects to this commit, e.g. in SSLv3 a
warning alert gets sent "no_certificate" if a client does not send a
Certificate during Client Auth. With the above commit this causes the
connection to abort, which is incorrect. There may be some other edge cases
like this so we need to have a rethink on this.
Richard Levitte [Tue, 6 Sep 2016 15:39:35 +0000 (17:39 +0200)]
VMS: only use _realloc32 with /POINTER_SIZE=32
This fixes the following error when building with no particular pointer size
is specified (implied 32 bit):
static void *(*realloc_func) (void *, size_t) = realloc;
................................................^
%CC-E-UNDECLARED, In the initializer for realloc_func, "_realloc32" is not declared.
at line number 93 in file DEV:[OPENSSL102.crypto]mem.c;1
Matt Caswell [Thu, 8 Sep 2016 13:32:27 +0000 (14:32 +0100)]
Add some sanity checks around usage of t_fromb64()
The internal SRP function t_fromb64() converts from base64 to binary. It
does not validate that the size of the destination is sufficiently large -
that is up to the callers. In some places there was such a check, but not
in others.
Add an argument to t_fromb64() to provide the size of the destination
buffer and validate that we don't write too much data. Also add some sanity
checks to the callers where appropriate.
Matt Caswell [Mon, 12 Sep 2016 10:04:51 +0000 (11:04 +0100)]
Abort on unrecognised warning alerts
A peer continually sending unrecognised warning alerts could mean that we
make no progress on a connection. We should abort rather than continuing if
we receive an unrecognised warning alert.
David Woodhouse [Wed, 7 Sep 2016 15:53:18 +0000 (16:53 +0100)]
Avoid EVP_PKEY_cmp() crash on EC keys without public component
Some hardware devices don't provide the public EC_POINT data. The only
way for X509_check_private_key() to validate that the key matches a
given certificate is to actually perform a sign operation and then
verify it using the public key in the certificate.
Maybe that can come later, as discussed in issue 1532. But for now let's
at least make it fail gracefully and not crash.
GH: 1532
Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1547)
(cherry picked from commit 92ed7fa575a80955f3bb6efefca9bf576a953586)
Never output -0; make "negative zero" an impossibility.
Do better checking on BN_rand top/bottom requirements and #bits.
Update doc.
Ignoring trailing garbage in BN_asc2bn.
Port this commit from boringSSL: https://boringssl.googlesource.com/boringssl/+/899b9b19a4cd3fe526aaf5047ab9234cdca19f7d%5E!/
Ensure |BN_div| never gives negative zero in the no_branch code.
Have |bn_correct_top| fix |bn->neg| if the input is zero so that we
don't have negative zeros lying around.
Matt Caswell [Tue, 30 Aug 2016 14:06:01 +0000 (15:06 +0100)]
Ensure the CertStatus message adds a DTLS message header where needed
The function tls_construct_cert_status() is called by both TLS and DTLS
code. However it only ever constructed a TLS message header for the message
which obviously failed in DTLS.
Richard Levitte [Fri, 26 Aug 2016 07:59:55 +0000 (09:59 +0200)]
Improve the definition of STITCHED_CALL in e_rc4_hmac_md5.c
The definition of STITCHED_CALL relies on OPENSSL_NO_ASM. However,
when a configuration simply lacks the assembler implementation for RC4
(which is where we have implemented the stitched call), OPENSSL_NO_ASM
isn't implemented. Better, then, to rely on specific macros that
indicated that RC4 (and MD5) are implemented in assembler.
For this to work properly, we must also make sure Configure adds the
definition of RC4_ASM among the C flags.
David Woodhouse [Tue, 2 Aug 2016 21:54:46 +0000 (22:54 +0100)]
Fix ubsan 'left shift of negative value -1' error in satsub64be()
Baroque, almost uncommented code triggers behaviour which is undefined
by the C standard. You might quite reasonably not care that the code was
broken on ones-complement machines, but if we support a ubsan build then
we need to at least pretend to care.
It looks like the special-case code for 64-bit big-endian is going to
behave differently (and wrongly) on wrap-around, because it treats the
values as signed. That seems wrong, and allows replay and other attacks.
Surely you need to renegotiate and start a new epoch rather than
wrapping around to sequence number zero again?
David Woodhouse [Fri, 8 Jul 2016 19:46:07 +0000 (20:46 +0100)]
Fix SSL_export_keying_material() for DTLS1_BAD_VER
Commit d8e8590e ("Fix missing return value checks in SCTP") made the
DTLS handshake fail, even for non-SCTP connections, if
SSL_export_keying_material() fails. Which it does, for DTLS1_BAD_VER.
Apply the trivial fix to make it succeed, since there's no real reason
why it shouldn't even though we never need it.
Andy Polyakov [Wed, 24 Aug 2016 15:13:09 +0000 (17:13 +0200)]
ec/asm/ecp_nistz256-x86_64.pl: /cmovb/cmovc/ as nasm doesn't recognize cmovb.
Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org>
(cherry picked from commit d3034d31e7c04b334dd245504dd4f56e513ca115)
If a ticket callback changes the HMAC digest to SHA512 the existing
sanity checks are not sufficient and an attacker could perform a DoS
attack with a malformed ticket. Add additional checks based on
HMAC size.