From 5b8fa431ae8eb5a18ba913494119e394230d4b70 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 16 Jun 2016 14:15:19 -0400 Subject: [PATCH] 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 Reviewed-by: Rich Salz GH: #1222 --- ssl/statem/statem_srvr.c | 51 ++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index f88b6c8194..a88b3219ad 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); -- 2.25.1