generic-library/openssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Disentangle RSA premaster secret parsing

Browse Source 
Simplify encrypted premaster secret reading by using new methods in the
PACKET API.

Don't overwrite the packet buffer. RSA decrypt accepts truncated
ciphertext with leading zeroes omitted, so it's even possible that by
crafting a valid ciphertext with several leading zeroes, this could
cause a few bytes out-of-bounds write. The write is harmless because of
the size of the underlying message buffer, but nevertheless we shouldn't
write into the packet.

Reviewed-by: Matt Caswell <matt@openssl.org>
This commit is contained in:
Emilia Kasper 2015-09-09 14:45:00 +02:00
parent 95ed0e7c1f
commit 20ca916d7d
1 changed files with 51 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

108
ssl/s3_srvr.c
View File

@ -2226,9 +2226,8 @@ int ssl3_get_client_key_exchange(SSL *s)
EC_POINT *clnt_ecpoint = NULL; EC_POINT *clnt_ecpoint = NULL;
BN_CTX *bn_ctx = NULL; BN_CTX *bn_ctx = NULL;
#endif #endif
PACKET pkt; PACKET pkt, enc_premaster;
unsigned char *data; unsigned char *data, *rsa_decrypt = NULL;
size_t remain;
n = s->method->ssl_get_message(s, n = s->method->ssl_get_message(s,
SSL3_ST_SR_KEY_EXCH_A, SSL3_ST_SR_KEY_EXCH_A,
@ -2353,59 +2352,44 @@ int ssl3_get_client_key_exchange(SSL *s)
rsa = pkey->pkey.rsa; rsa = pkey->pkey.rsa;
} }
/* TLS and [incidentally] DTLS{0xFEFF} */ /* SSLv3 and pre-standard DTLS omit the length bytes. */
if (s->version > SSL3_VERSION && s->version != DTLS1_BAD_VER) { if (s->version == SSL3_VERSION || s->version == DTLS1_BAD_VER) {
if (!PACKET_get_net_2(&pkt, &i)) { enc_premaster = pkt;
al = SSL_AD_DECODE_ERROR; } else {
SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH); PACKET orig = pkt;
goto f_err; if (!PACKET_get_length_prefixed_2(&pkt, &enc_premaster)
} || PACKET_remaining(&pkt) != 0) {
remain = PACKET_remaining(&pkt); /* Try SSLv3 behaviour for TLS. */
if (remain != i) { if (s->options & SSL_OP_TLS_D5_BUG) {
if (!(s->options & SSL_OP_TLS_D5_BUG)) { enc_premaster = orig;
al = SSL_AD_DECODE_ERROR;
SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,
SSL_R_TLS_RSA_ENCRYPTED_VALUE_LENGTH_IS_WRONG);
goto f_err;
} else { } else {
remain += 2; al = SSL_AD_DECODE_ERROR;
if (!PACKET_back(&pkt, 2)) { SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH);
/* goto f_err;
* We already read these 2 bytes so this should never
* fail
*/
al = SSL_AD_INTERNAL_ERROR;
SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,
ERR_R_INTERNAL_ERROR);
goto f_err;
}
} }
} }
} else {
remain = PACKET_remaining(&pkt);
} }
/* /*
* Reject overly short RSA ciphertext because we want to be sure * We want to be sure that the plaintext buffer size makes it safe to
* that the buffer size makes it safe to iterate over the entire * iterate over the entire size of a premaster secret
* size of a premaster secret (SSL_MAX_MASTER_KEY_LENGTH). The * (SSL_MAX_MASTER_KEY_LENGTH). Reject overly short RSA keys because
* actual expected size is larger due to RSA padding, but the * their ciphertext cannot accommodate a premaster secret anyway.
* bound is sufficient to be safe.
*/ */
if (RSA_size(rsa) < SSL_MAX_MASTER_KEY_LENGTH) {
if (remain < SSL_MAX_MASTER_KEY_LENGTH) {
al = SSL_AD_DECRYPT_ERROR;
SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,
SSL_R_TLS_RSA_ENCRYPTED_VALUE_LENGTH_IS_WRONG);
goto f_err;
}
if (!PACKET_get_bytes(&pkt, &data, remain)) {
/* We already checked we had enough data so this shouldn't happen */
al = SSL_AD_INTERNAL_ERROR; al = SSL_AD_INTERNAL_ERROR;
SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,
RSA_R_KEY_SIZE_TOO_SMALL);
goto f_err; goto f_err;
} }
rsa_decrypt = OPENSSL_malloc(RSA_size(rsa));
if (rsa_decrypt == NULL) {
al = SSL_AD_INTERNAL_ERROR;
SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE);
goto f_err;
}
/* /*
* We must not leak whether a decryption failure occurs because of * We must not leak whether a decryption failure occurs because of
* Bleichenbacher's attack on PKCS #1 v1.5 RSA padding (see RFC 2246, * Bleichenbacher's attack on PKCS #1 v1.5 RSA padding (see RFC 2246,
@ -2415,10 +2399,13 @@ int ssl3_get_client_key_exchange(SSL *s)
*/ */
if (RAND_bytes(rand_premaster_secret, if (RAND_bytes(rand_premaster_secret,
sizeof(rand_premaster_secret)) <= 0) sizeof(rand_premaster_secret)) <= 0) {
goto err; goto err;
decrypt_len = }
RSA_private_decrypt(remain, data, data, rsa, RSA_PKCS1_PADDING);
decrypt_len = RSA_private_decrypt(PACKET_remaining(&enc_premaster),
PACKET_data(&enc_premaster),
rsa_decrypt, rsa, RSA_PKCS1_PADDING);
ERR_clear_error(); ERR_clear_error();
/* /*
@ -2437,9 +2424,11 @@ int ssl3_get_client_key_exchange(SSL *s)
* constant time and are treated like any other decryption error. * constant time and are treated like any other decryption error.
*/ */
version_good = version_good =
constant_time_eq_8(data[0], (unsigned)(s->client_version >> 8)); constant_time_eq_8(rsa_decrypt[0],
(unsigned)(s->client_version >> 8));
version_good &= version_good &=
constant_time_eq_8(data[1], (unsigned)(s->client_version & 0xff)); constant_time_eq_8(rsa_decrypt[1],
(unsigned)(s->client_version & 0xff));
/* /*
* The premaster secret must contain the same version number as the * The premaster secret must contain the same version number as the
@ -2453,9 +2442,10 @@ int ssl3_get_client_key_exchange(SSL *s)
if (s->options & SSL_OP_TLS_ROLLBACK_BUG) { if (s->options & SSL_OP_TLS_ROLLBACK_BUG) {
unsigned char workaround_good; unsigned char workaround_good;
workaround_good = workaround_good =
constant_time_eq_8(data[0], (unsigned)(s->version >> 8)); constant_time_eq_8(rsa_decrypt[0], (unsigned)(s->version >> 8));
workaround_good &= workaround_good &=
constant_time_eq_8(data[1], (unsigned)(s->version & 0xff)); constant_time_eq_8(rsa_decrypt[1],
(unsigned)(s->version & 0xff));
version_good |= workaround_good; version_good |= workaround_good;
} }
@ -2472,16 +2462,19 @@ int ssl3_get_client_key_exchange(SSL *s)
* it is still sufficiently large to read from. * it is still sufficiently large to read from.
*/ */
for (j = 0; j < sizeof(rand_premaster_secret); j++) { for (j = 0; j < sizeof(rand_premaster_secret); j++) {
data[j] = constant_time_select_8(decrypt_good, data[j], rsa_decrypt[j] =
rand_premaster_secret[j]); constant_time_select_8(decrypt_good, rsa_decrypt[j],
rand_premaster_secret[j]);
} }
if (!ssl_generate_master_secret(s, data, sizeof(rand_premaster_secret), if (!ssl_generate_master_secret(s, rsa_decrypt,
0)) { sizeof(rand_premaster_secret), 0)) {
al = SSL_AD_INTERNAL_ERROR; al = SSL_AD_INTERNAL_ERROR;
SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
goto f_err; goto f_err;
} }
OPENSSL_free(rsa_decrypt);
rsa_decrypt = NULL;
} else } else
#endif #endif
#ifndef OPENSSL_NO_DH #ifndef OPENSSL_NO_DH
@ -2852,6 +2845,7 @@ int ssl3_get_client_key_exchange(SSL *s)
EC_POINT_free(clnt_ecpoint); EC_POINT_free(clnt_ecpoint);
EC_KEY_free(srvr_ecdh); EC_KEY_free(srvr_ecdh);
BN_CTX_free(bn_ctx); BN_CTX_free(bn_ctx);
OPENSSL_free(rsa_decrypt);
#endif #endif
#ifndef OPENSSL_NO_PSK #ifndef OPENSSL_NO_PSK
OPENSSL_clear_free(s->s3->tmp.psk, s->s3->tmp.psklen); OPENSSL_clear_free(s->s3->tmp.psk, s->s3->tmp.psklen);