Disentangle RSA premaster secret parsing

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>

diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index e864ad158076cfaaef10f9d7c57fdad06629748f..18d3be1b2abe6c4f8a889a241812be135bdffb21 100644 (file)
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -2226,9 +2226,8 @@ int ssl3_get_client_key_exchange(SSL *s)
     EC_POINT *clnt_ecpoint = NULL;
     BN_CTX *bn_ctx = NULL;
 #endif
-    PACKET pkt;
-    unsigned char *data;
-    size_t remain;
+    PACKET pkt, enc_premaster;
+    unsigned char *data, *rsa_decrypt = NULL;
 
     n = s->method->ssl_get_message(s,
                                    SSL3_ST_SR_KEY_EXCH_A,
@@ -2353,59 +2352,44 @@ int ssl3_get_client_key_exchange(SSL *s)
             rsa = pkey->pkey.rsa;
         }
 
-        /* TLS and [incidentally] DTLS{0xFEFF} */
-        if (s->version > SSL3_VERSION && s->version != DTLS1_BAD_VER) {
-            if (!PACKET_get_net_2(&pkt, &i)) {
-                al = SSL_AD_DECODE_ERROR;
-                SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH);
-                goto f_err;
-            }
-            remain = PACKET_remaining(&pkt);
-            if (remain != i) {
-                if (!(s->options & SSL_OP_TLS_D5_BUG)) {
+        /* SSLv3 and pre-standard DTLS omit the length bytes. */
+        if (s->version == SSL3_VERSION || s->version == DTLS1_BAD_VER) {
+            enc_premaster = pkt;
+        } else {
+            PACKET orig = pkt;
+            if (!PACKET_get_length_prefixed_2(&pkt, &enc_premaster)
+                || PACKET_remaining(&pkt) != 0) {
+                /* Try SSLv3 behaviour for TLS. */
+                if (s->options & SSL_OP_TLS_D5_BUG) {
+                    enc_premaster = orig;
+                } else {
                     al = SSL_AD_DECODE_ERROR;
-                    SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,
-                           SSL_R_TLS_RSA_ENCRYPTED_VALUE_LENGTH_IS_WRONG);
+                    SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH);
                     goto f_err;
-                } else {
-                    remain += 2;
-                    if (!PACKET_back(&pkt, 2)) {
-                        /*
-                         * 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
-         * 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.
+         * We want to be sure that the plaintext buffer size makes it safe to
+         * iterate over the entire size of a premaster secret
+         * (SSL_MAX_MASTER_KEY_LENGTH). Reject overly short RSA keys because
+         * their ciphertext cannot accommodate a premaster secret anyway.
          */
-
-        if (remain < SSL_MAX_MASTER_KEY_LENGTH) {
-            al = SSL_AD_DECRYPT_ERROR;
+        if (RSA_size(rsa) < SSL_MAX_MASTER_KEY_LENGTH) {
+            al = SSL_AD_INTERNAL_ERROR;
             SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,
-                   SSL_R_TLS_RSA_ENCRYPTED_VALUE_LENGTH_IS_WRONG);
+                   RSA_R_KEY_SIZE_TOO_SMALL);
             goto f_err;
         }
 
-        if (!PACKET_get_bytes(&pkt, &data, remain)) {
-            /* We already checked we had enough data so this shouldn't happen */
+        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_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
          * 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,
-                              sizeof(rand_premaster_secret)) <= 0)
+                       sizeof(rand_premaster_secret)) <= 0) {
             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();
 
         /*
@@ -2437,9 +2424,11 @@ int ssl3_get_client_key_exchange(SSL *s)
          * constant time and are treated like any other decryption error.
          */
         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 &=
-            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
@@ -2453,9 +2442,10 @@ int ssl3_get_client_key_exchange(SSL *s)
         if (s->options & SSL_OP_TLS_ROLLBACK_BUG) {
             unsigned char 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 &=
-                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;
         }
 
@@ -2472,16 +2462,19 @@ int ssl3_get_client_key_exchange(SSL *s)
          * it is still sufficiently large to read from.
          */
         for (j = 0; j < sizeof(rand_premaster_secret); j++) {
-            data[j] = constant_time_select_8(decrypt_good, data[j],
-                                          rand_premaster_secret[j]);
+            rsa_decrypt[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),
-                                        0)) {
+        if (!ssl_generate_master_secret(s, rsa_decrypt,
+                                        sizeof(rand_premaster_secret), 0)) {
             al = SSL_AD_INTERNAL_ERROR;
             SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
             goto f_err;
         }
+        OPENSSL_free(rsa_decrypt);
+        rsa_decrypt = NULL;
     } else
 #endif
 #ifndef OPENSSL_NO_DH
@@ -2852,6 +2845,7 @@ int ssl3_get_client_key_exchange(SSL *s)
     EC_POINT_free(clnt_ecpoint);
     EC_KEY_free(srvr_ecdh);
     BN_CTX_free(bn_ctx);
+    OPENSSL_free(rsa_decrypt);
 #endif
 #ifndef OPENSSL_NO_PSK
     OPENSSL_clear_free(s->s3->tmp.psk, s->s3->tmp.psklen);