Split out PSK preamble and RSA from process CKE code

The tls_process_client_key_exchange() function is far too long. This
splits out the PSK preamble processing, and the RSA processing into
separate functions.
Reviewed-by: NRichard Levitte <levitte@openssl.org>

ssl/s3_lib.c

@@ -3919,9 +3919,9 @@ int ssl_fill_hello_random(SSL *s, int server, unsigned char *result, int len)
 int ssl_generate_master_secret(SSL *s, unsigned char *pms, size_t pmslen,
                                int free_pms)
 {
-#ifndef OPENSSL_NO_PSK
     unsigned long alg_k = s->s3->tmp.new_cipher->algorithm_mkey;
     if (alg_k & SSL_PSK) {
+#ifndef OPENSSL_NO_PSK
         unsigned char *pskpms, *t;
         size_t psklen = s->s3->tmp.psklen;
         size_t pskpmslen;
@@ -3955,15 +3955,19 @@ int ssl_generate_master_secret(SSL *s, unsigned char *pms, size_t pmslen,
                                                         s->session->master_key,
                                                         pskpms, pskpmslen);
         OPENSSL_clear_free(pskpms, pskpmslen);
-    } else
+#else
+        /* Should never happen */
+        s->session->master_key_length = 0;
+        goto err;
 #endif
+    } else {
         s->session->master_key_length =
             s->method->ssl3_enc->generate_master_secret(s,
                                                         s->session->master_key,
                                                         pms, pmslen);
-#ifndef OPENSSL_NO_PSK
+    }
+
  err:
-#endif
     if (pms) {
         if (free_pms)
             OPENSSL_clear_free(pms, pmslen);

ssl/statem/statem_srvr.c

@@ -2015,59 +2015,52 @@ int tls_construct_certificate_request(SSL *s)
     return 0;
 }
 
-MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
+static int tls_process_cke_psk_preamble(SSL *s, PACKET *pkt, int *al)
 {
-    int al;
-    unsigned long alg_k;
-
-    alg_k = s->s3->tmp.new_cipher->algorithm_mkey;
-
 #ifndef OPENSSL_NO_PSK
-    /* For PSK parse and retrieve identity, obtain PSK key */
-    if (alg_k & SSL_PSK) {
     unsigned char psk[PSK_MAX_PSK_LEN];
     size_t psklen;
     PACKET psk_identity;
 
     if (!PACKET_get_length_prefixed_2(pkt, &psk_identity)) {
-            al = SSL_AD_DECODE_ERROR;
+        *al = SSL_AD_DECODE_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH);
-            goto f_err;
+        return 0;
     }
     if (PACKET_remaining(&psk_identity) > PSK_MAX_IDENTITY_LEN) {
-            al = SSL_AD_DECODE_ERROR;
+        *al = SSL_AD_DECODE_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE,
                SSL_R_DATA_LENGTH_TOO_LONG);
-            goto f_err;
+        return 0;
     }
     if (s->psk_server_callback == NULL) {
-            al = SSL_AD_INTERNAL_ERROR;
+        *al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE,
                SSL_R_PSK_NO_SERVER_CB);
-            goto f_err;
+        return 0;
     }
 
     if (!PACKET_strndup(&psk_identity, &s->session->psk_identity)) {
+        *al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
-            al = SSL_AD_INTERNAL_ERROR;
-            goto f_err;
+        return 0;
     }
 
     psklen = s->psk_server_callback(s, s->session->psk_identity,
                                      psk, sizeof(psk));
 
     if (psklen > PSK_MAX_PSK_LEN) {
-            al = SSL_AD_INTERNAL_ERROR;
+        *al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
-            goto f_err;
+        return 0;
     } else if (psklen == 0) {
         /*
          * PSK related to the given identity not found
          */
+        *al = SSL_AD_UNKNOWN_PSK_IDENTITY;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE,
                SSL_R_PSK_IDENTITY_NOT_FOUND);
-            al = SSL_AD_UNKNOWN_PSK_IDENTITY;
-            goto f_err;
+        return 0;
     }
 
     OPENSSL_free(s->s3->tmp.psk);
@@ -2075,30 +2068,26 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
     OPENSSL_cleanse(psk, psklen);
 
     if (s->s3->tmp.psk == NULL) {
-            al = SSL_AD_INTERNAL_ERROR;
+        *al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE);
-            goto f_err;
+        return 0;
     }
 
     s->s3->tmp.psklen = psklen;
-    }
-    if (alg_k & SSL_kPSK) {
-        /* Identity extracted earlier: should be nothing left */
-        if (PACKET_remaining(pkt) != 0) {
-            al = SSL_AD_HANDSHAKE_FAILURE;
-            SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH);
-            goto f_err;
-        }
-        /* PSK handled by ssl_generate_master_secret */
-        if (!ssl_generate_master_secret(s, NULL, 0, 0)) {
-            al = SSL_AD_INTERNAL_ERROR;
+
+    return 1;
+#else
+    /* Should never happen */
+    *al = SSL_AD_INTERNAL_ERROR;
     SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
-            goto f_err;
-        }
-    } else
+    return 0;
 #endif
