Premaster secret handling fixes

From BoringSSL
- Send an alert when the client key exchange isn't correctly formatted.
- Reject overly short RSA ciphertexts to avoid a (benign) out-of-bounds memory access.

Reviewed-by: Kurt Roeckx <kurt@openssl.org>

diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 7a61864bb6a86419c91c3817d5513cb4f7ed3e6d..02c8c10aa1629776a0e599c904129811d18648d5 100644 (file)
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -2276,6 +2276,7 @@ int ssl3_get_client_key_exchange(SSL *s)
                unsigned char rand_premaster_secret[SSL_MAX_MASTER_KEY_LENGTH];
                int decrypt_len;
                unsigned char decrypt_good, version_good;
+               size_t j;
 
                /* FIX THIS UP EAY EAY EAY EAY */
                if (s->s3->tmp.use_rsa_tmp)
@@ -2314,8 +2315,9 @@ int ssl3_get_client_key_exchange(SSL *s)
                                {
                                if (!(s->options & SSL_OP_TLS_D5_BUG))
                                        {
+                                       al = SSL_AD_DECODE_ERROR;
                                        SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,SSL_R_TLS_RSA_ENCRYPTED_VALUE_LENGTH_IS_WRONG);
-                                       goto err;
+                                       goto f_err;
                                        }
                                else
                                        p-=2;
@@ -2324,6 +2326,20 @@ int ssl3_get_client_key_exchange(SSL *s)
                                n=i;
                        }
 
+               /*
+                * Reject overly short RSA ciphertext 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 (n < 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;
+                       }
+
                /* 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
@@ -2371,19 +2387,23 @@ int ssl3_get_client_key_exchange(SSL *s)
                 * to remain non-zero (0xff). */
                decrypt_good &= version_good;
 
-               /* Now copy rand_premaster_secret over p using
-                * decrypt_good_mask. */
-               for (i = 0; i < (int) sizeof(rand_premaster_secret); i++)
+               /*
+                * Now copy rand_premaster_secret over from p using
+                * decrypt_good_mask. If decryption failed, then p does not
+                * contain valid plaintext, however, a check above guarantees
+                * it is still sufficiently large to read from.
+                */
+               for (j = 0; j < sizeof(rand_premaster_secret); j++)
                        {
-                       p[i] = constant_time_select_8(decrypt_good, p[i],
-                                                     rand_premaster_secret[i]);
+                       p[j] = constant_time_select_8(decrypt_good, p[j],
+                                                     rand_premaster_secret[j]);
                        }
 
                s->session->master_key_length=
                        s->method->ssl3_enc->generate_master_secret(s,
                                s->session->master_key,
-                               p,i);
-               OPENSSL_cleanse(p,i);
+                               p,sizeof(rand_premaster_secret));
+               OPENSSL_cleanse(p,sizeof(rand_premaster_secret));
                }
        else
 #endif