The problem occurs in EVP_PKEY_sign() when using RSA with X931 padding.
It is only triggered if the RSA key size is smaller than the digest length.
So with SHA512 you can trigger the overflow with anything less than an RSA
512 bit key. I managed to trigger a 62 byte overflow when using a 16 bit RSA
key. This wasn't sufficient to cause a crash, although your mileage may
vary.
In practice RSA keys of this length are never used and X931 padding is very
rare. Even if someone did use an excessively short RSA key, the chances of
them combining that with a longer digest and X931 padding is very
small. For these reasons I do not believe there is a security implication to
this. Thanks to Kevin Wojtysiak (Int3 Solutions) and Paramjot Oberoi (Int3
Solutions) for reporting this issue.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit 34166d4189)
Add a sanity check to the print_bin function to ensure that the |off|
argument is positive. Thanks to Kevin Wojtysiak (Int3 Solutions) and
Paramjot Oberoi (Int3 Solutions) for reporting this issue.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit 3deeeeb61b)
Sanity check the |len| parameter to ensure it is positive. Thanks to Kevin
Wojtysiak (Int3 Solutions) and Paramjot Oberoi (Int3 Solutions) for
reporting this issue.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit cb0f400b0c)
The return value is checked for 0. This is currently safe but we should
really check for <= 0 since -1 is frequently used for error conditions.
Thanks to Kevin Wojtysiak (Int3 Solutions) and Paramjot Oberoi (Int3
Solutions) for reporting this issue.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit c427570e50)
Conflicts:
ssl/ssl_locl.h
For SSLv3 the code assumes that |header_length| > |md_block_size|. Whilst
this is true for all SSLv3 ciphersuites, this fact is far from obvious by
looking at the code. If this were not the case then an integer overflow
would occur, leading to a subsequent buffer overflow. Therefore I have
added an explicit sanity check to ensure header_length is always valid.
Thanks to Kevin Wojtysiak (Int3 Solutions) and Paramjot Oberoi (Int3
Solutions) for reporting this issue.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit 29b0a15a48)
The static function dynamically allocates an output buffer if the output
grows larger than the static buffer that is normally used. The original
logic implied that |currlen| could be greater than |maxlen| which is
incorrect (and if so would cause a buffer overrun). Also the original
logic would call OPENSSL_malloc to create a dynamic buffer equal to the
size of the static buffer, and then immediately call OPENSSL_realloc to
make it bigger, rather than just creating a buffer than was big enough in
the first place. Thanks to Kevin Wojtysiak (Int3 Solutions) and Paramjot
Oberoi (Int3 Solutions) for reporting this issue.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit 9d9e37744c)
There was already a sanity check to ensure the passed buffer length is not
zero. Extend this to ensure that it also not negative. Thanks to Kevin
Wojtysiak (Int3 Solutions) and Paramjot Oberoi (Int3 Solutions) for
reporting this issue.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit b86d7dca69)
The various implementations of EVP_CTRL_AEAD_TLS_AAD expect a buffer of at
least 13 bytes long. Add sanity checks to ensure that the length is at
least that. Also add a new constant (EVP_AEAD_TLS1_AAD_LEN) to evp.h to
represent this length. Thanks to Kevin Wojtysiak (Int3 Solutions) and
Paramjot Oberoi (Int3 Solutions) for reporting this issue.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit c826988109)
Conflicts:
ssl/record/ssl3_record.c
Add a sanity check to DES_enc_write to ensure the buffer length provided
is not negative. Thanks to Kevin Wojtysiak (Int3 Solutions) and Paramjot
Oberoi (Int3 Solutions) for reporting this issue.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit 873fb39f20)
Fortify flagged up a problem in n_do_ssl_write() in SSLv2. Analysing the
code I do not believe there is a real problem here. However the logic flows
are complicated enough that a sanity check of |len| is probably worthwhile.
Thanks to Kevin Wojtysiak (Int3 Solutions) and Paramjot Oberoi (Int3
Solutions) for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
This reverts commit 47daa155a3.
The above commit was backported to the 1.0.2 branch as part of backporting
the alternative chain verify algorithm changes. However it has been pointed
out (credit to Shigeki Ohtsu) that this is unnecessary in 1.0.2 as this
commit is a work around for loop checking that only exists in master.
Reviewed-by: Richard Levitte <levitte@openssl.org>
The function CRYPTO_strdup (aka OPENSSL_strdup) fails to check the return
value from CRYPTO_malloc to see if it is NULL before attempting to use it.
This patch adds a NULL check.
RT3786
Signed-off-by: Matt Caswell <matt@openssl.org>
(cherry picked from commit 37b0cf936744d9edb99b5dd82cae78a7eac6ad60)
Reviewed-by: Rich Salz <rsalz@openssl.org>
(cherry picked from commit 20d21389c8b6f5b754573ffb6a4dc4f3986f2ca4)
EAP-FAST session resumption relies on handshake message lookahead
to determine server intentions. Commits
980bc1ec61
and
7b3ba508af
removed the lookahead so broke session resumption.
This change partially reverts the commits and brings the lookahead back
in reduced capacity for TLS + EAP-FAST only. Since EAP-FAST does not
support regular session tickets, the lookahead now only checks for a
Finished message.
Regular handshakes are unaffected by this change.
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(cherry picked from commit 6e3d015363)
The logic with how 'ok' was calculated didn't quite convey what's "ok",
so the logic is slightly redone to make it less confusing.
Reviewed-by: Andy Polyakov <appro@openssl.org>
(cherry picked from commit 06affe3dac)
Filled in lots of return value checks that were missing the GOST engine, and
added appropriate error handling.
Reviewed-by: Richard Levitte <levitte@openssl.org>
(cherry picked from commit 8817e2e0c9)
Fix miscellaneous NULL pointer derefs in the sureware engine.
Reviewed-by: Richard Levitte <levitte@openssl.org>
(cherry picked from commit 7b611e5fe8)
This addresses
- request for improvement for faster key setup in RT#3576;
- clearing registers and stack in RT#3554 (this is more of a gesture to
see if there will be some traction from compiler side);
- more commentary around input parameters handling and stack layout
(desired when RT#3553 was reviewed);
- minor size and single block performance optimization (was lying around);
Reviewed-by: Matt Caswell <matt@openssl.org>
(cherry picked from commit 23f6eec71d)
Disable loop checking when we retry verification with an alternative path.
This fixes the case where an intermediate CA is explicitly trusted and part
of the untrusted certificate list. By disabling loop checking for this case
the untrusted CA can be replaced by the explicitly trusted case and
verification will succeed.
Signed-off-by: Matt Caswell <matt@openssl.org>
(cherry picked from commit e5991ec528)
Reviewed-by: Rich Salz <rsalz@openssl.org>
X509_V_FLAG_NO_ALT_CHAINS flag. Using this option means that when building
certificate chains, the first chain found will be the one used. Without this
flag, if the first chain found is not trusted then we will keep looking to
see if we can build an alternative chain instead.
Conflicts:
apps/cms.c
apps/ocsp.c
apps/s_client.c
apps/s_server.c
apps/smime.c
apps/verify.c
Reviewed-by: Rich Salz <rsalz@openssl.org>
valid. However the issuer of the leaf, or some intermediate cert is in fact
in the trust store.
When building a trust chain if the first attempt fails, then try to see if
alternate chains could be constructed that are trusted.
RT3637
RT3621
Reviewed-by: Rich Salz <rsalz@openssl.org>
Fix bug where i2c_ASN1_INTEGER mishandles zero if it is marked as
negative.
Thanks to Huzaifa Sidhpurwala <huzaifas@redhat.com> and
Hanno Böck <hanno@hboeck.de> for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(cherry picked from commit a0eed48d37)
A 0-length ciphers list is never permitted. The old code only used to
reject an empty ciphers list for connections with a session ID. It
would later error out on a NULL structure, so this change just moves
the alert closer to the problem source.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(cherry picked from commit 3ae91cfb32)
The disabled set of -Weverything is hard to maintain across versions.
Use -Wall -Wextra but also document other useful warnings that currently trigger.
Reviewed-by: Rich Salz <rsalz@openssl.org>
If OpenSSL is configured with no-tlsext then ssl_get_prev_session can read
past the end of the ClientHello message if the session_id length in the
ClientHello is invalid. This should not cause any security issues since the
underlying buffer is 16k in size. It should never be possible to overrun by
that many bytes.
This is probably made redundant by the previous commit - but you can never be
too careful.
With thanks to Qinghao Tang for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(cherry picked from commit 5e0a80c1c9)
The ClientHello processing is insufficiently rigorous in its checks to make
sure that we don't read past the end of the message. This does not have
security implications due to the size of the underlying buffer - but still
needs to be fixed.
With thanks to Qinghao Tang for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(cherry picked from commit c9642eb1ff79a30e2c7632ef8267cc34cc2b0d79)
It would set gen->d.dirn to a freed pointer in case X509V3_NAME_from_section
failed.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(cherry picked from commit 8ec5c5dd36)
While *pval is usually a pointer in rare circumstances it can be a long
value. One some platforms (e.g. WIN64) where
sizeof(long) < sizeof(ASN1_VALUE *) this will write past the field.
*pval is initialised correctly in the rest of ASN1_item_ex_new so setting it
to NULL is unecessary anyway.
Thanks to Julien Kauffmann for reporting this issue.
Reviewed-by: Richard Levitte <levitte@openssl.org>
(cherry picked from commit f617b4969a)
Conflicts:
crypto/asn1/tasn_new.c
Since source reformat, we ended up with some error reason string
definitions that spanned two lines. That in itself is fine, but we
sometimes edited them to provide better strings than what could be
automatically determined from the reason macro, for example:
{ERR_REASON(SSL_R_NO_GOST_CERTIFICATE_SENT_BY_PEER),
"Peer haven't sent GOST certificate, required for selected ciphersuite"},
However, mkerr.pl didn't treat those two-line definitions right, and
they ended up being retranslated to whatever the macro name would
indicate, for example:
{ERR_REASON(SSL_R_NO_GOST_CERTIFICATE_SENT_BY_PEER),
"No gost certificate sent by peer"},
Clearly not what we wanted. This change fixes this problem.
Reviewed-by: Matt Caswell <matt@openssl.org>
(cherry picked from commit 2cfdfe0918)
The macros BSWAP4 and BSWAP8 have statetemnt expressions
implementations that use local variable names that shadow variables
outside the macro call, generating warnings like this
e_aes_cbc_hmac_sha1.c:263:14: warning: declaration shadows a local variable
[-Wshadow]
seqnum = BSWAP8(blocks[0].q[0]);
^
../modes/modes_lcl.h:41:29: note: expanded from macro 'BSWAP8'
^
e_aes_cbc_hmac_sha1.c:223:12: note: previous declaration is here
size_t ret = 0;
^
Have clang be quiet by modifying the macro variable names slightly
(suffixing them with an underscore).
Reviewed-by: Rich Salz <rsalz@openssl.org>
(cherry picked from commit 2da2a4349c)