+}
+
+
+static int tls_process_cke_rsa(SSL *s, PACKET *pkt, int *al)
+{
 #ifndef OPENSSL_NO_RSA
-    if (alg_k & (SSL_kRSA | SSL_kRSAPSK)) {
     unsigned char rand_premaster_secret[SSL_MAX_MASTER_KEY_LENGTH];
     int decrypt_len;
     unsigned char decrypt_good, version_good;
@@ -2106,13 +2095,14 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
     PACKET enc_premaster;
     RSA *rsa = NULL;
     unsigned char *rsa_decrypt = NULL;
+    int ret = 0;
 
     rsa = EVP_PKEY_get0_RSA(s->cert->pkeys[SSL_PKEY_RSA_ENC].privatekey);
     if (rsa == NULL) {
-            al = SSL_AD_HANDSHAKE_FAILURE;
+        *al = SSL_AD_HANDSHAKE_FAILURE;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE,
                SSL_R_MISSING_RSA_CERTIFICATE);
-            goto f_err;
+        return 0;
     }
 
     /* SSLv3 and pre-standard DTLS omit the length bytes. */
@@ -2121,10 +2111,10 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
     } else {
         if (!PACKET_get_length_prefixed_2(pkt, &enc_premaster)
             || PACKET_remaining(pkt) != 0) {
-                al = SSL_AD_DECODE_ERROR;
+            *al = SSL_AD_DECODE_ERROR;
             SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE,
                    SSL_R_LENGTH_MISMATCH);
-                goto f_err;
+            return 0;
         }
     }
 
@@ -2135,18 +2125,17 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
      * their ciphertext cannot accommodate a premaster secret anyway.
      */
     if (RSA_size(rsa) < SSL_MAX_MASTER_KEY_LENGTH) {
-            al = SSL_AD_INTERNAL_ERROR;
+        *al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE,
                RSA_R_KEY_SIZE_TOO_SMALL);
-            goto f_err;
+        return 0;
     }
 
     rsa_decrypt = OPENSSL_malloc(RSA_size(rsa));
     if (rsa_decrypt == NULL) {
-            al = SSL_AD_INTERNAL_ERROR;
+        *al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE);
-            OPENSSL_free(rsa_decrypt);
-            goto f_err;
+        return 0;
     }
 
     /*
@@ -2158,10 +2147,8 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
      */
 
     if (RAND_bytes(rand_premaster_secret,
-                       sizeof(rand_premaster_secret)) <= 0) {
-            OPENSSL_free(rsa_decrypt);
+                   sizeof(rand_premaster_secret)) <= 0)
         goto err;
-        }
 
     /*
      * Decrypt with no padding. PKCS#1 padding will be removed as part of
@@ -2170,10 +2157,8 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
     decrypt_len = RSA_private_decrypt(PACKET_remaining(&enc_premaster),
                                       PACKET_data(&enc_premaster),
                                       rsa_decrypt, rsa, RSA_NO_PADDING);
-        if (decrypt_len < 0) {
-            OPENSSL_free(rsa_decrypt);
+    if (decrypt_len < 0)
         goto err;
-        }
 
     /* Check the padding. See RFC 3447, section 7.2.2. */
 
@@ -2183,10 +2168,9 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
      * PS is at least 8 bytes.
      */
     if (decrypt_len < 11 + SSL_MAX_MASTER_KEY_LENGTH) {
-            al = SSL_AD_DECRYPT_ERROR;
+        *al = SSL_AD_DECRYPT_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_DECRYPTION_FAILED);
-            OPENSSL_free(rsa_decrypt);
-            goto f_err;
+        goto err;
     }
 
     padding_len = decrypt_len - SSL_MAX_MASTER_KEY_LENGTH;
@@ -2252,14 +2236,51 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
 
     if (!ssl_generate_master_secret(s, rsa_decrypt + padding_len,
                                     sizeof(rand_premaster_secret), 0)) {
-            al = SSL_AD_INTERNAL_ERROR;
+        *al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+        goto err;
+    }
+
+    ret = 1;
+ err:
     OPENSSL_free(rsa_decrypt);
+    return ret;
+#else
+    /* Should never happen */
+    *al = SSL_AD_INTERNAL_ERROR;
+    SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+    return 0;
+#endif
+}
+
+MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
+{
+    int al = -1;
+    unsigned long alg_k;
+
+    alg_k = s->s3->tmp.new_cipher->algorithm_mkey;
+
+    /* For PSK parse and retrieve identity, obtain PSK key */
+    if ((alg_k & SSL_PSK) && !tls_process_cke_psk_preamble(s, pkt, &al))
+        goto err;
+
+    if (alg_k & SSL_kPSK) {
+        /* Identity extracted earlier: should be nothing left */
+        if (PACKET_remaining(pkt) != 0) {
+            al = SSL_AD_HANDSHAKE_FAILURE;
+            SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH);
             goto f_err;
         }
-        OPENSSL_free(rsa_decrypt);
+        /* PSK handled by ssl_generate_master_secret */
+        if (!ssl_generate_master_secret(s, NULL, 0, 0)) {
+            al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+            goto f_err;
+        }
+    } else if (alg_k & (SSL_kRSA | SSL_kRSAPSK)) {
+        if (!tls_process_cke_rsa(s, pkt, &al))
+            goto err;
     } else
-#endif
 #ifndef OPENSSL_NO_DH
     if (alg_k & (SSL_kDHE | SSL_kDHEPSK)) {
         EVP_PKEY *skey = NULL;
@@ -2533,11 +2554,10 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
     }
 
     return MSG_PROCESS_CONTINUE_PROCESSING;
+ err:
  f_err:
+    if (al != -1)
         ssl3_send_alert(s, SSL3_AL_FATAL, al);
-#if !defined(OPENSSL_NO_DH) || !defined(OPENSSL_NO_RSA) || !defined(OPENSSL_NO_EC) || defined(OPENSSL_NO_SRP)
- err:
-#endif
 #ifndef OPENSSL_NO_PSK
     OPENSSL_clear_free(s->s3->tmp.psk, s->s3->tmp.psklen);
     s->s3->tmp.psk = NULL;