Simplify RSA key exchange padding check.
This check was fixed a while ago, but it could have been much simpler.
In the RSA key exchange, the expected size of the output is known, making the
padding check much simpler. There isn't any use in exporting the more general
RSA_message_index_PKCS1_type_2. (Without knowing the expected size, any
integrity check or swap to randomness or other mitigation is basically doomed
to fail.)
Verified with the valgrind uninitialized memory trick that we're still
constant-time.
Also update rsa.h to recommend against using the PKCS#1 v1.5 schemes.
Thanks to Ryan Sleevi for the suggestion.
Change-Id: I4328076b1d2e5e06617dd8907cdaa702635c2651
Reviewed-on: https://boringssl-review.googlesource.com/6613
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/rsa/padding.c b/crypto/rsa/padding.c
index 274c113..032df2e 100644
--- a/crypto/rsa/padding.c
+++ b/crypto/rsa/padding.c
@@ -70,17 +70,17 @@
/* TODO(fork): don't the check functions have to be constant time? */
-int RSA_padding_add_PKCS1_type_1(uint8_t *to, unsigned tlen,
- const uint8_t *from, unsigned flen) {
+int RSA_padding_add_PKCS1_type_1(uint8_t *to, unsigned to_len,
+ const uint8_t *from, unsigned from_len) {
unsigned j;
uint8_t *p;
- if (tlen < RSA_PKCS1_PADDING_SIZE) {
+ if (to_len < RSA_PKCS1_PADDING_SIZE) {
OPENSSL_PUT_ERROR(RSA, RSA_R_KEY_SIZE_TOO_SMALL);
return 0;
}
- if (flen > tlen - RSA_PKCS1_PADDING_SIZE) {
+ if (from_len > to_len - RSA_PKCS1_PADDING_SIZE) {
OPENSSL_PUT_ERROR(RSA, RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE);
return 0;
}
@@ -91,20 +91,20 @@
*(p++) = 1; /* Private Key BT (Block Type) */
/* pad out with 0xff data */
- j = tlen - 3 - flen;
+ j = to_len - 3 - from_len;
memset(p, 0xff, j);
p += j;
*(p++) = 0;
- memcpy(p, from, (unsigned int)flen);
+ memcpy(p, from, (unsigned int)from_len);
return 1;
}
-int RSA_padding_check_PKCS1_type_1(uint8_t *to, unsigned tlen,
- const uint8_t *from, unsigned flen) {
+int RSA_padding_check_PKCS1_type_1(uint8_t *to, unsigned to_len,
+ const uint8_t *from, unsigned from_len) {
unsigned i, j;
const uint8_t *p;
- if (flen < 2) {
+ if (from_len < 2) {
OPENSSL_PUT_ERROR(RSA, RSA_R_DATA_TOO_SMALL);
return -1;
}
@@ -116,7 +116,7 @@
}
/* scan over padding data */
- j = flen - 2; /* one for leading 00, one for type. */
+ j = from_len - 2; /* one for leading 00, one for type. */
for (i = 0; i < j; i++) {
/* should decrypt to 0xff */
if (*p != 0xff) {
@@ -142,7 +142,7 @@
}
i++; /* Skip over the '\0' */
j -= i;
- if (j > tlen) {
+ if (j > to_len) {
OPENSSL_PUT_ERROR(RSA, RSA_R_DATA_TOO_LARGE);
return -1;
}
@@ -151,17 +151,17 @@
return j;
}
-int RSA_padding_add_PKCS1_type_2(uint8_t *to, unsigned tlen,
- const uint8_t *from, unsigned flen) {
+int RSA_padding_add_PKCS1_type_2(uint8_t *to, unsigned to_len,
+ const uint8_t *from, unsigned from_len) {
unsigned i, j;
uint8_t *p;
- if (tlen < RSA_PKCS1_PADDING_SIZE) {
+ if (to_len < RSA_PKCS1_PADDING_SIZE) {
OPENSSL_PUT_ERROR(RSA, RSA_R_KEY_SIZE_TOO_SMALL);
return 0;
}
- if (flen > tlen - RSA_PKCS1_PADDING_SIZE) {
+ if (from_len > to_len - RSA_PKCS1_PADDING_SIZE) {
OPENSSL_PUT_ERROR(RSA, RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE);
return 0;
}
@@ -172,7 +172,7 @@
*(p++) = 2; /* Public Key BT (Block Type) */
/* pad out with non-zero random data */
- j = tlen - 3 - flen;
+ j = to_len - 3 - from_len;
if (!RAND_bytes(p, j)) {
return 0;
@@ -189,30 +189,30 @@
*(p++) = 0;
- memcpy(p, from, (unsigned int)flen);
+ memcpy(p, from, (unsigned int)from_len);
return 1;
}
-int RSA_message_index_PKCS1_type_2(const uint8_t *from, size_t from_len,
- size_t *out_index) {
- size_t i;
- unsigned first_byte_is_zero, second_byte_is_two, looking_for_index;
- unsigned valid_index, zero_index = 0;
+int RSA_padding_check_PKCS1_type_2(uint8_t *to, unsigned to_len,
+ const uint8_t *from, unsigned from_len) {
+ if (from_len == 0) {
+ OPENSSL_PUT_ERROR(RSA, RSA_R_EMPTY_PUBLIC_KEY);
+ return -1;
+ }
/* PKCS#1 v1.5 decryption. See "PKCS #1 v2.2: RSA Cryptography
* Standard", section 7.2.2. */
- if (from_len < RSA_PKCS1_PADDING_SIZE || from_len > UINT_MAX) {
+ if (from_len < RSA_PKCS1_PADDING_SIZE) {
/* |from| is zero-padded to the size of the RSA modulus, a public value, so
- * this can be rejected in non-constant time. This logic also requires
- * |from_len| fit in an |unsigned|. */
- *out_index = 0;
- return 0;
+ * this can be rejected in non-constant time. */
+ OPENSSL_PUT_ERROR(RSA, RSA_R_KEY_SIZE_TOO_SMALL);
+ return -1;
}
- first_byte_is_zero = constant_time_eq(from[0], 0);
- second_byte_is_two = constant_time_eq(from[1], 2);
+ unsigned first_byte_is_zero = constant_time_eq(from[0], 0);
+ unsigned second_byte_is_two = constant_time_eq(from[1], 2);
- looking_for_index = ~0u;
+ unsigned i, zero_index = 0, looking_for_index = ~0u;
for (i = 2; i < from_len; i++) {
unsigned equals0 = constant_time_is_zero(from[i]);
zero_index = constant_time_select(looking_for_index & equals0, (unsigned)i,
@@ -221,7 +221,7 @@
}
/* The input must begin with 00 02. */
- valid_index = first_byte_is_zero;
+ unsigned valid_index = first_byte_is_zero;
valid_index &= second_byte_is_two;
/* We must have found the end of PS. */
@@ -233,51 +233,45 @@
/* Skip the zero byte. */
zero_index++;
- *out_index = constant_time_select(valid_index, zero_index, 0);
- return constant_time_select(valid_index, 1, 0);
-}
-
-int RSA_padding_check_PKCS1_type_2(uint8_t *to, unsigned tlen,
- const uint8_t *from, unsigned flen) {
- size_t msg_index, msg_len;
-
- if (flen == 0) {
- OPENSSL_PUT_ERROR(RSA, RSA_R_EMPTY_PUBLIC_KEY);
- return -1;
- }
-
- /* NOTE: Although |RSA_message_index_PKCS1_type_2| itself is constant time,
- * the API contracts of this function and |RSA_decrypt| with
- * |RSA_PKCS1_PADDING| make it impossible to completely avoid Bleichenbacher's
- * attack. */
- if (!RSA_message_index_PKCS1_type_2(from, flen, &msg_index)) {
+ /* NOTE: Although this logic attempts to be constant time, the API contracts
+ * of this function and |RSA_decrypt| with |RSA_PKCS1_PADDING| make it
+ * impossible to completely avoid Bleichenbacher's attack. Consumers should
+ * use |RSA_unpad_key_pkcs1|. */
+ if (!valid_index) {
OPENSSL_PUT_ERROR(RSA, RSA_R_PKCS_DECODING_ERROR);
return -1;
}
- msg_len = flen - msg_index;
- if (msg_len > tlen) {
- /* This shouldn't happen because this function is always called with |tlen|
- * the key size and |flen| is bounded by the key size. */
+ const unsigned msg_len = from_len - zero_index;
+ if (msg_len > to_len) {
+ /* This shouldn't happen because this function is always called with
+ * |to_len| as the key size and |from_len| is bounded by the key size. */
OPENSSL_PUT_ERROR(RSA, RSA_R_PKCS_DECODING_ERROR);
return -1;
}
- memcpy(to, &from[msg_index], msg_len);
- return msg_len;
+
+ if (msg_len > INT_MAX) {
+ OPENSSL_PUT_ERROR(RSA, ERR_R_OVERFLOW);
+ return -1;
+ }
+
+ memcpy(to, &from[zero_index], msg_len);
+ return (int)msg_len;
}
-int RSA_padding_add_none(uint8_t *to, unsigned tlen, const uint8_t *from, unsigned flen) {
- if (flen > tlen) {
+int RSA_padding_add_none(uint8_t *to, unsigned to_len, const uint8_t *from,
+ unsigned from_len) {
+ if (from_len > to_len) {
OPENSSL_PUT_ERROR(RSA, RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE);
return 0;
}
- if (flen < tlen) {
+ if (from_len < to_len) {
OPENSSL_PUT_ERROR(RSA, RSA_R_DATA_TOO_SMALL_FOR_KEY_SIZE);
return 0;
}
- memcpy(to, from, (unsigned int)flen);
+ memcpy(to, from, (unsigned int)from_len);
return 1;
}
@@ -300,7 +294,8 @@
cnt[2] = (uint8_t)((i >> 8)) & 255;
cnt[3] = (uint8_t)(i & 255);
if (!EVP_DigestInit_ex(&c, dgst, NULL) ||
- !EVP_DigestUpdate(&c, seed, seedlen) || !EVP_DigestUpdate(&c, cnt, 4)) {
+ !EVP_DigestUpdate(&c, seed, seedlen) ||
+ !EVP_DigestUpdate(&c, cnt, 4)) {
goto err;
}
@@ -324,9 +319,9 @@
return ret;
}
-int RSA_padding_add_PKCS1_OAEP_mgf1(uint8_t *to, unsigned tlen,
- const uint8_t *from, unsigned flen,
- const uint8_t *param, unsigned plen,
+int RSA_padding_add_PKCS1_OAEP_mgf1(uint8_t *to, unsigned to_len,
+ const uint8_t *from, unsigned from_len,
+ const uint8_t *param, unsigned param_len,
const EVP_MD *md, const EVP_MD *mgf1md) {
unsigned i, emlen, mdlen;
uint8_t *db, *seed;
@@ -342,13 +337,13 @@
mdlen = EVP_MD_size(md);
- if (tlen < 2 * mdlen + 2) {
+ if (to_len < 2 * mdlen + 2) {
OPENSSL_PUT_ERROR(RSA, RSA_R_KEY_SIZE_TOO_SMALL);
return 0;
}
- emlen = tlen - 1;
- if (flen > emlen - 2 * mdlen - 1) {
+ emlen = to_len - 1;
+ if (from_len > emlen - 2 * mdlen - 1) {
OPENSSL_PUT_ERROR(RSA, RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE);
return 0;
}
@@ -362,12 +357,12 @@
seed = to + 1;
db = to + mdlen + 1;
- if (!EVP_Digest((void *)param, plen, db, NULL, md, NULL)) {
+ if (!EVP_Digest((void *)param, param_len, db, NULL, md, NULL)) {
return 0;
}
- memset(db + mdlen, 0, emlen - flen - 2 * mdlen - 1);
- db[emlen - flen - mdlen - 1] = 0x01;
- memcpy(db + emlen - flen - mdlen, from, flen);
+ memset(db + mdlen, 0, emlen - from_len - 2 * mdlen - 1);
+ db[emlen - from_len - mdlen - 1] = 0x01;
+ memcpy(db + emlen - from_len - mdlen, from, from_len);
if (!RAND_bytes(seed, mdlen)) {
return 0;
}
@@ -398,9 +393,9 @@
return ret;
}
-int RSA_padding_check_PKCS1_OAEP_mgf1(uint8_t *to, unsigned tlen,
- const uint8_t *from, unsigned flen,
- const uint8_t *param, unsigned plen,
+int RSA_padding_check_PKCS1_OAEP_mgf1(uint8_t *to, unsigned to_len,
+ const uint8_t *from, unsigned from_len,
+ const uint8_t *param, unsigned param_len,
const EVP_MD *md, const EVP_MD *mgf1md) {
unsigned i, dblen, mlen = -1, mdlen, bad, looking_for_one_byte, one_index = 0;
const uint8_t *maskeddb, *maskedseed;
@@ -418,13 +413,13 @@
/* The encoded message is one byte smaller than the modulus to ensure that it
* doesn't end up greater than the modulus. Thus there's an extra "+1" here
* compared to https://tools.ietf.org/html/rfc2437#section-9.1.1.2. */
- if (flen < 1 + 2*mdlen + 1) {
- /* 'flen' is the length of the modulus, i.e. does not depend on the
+ if (from_len < 1 + 2*mdlen + 1) {
+ /* 'from_len' is the length of the modulus, i.e. does not depend on the
* particular ciphertext. */
goto decoding_err;
}
- dblen = flen - mdlen - 1;
+ dblen = from_len - mdlen - 1;
db = OPENSSL_malloc(dblen);
if (db == NULL) {
OPENSSL_PUT_ERROR(RSA, ERR_R_MALLOC_FAILURE);
@@ -448,7 +443,7 @@
db[i] ^= maskeddb[i];
}
- if (!EVP_Digest((void *)param, plen, phash, NULL, md, NULL)) {
+ if (!EVP_Digest((void *)param, param_len, phash, NULL, md, NULL)) {
goto err;
}
@@ -459,7 +454,8 @@
for (i = mdlen; i < dblen; i++) {
unsigned equals1 = constant_time_eq(db[i], 1);
unsigned equals0 = constant_time_eq(db[i], 0);
- one_index = constant_time_select(looking_for_one_byte & equals1, i, one_index);
+ one_index = constant_time_select(looking_for_one_byte & equals1, i,
+ one_index);
looking_for_one_byte =
constant_time_select(equals1, 0, looking_for_one_byte);
bad |= looking_for_one_byte & ~equals0;
@@ -473,7 +469,7 @@
one_index++;
mlen = dblen - one_index;
- if (tlen < mlen) {
+ if (to_len < mlen) {
OPENSSL_PUT_ERROR(RSA, RSA_R_DATA_TOO_LARGE);
mlen = -1;
} else {
diff --git a/include/openssl/rsa.h b/include/openssl/rsa.h
index 7f8cfe3..304c555 100644
--- a/include/openssl/rsa.h
+++ b/include/openssl/rsa.h
@@ -124,8 +124,8 @@
* It returns 1 on success or zero on error.
*
* The |padding| argument must be one of the |RSA_*_PADDING| values. If in
- * doubt, |RSA_PKCS1_PADDING| is the most common but |RSA_PKCS1_OAEP_PADDING|
- * is the most secure. */
+ * doubt, use |RSA_PKCS1_OAEP_PADDING| for new protocols but
+ * |RSA_PKCS1_PADDING| is most common. */
OPENSSL_EXPORT int RSA_encrypt(RSA *rsa, size_t *out_len, uint8_t *out,
size_t max_out, const uint8_t *in, size_t in_len,
int padding);
@@ -137,8 +137,14 @@
* It returns 1 on success or zero on error.
*
* The |padding| argument must be one of the |RSA_*_PADDING| values. If in
- * doubt, |RSA_PKCS1_PADDING| is the most common but |RSA_PKCS1_OAEP_PADDING|
- * is the most secure. */
+ * doubt, use |RSA_PKCS1_OAEP_PADDING| for new protocols.
+ *
+ * Passing |RSA_PKCS1_PADDING| into this function is deprecated and insecure. If
+ * implementing a protocol using RSAES-PKCS1-V1_5, use |RSA_NO_PADDING| and then
+ * check padding in constant-time combined with a swap to a random session key
+ * or other mitigation. See "Chosen Ciphertext Attacks Against Protocols Based
+ * on the RSA Encryption Standard PKCS #1", Daniel Bleichenbacher, Advances in
+ * Cryptology (Crypto '98). */
OPENSSL_EXPORT int RSA_decrypt(RSA *rsa, size_t *out_len, uint8_t *out,
size_t max_out, const uint8_t *in, size_t in_len,
int padding);
@@ -147,8 +153,8 @@
* |rsa| and writes the encrypted data to |to|. The |to| buffer must have at
* least |RSA_size| bytes of space. It returns the number of bytes written, or
* -1 on error. The |padding| argument must be one of the |RSA_*_PADDING|
- * values. If in doubt, |RSA_PKCS1_PADDING| is the most common but
- * |RSA_PKCS1_OAEP_PADDING| is the most secure.
+ * values. If in doubt, use |RSA_PKCS1_OAEP_PADDING| for new protocols but
+ * |RSA_PKCS1_PADDING| is most common.
*
* WARNING: this function is dangerous because it breaks the usual return value
* convention. Use |RSA_encrypt| instead. */
@@ -156,37 +162,25 @@
uint8_t *to, RSA *rsa, int padding);
/* RSA_private_decrypt decrypts |flen| bytes from |from| with the public key in
- * |rsa| and writes the plaintext to |to|. The |to| buffer must have at
- * least |RSA_size| bytes of space. It returns the number of bytes written, or
- * -1 on error. The |padding| argument must be one of the |RSA_*_PADDING|
- * values. If in doubt, |RSA_PKCS1_PADDING| is the most common but
- * |RSA_PKCS1_OAEP_PADDING| is the most secure.
+ * |rsa| and writes the plaintext to |to|. The |to| buffer must have at least
+ * |RSA_size| bytes of space. It returns the number of bytes written, or -1 on
+ * error. The |padding| argument must be one of the |RSA_*_PADDING| values. If
+ * in doubt, use |RSA_PKCS1_OAEP_PADDING| for new protocols. Passing
+ * |RSA_PKCS1_PADDING| into this function is deprecated and insecure. See
+ * |RSA_decrypt|.
*
* WARNING: this function is dangerous because it breaks the usual return value
* convention. Use |RSA_decrypt| instead. */
OPENSSL_EXPORT int RSA_private_decrypt(size_t flen, const uint8_t *from,
uint8_t *to, RSA *rsa, int padding);
-/* RSA_message_index_PKCS1_type_2 performs the first step of a PKCS #1 padding
- * check for decryption. If the |from_len| bytes pointed to at |from| are a
- * valid PKCS #1 message, it returns one and sets |*out_index| to the start of
- * the unpadded message. The unpadded message is a suffix of the input and has
- * length |from_len - *out_index|. Otherwise, it returns zero and sets
- * |*out_index| to zero. This function runs in time independent of the input
- * data and is intended to be used directly to avoid Bleichenbacher's attack.
- *
- * WARNING: This function behaves differently from the usual OpenSSL convention
- * in that it does NOT put an error on the queue in the error case. */
-OPENSSL_EXPORT int RSA_message_index_PKCS1_type_2(const uint8_t *from,
- size_t from_len,
- size_t *out_index);
-
/* Signing / Verification */
-/* RSA_sign signs |in_len| bytes of digest from |in| with |rsa| and writes, at
- * most, |RSA_size(rsa)| bytes to |out|. On successful return, the actual
- * number of bytes written is written to |*out_len|.
+/* RSA_sign signs |in_len| bytes of digest from |in| with |rsa| using
+ * RSASSA-PKCS1-v1_5. It writes, at most, |RSA_size(rsa)| bytes to |out|. On
+ * successful return, the actual number of bytes written is written to
+ * |*out_len|.
*
* The |hash_nid| argument identifies the hash function used to calculate |in|
* and is embedded in the resulting signature. For example, it might be
@@ -204,13 +198,14 @@
* It returns 1 on success or zero on error.
*
* The |padding| argument must be one of the |RSA_*_PADDING| values. If in
- * doubt, |RSA_PKCS1_PADDING| is the most common. */
+ * doubt, |RSA_PKCS1_PADDING| is the most common but |RSA_PKCS1_PSS_PADDING|
+ * (via the |EVP_PKEY| interface) is preferred for new protocols. */
OPENSSL_EXPORT int RSA_sign_raw(RSA *rsa, size_t *out_len, uint8_t *out,
size_t max_out, const uint8_t *in,
size_t in_len, int padding);
-/* RSA_verify verifies that |sig_len| bytes from |sig| are a valid, PKCS#1
- * signature of |msg_len| bytes at |msg| by |rsa|.
+/* RSA_verify verifies that |sig_len| bytes from |sig| are a valid,
+ * RSASSA-PKCS1-v1_5 signature of |msg_len| bytes at |msg| by |rsa|.
*
* The |hash_nid| argument identifies the hash function used to calculate |in|
* and is embedded in the resulting signature in order to prevent hash
@@ -231,7 +226,8 @@
* It returns 1 on success or zero on error.
*
* The |padding| argument must be one of the |RSA_*_PADDING| values. If in
- * doubt, |RSA_PKCS1_PADDING| is the most common. */
+ * doubt, |RSA_PKCS1_PADDING| is the most common but |RSA_PKCS1_PSS_PADDING|
+ * (via the |EVP_PKEY| interface) is preferred for new protocols. */
OPENSSL_EXPORT int RSA_verify_raw(RSA *rsa, size_t *out_len, uint8_t *out,
size_t max_out, const uint8_t *in,
size_t in_len, int padding);
@@ -240,7 +236,9 @@
* |rsa| and writes the encrypted data to |to|. The |to| buffer must have at
* least |RSA_size| bytes of space. It returns the number of bytes written, or
* -1 on error. The |padding| argument must be one of the |RSA_*_PADDING|
- * values. If in doubt, |RSA_PKCS1_PADDING| is the most common.
+ * values. If in doubt, |RSA_PKCS1_PADDING| is the most common but
+ * |RSA_PKCS1_PSS_PADDING| (via the |EVP_PKEY| interface) is preferred for new
+ * protocols.
*
* WARNING: this function is dangerous because it breaks the usual return value
* convention. Use |RSA_sign_raw| instead. */
@@ -251,7 +249,9 @@
* public key in |rsa| and writes the plaintext to |to|. The |to| buffer must
* have at least |RSA_size| bytes of space. It returns the number of bytes
* written, or -1 on error. The |padding| argument must be one of the
- * |RSA_*_PADDING| values. If in doubt, |RSA_PKCS1_PADDING| is the most common.
+ * |RSA_*_PADDING| values. If in doubt, |RSA_PKCS1_PADDING| is the most common
+ * but |RSA_PKCS1_PSS_PADDING| (via the |EVP_PKEY| interface) is preferred for
+ * new protocols.
*
* WARNING: this function is dangerous because it breaks the usual return value
* convention. Use |RSA_verify_raw| instead. */
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index a3130f9..cbb1418 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -1654,13 +1654,8 @@
/* Depending on the key exchange method, compute |premaster_secret| and
* |premaster_secret_len|. */
if (alg_k & SSL_kRSA) {
- CBS encrypted_premaster_secret;
- uint8_t rand_premaster_secret[SSL_MAX_MASTER_KEY_LENGTH];
- uint8_t good;
- size_t decrypt_len, premaster_index, j;
- const size_t rsa_size = ssl_private_key_max_signature_len(s);
-
/* Allocate a buffer large enough for an RSA decryption. */
+ const size_t rsa_size = ssl_private_key_max_signature_len(s);
decrypt_buf = OPENSSL_malloc(rsa_size);
if (decrypt_buf == NULL) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
@@ -1668,13 +1663,14 @@
}
enum ssl_private_key_result_t decrypt_result;
+ size_t decrypt_len;
if (s->state == SSL3_ST_SR_KEY_EXCH_B) {
if (!ssl_has_private_key(s) || ssl_private_key_type(s) != EVP_PKEY_RSA) {
al = SSL_AD_HANDSHAKE_FAILURE;
OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_RSA_CERTIFICATE);
goto f_err;
}
- /* TLS and [incidentally] DTLS{0xFEFF} */
+ CBS encrypted_premaster_secret;
if (s->version > SSL3_VERSION) {
if (!CBS_get_u16_length_prefixed(&client_key_exchange,
&encrypted_premaster_secret) ||
@@ -1688,16 +1684,6 @@
encrypted_premaster_secret = client_key_exchange;
}
- /* Reject overly short RSA keys because we want to be sure that the buffer
- * size makes it safe to iterate over the entire size of a premaster
- * secret (SSL_MAX_MASTER_KEY_LENGTH). The actual expected size is larger
- * due to RSA padding, but the bound is sufficient to be safe. */
- if (rsa_size < SSL_MAX_MASTER_KEY_LENGTH) {
- al = SSL_AD_DECRYPT_ERROR;
- OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED);
- goto f_err;
- }
-
/* Decrypt with no padding. PKCS#1 padding will be removed as part of the
* timing-sensitive code below. */
decrypt_result = ssl_private_key_decrypt(
@@ -1724,66 +1710,54 @@
goto err;
}
- if (decrypt_len != rsa_size) {
- /* This should never happen, but do a check so we do not read
- * uninitialized memory. */
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- goto err;
- }
+ assert(decrypt_len == rsa_size);
- /* Remove the PKCS#1 padding and adjust |decrypt_len| as appropriate.
- * |good| will be 0xff if the premaster is acceptable and zero otherwise.
- * */
- good =
- constant_time_eq_int_8(RSA_message_index_PKCS1_type_2(
- decrypt_buf, decrypt_len, &premaster_index),
- 1);
- decrypt_len = decrypt_len - premaster_index;
-
- /* decrypt_len should be SSL_MAX_MASTER_KEY_LENGTH. */
- good &= constant_time_eq_8(decrypt_len, SSL_MAX_MASTER_KEY_LENGTH);
-
- /* Copy over the unpadded premaster. Whatever the value of
- * |decrypt_good_mask|, copy as if the premaster were the right length. It
- * is important the memory access pattern be constant. */
- premaster_secret =
- BUF_memdup(decrypt_buf + (rsa_size - SSL_MAX_MASTER_KEY_LENGTH),
- SSL_MAX_MASTER_KEY_LENGTH);
+ /* Prepare a random premaster, to be used on invalid padding. See RFC 5246,
+ * section 7.4.7.1. */
+ premaster_secret_len = SSL_MAX_MASTER_KEY_LENGTH;
+ premaster_secret = OPENSSL_malloc(premaster_secret_len);
if (premaster_secret == NULL) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
goto err;
}
- OPENSSL_free(decrypt_buf);
- decrypt_buf = NULL;
-
- /* If the version in the decrypted pre-master secret is correct then
- * version_good will be 0xff, otherwise it'll be zero. The
- * Klima-Pokorny-Rosa extension of Bleichenbacher's attack
- * (http://eprint.iacr.org/2003/052/) exploits the version number check as
- * a "bad version oracle". Thus version checks are done in constant time
- * and are treated like any other decryption error. */
- good &= constant_time_eq_8(premaster_secret[0],
- (unsigned)(s->client_version >> 8));
- good &= constant_time_eq_8(premaster_secret[1],
- (unsigned)(s->client_version & 0xff));
-
- /* We must not leak whether a decryption failure occurs because of
- * Bleichenbacher's attack on PKCS #1 v1.5 RSA padding (see RFC 2246,
- * section 7.4.7.1). The code follows that advice of the TLS RFC and
- * generates a random premaster secret for the case that the decrypt
- * fails. See https://tools.ietf.org/html/rfc5246#section-7.4.7.1 */
- if (!RAND_bytes(rand_premaster_secret, sizeof(rand_premaster_secret))) {
+ if (!RAND_bytes(premaster_secret, premaster_secret_len)) {
goto err;
}
- /* Now copy rand_premaster_secret over premaster_secret using
- * decrypt_good_mask. */
- for (j = 0; j < sizeof(rand_premaster_secret); j++) {
- premaster_secret[j] = constant_time_select_8(good, premaster_secret[j],
- rand_premaster_secret[j]);
+ /* The smallest padded premaster is 11 bytes of overhead. Small keys are
+ * publicly invalid. */
+ if (decrypt_len < 11 + premaster_secret_len) {
+ al = SSL_AD_DECRYPT_ERROR;
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED);
+ goto f_err;
}
- premaster_secret_len = sizeof(rand_premaster_secret);
+ /* Check the padding. See RFC 3447, section 7.2.2. */
+ size_t padding_len = decrypt_len - premaster_secret_len;
+ uint8_t good = constant_time_eq_int_8(decrypt_buf[0], 0) &
+ constant_time_eq_int_8(decrypt_buf[1], 2);
+ size_t i;
+ for (i = 2; i < padding_len - 1; i++) {
+ good &= ~constant_time_is_zero_8(decrypt_buf[i]);
+ }
+ good &= constant_time_is_zero_8(decrypt_buf[padding_len - 1]);
+
+ /* The premaster secret must begin with |client_version|. This too must be
+ * checked in constant time (http://eprint.iacr.org/2003/052/). */
+ good &= constant_time_eq_8(decrypt_buf[padding_len],
+ (unsigned)(s->client_version >> 8));
+ good &= constant_time_eq_8(decrypt_buf[padding_len + 1],
+ (unsigned)(s->client_version & 0xff));
+
+ /* Select, in constant time, either the decrypted premaster or the random
+ * premaster based on |good|. */
+ for (i = 0; i < premaster_secret_len; i++) {
+ premaster_secret[i] = constant_time_select_8(
+ good, decrypt_buf[padding_len + i], premaster_secret[i]);
+ }
+
+ OPENSSL_free(decrypt_buf);
+ decrypt_buf = NULL;
} else if (alg_k & SSL_kDHE) {
CBS dh_Yc;
int dh_len;
