[openssl-commits] [openssl] master update
Kurt Roeckx
kurt at openssl.org
Tue Jun 21 18:56:12 UTC 2016
The branch master has been updated
via 5388b8d4e8faac21921843c63b12b71c0ab9153e (commit)
via 5b8fa431ae8eb5a18ba913494119e394230d4b70 (commit)
via 01238aec4071eabf072f4e98e3fb84cbab3c7107 (commit)
from 28bd8e945ff0bf50183af2481cc36180fbccaedb (commit)
- Log -----------------------------------------------------------------
commit 5388b8d4e8faac21921843c63b12b71c0ab9153e
Author: Kurt Roeckx <kurt at roeckx.be>
Date: Sat Jun 18 19:50:11 2016 +0200
Avoid creating an illegal pointer.
Found by tis-interpreter
Reviewed-by: Rich Salz <rsalz at openssl.org>
GH: #1230
commit 5b8fa431ae8eb5a18ba913494119e394230d4b70
Author: David Benjamin <davidben at google.com>
Date: Thu Jun 16 14:15:19 2016 -0400
Make RSA key exchange code actually constant-time.
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
294d1e36c2495ff00e697c9ff622856d3114f14f, and the logic after the
decryption in the SSL code is constant-time as of
adb46dbc6dd7347750df2468c93e8c34bcb93a4b, 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 at roeckx.be>
Reviewed-by: Rich Salz <rsalz at openssl.org>
GH: #1222
commit 01238aec4071eabf072f4e98e3fb84cbab3c7107
Author: Kurt Roeckx <kurt at roeckx.be>
Date: Sun Jun 19 14:16:16 2016 +0200
buf2hexstr: properly deal with empty string
It wrote before the start of the string
found by afl
Reviewed-by: Richard Levitte <levitte at openssl.org>
MR: #2994
-----------------------------------------------------------------------
Summary of changes:
crypto/asn1/a_int.c | 12 +++++-----
crypto/o_str.c | 7 +++++-
doc/crypto/OPENSSL_malloc.pod | 2 +-
ssl/statem/statem_srvr.c | 51 ++++++++++++++++++++++++++++++-------------
4 files changed, 49 insertions(+), 23 deletions(-)
diff --git a/crypto/asn1/a_int.c b/crypto/asn1/a_int.c
index 9c28c02..43174f7 100644
--- a/crypto/asn1/a_int.c
+++ b/crypto/asn1/a_int.c
@@ -115,21 +115,21 @@ static size_t i2c_ibuf(const unsigned char *b, size_t blen, int neg,
memcpy(p, b, blen);
else {
/* Begin at the end of the encoding */
- n = b + blen - 1;
- p += blen - 1;
+ n = b + blen;
+ p += blen;
i = blen;
/* Copy zeros to destination as long as source is zero */
- while (!*n && i > 1) {
- *(p--) = 0;
+ while (!n[-1] && i > 1) {
+ *(--p) = 0;
n--;
i--;
}
/* Complement and increment next octet */
- *(p--) = ((*(n--)) ^ 0xff) + 1;
+ *(--p) = ((*(--n)) ^ 0xff) + 1;
i--;
/* Complement any octets left */
for (; i > 0; i--)
- *(p--) = *(n--) ^ 0xff;
+ *(--p) = *(--n) ^ 0xff;
}
*pp += ret;
diff --git a/crypto/o_str.c b/crypto/o_str.c
index 29c324f..beabec0 100644
--- a/crypto/o_str.c
+++ b/crypto/o_str.c
@@ -198,7 +198,12 @@ char *OPENSSL_buf2hexstr(const unsigned char *buffer, long len)
const unsigned char *p;
int i;
- if ((tmp = OPENSSL_malloc(len * 3 + 1)) == NULL) {
+ if (len == 0)
+ {
+ return OPENSSL_zalloc(1);
+ }
+
+ if ((tmp = OPENSSL_malloc(len * 3)) == NULL) {
CRYPTOerr(CRYPTO_F_OPENSSL_BUF2HEXSTR, ERR_R_MALLOC_FAILURE);
return NULL;
}
diff --git a/doc/crypto/OPENSSL_malloc.pod b/doc/crypto/OPENSSL_malloc.pod
index ba50221..5d254f7 100644
--- a/doc/crypto/OPENSSL_malloc.pod
+++ b/doc/crypto/OPENSSL_malloc.pod
@@ -124,7 +124,7 @@ An odd number of hex digits is an error.
OPENSSL_buf2hexstr() takes the specified buffer and length, and returns
a hex string for value, or NULL on error.
-B<Buffer> cannot be NULL; if B<len> is NULL an empty string is returned.
+B<Buffer> cannot be NULL; if B<len> is 0 an empty string is returned.
OPENSSL_hexchar2int() converts a character to the hexadecimal equivalent,
or returns -1 on error.
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index f88b6c8..a88b321 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -2087,7 +2087,7 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
unsigned char rand_premaster_secret[SSL_MAX_MASTER_KEY_LENGTH];
int decrypt_len;
unsigned char decrypt_good, version_good;
- size_t j;
+ size_t j, padding_len;
/* FIX THIS UP EAY EAY EAY EAY */
rsa = EVP_PKEY_get0_RSA(s->cert->pkeys[SSL_PKEY_RSA_ENC].privatekey);
@@ -2144,17 +2144,37 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
goto err;
}
+ /*
+ * Decrypt with no padding. PKCS#1 padding will be removed as part of
+ * the timing-sensitive code below.
+ */
decrypt_len = RSA_private_decrypt(PACKET_remaining(&enc_premaster),
PACKET_data(&enc_premaster),
- rsa_decrypt, rsa, RSA_PKCS1_PADDING);
- ERR_clear_error();
+ rsa_decrypt, rsa, RSA_NO_PADDING);
+ if (decrypt_len < 0) {
+ goto err;
+ }
+
+ /* Check the padding. See RFC 3447, section 7.2.2. */
/*
- * decrypt_len should be SSL_MAX_MASTER_KEY_LENGTH. decrypt_good will
- * be 0xff if so and zero otherwise.
+ * The smallest padded premaster is 11 bytes of overhead. Small keys
+ * are publicly invalid, so this may return immediately. This ensures
+ * PS is at least 8 bytes.
*/
- decrypt_good =
- constant_time_eq_int_8(decrypt_len, SSL_MAX_MASTER_KEY_LENGTH);
+ if (decrypt_len < 11 + SSL_MAX_MASTER_KEY_LENGTH) {
+ al = SSL_AD_DECRYPT_ERROR;
+ SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_DECRYPTION_FAILED);
+ goto f_err;
+ }
+
+ padding_len = decrypt_len - SSL_MAX_MASTER_KEY_LENGTH;
+ decrypt_good = constant_time_eq_int_8(rsa_decrypt[0], 0) &
+ constant_time_eq_int_8(rsa_decrypt[1], 2);
+ for (j = 2; j < padding_len - 1; j++) {
+ decrypt_good &= ~constant_time_is_zero_8(rsa_decrypt[j]);
+ }
+ decrypt_good &= constant_time_is_zero_8(rsa_decrypt[padding_len - 1]);
/*
* If the version in the decrypted pre-master secret is correct then
@@ -2165,10 +2185,10 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
* constant time and are treated like any other decryption error.
*/
version_good =
- constant_time_eq_8(rsa_decrypt[0],
+ constant_time_eq_8(rsa_decrypt[padding_len],
(unsigned)(s->client_version >> 8));
version_good &=
- constant_time_eq_8(rsa_decrypt[1],
+ constant_time_eq_8(rsa_decrypt[padding_len + 1],
(unsigned)(s->client_version & 0xff));
/*
@@ -2182,10 +2202,10 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
*/
if (s->options & SSL_OP_TLS_ROLLBACK_BUG) {
unsigned char workaround_good;
- workaround_good =
- constant_time_eq_8(rsa_decrypt[0], (unsigned)(s->version >> 8));
+ workaround_good = constant_time_eq_8(rsa_decrypt[padding_len],
+ (unsigned)(s->version >> 8));
workaround_good &=
- constant_time_eq_8(rsa_decrypt[1],
+ constant_time_eq_8(rsa_decrypt[padding_len + 1],
(unsigned)(s->version & 0xff));
version_good |= workaround_good;
}
@@ -2203,12 +2223,13 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
* it is still sufficiently large to read from.
*/
for (j = 0; j < sizeof(rand_premaster_secret); j++) {
- rsa_decrypt[j] =
- constant_time_select_8(decrypt_good, rsa_decrypt[j],
+ rsa_decrypt[padding_len + j] =
+ constant_time_select_8(decrypt_good,
+ rsa_decrypt[padding_len + j],
rand_premaster_secret[j]);
}
- if (!ssl_generate_master_secret(s, rsa_decrypt,
+ if (!ssl_generate_master_secret(s, rsa_decrypt + padding_len,
sizeof(rand_premaster_secret), 0)) {
al = SSL_AD_INTERNAL_ERROR;
SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
More information about the openssl-commits
mailing list