From 0907d7105cbf8d72b267f4453f96dd636fa59621 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 6 Jul 2016 09:55:31 +0100 Subject: [PATCH 1/1] 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: Richard Levitte --- ssl/s3_lib.c | 14 +- ssl/statem/statem_srvr.c | 452 ++++++++++++++++++++------------------- 2 files changed, 245 insertions(+), 221 deletions(-) diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index bd831bc48d..8218c2fa74 100644 --- a/ssl/s3_lib.c +++ b/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 + } + + err: if (pms) { if (free_pms) OPENSSL_clear_free(pms, pmslen); diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index ce76deb492..51ec2a7186 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -2015,251 +2015,272 @@ 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; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH); - goto f_err; - } - if (PACKET_remaining(&psk_identity) > PSK_MAX_IDENTITY_LEN) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, - SSL_R_DATA_LENGTH_TOO_LONG); - goto f_err; - } - if (s->psk_server_callback == NULL) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, - SSL_R_PSK_NO_SERVER_CB); - goto f_err; - } + unsigned char psk[PSK_MAX_PSK_LEN]; + size_t psklen; + PACKET psk_identity; - if (!PACKET_strndup(&psk_identity, &s->session->psk_identity)) { - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); - al = SSL_AD_INTERNAL_ERROR; - goto f_err; - } + if (!PACKET_get_length_prefixed_2(pkt, &psk_identity)) { + *al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH); + return 0; + } + if (PACKET_remaining(&psk_identity) > PSK_MAX_IDENTITY_LEN) { + *al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, + SSL_R_DATA_LENGTH_TOO_LONG); + return 0; + } + if (s->psk_server_callback == NULL) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, + SSL_R_PSK_NO_SERVER_CB); + return 0; + } - psklen = s->psk_server_callback(s, s->session->psk_identity, - psk, sizeof(psk)); + 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); + return 0; + } - if (psklen > PSK_MAX_PSK_LEN) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); - goto f_err; - } else if (psklen == 0) { - /* - * PSK related to the given identity not found - */ - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, - SSL_R_PSK_IDENTITY_NOT_FOUND); - al = SSL_AD_UNKNOWN_PSK_IDENTITY; - goto f_err; - } + psklen = s->psk_server_callback(s, s->session->psk_identity, + psk, sizeof(psk)); - OPENSSL_free(s->s3->tmp.psk); - s->s3->tmp.psk = OPENSSL_memdup(psk, psklen); - OPENSSL_cleanse(psk, psklen); + if (psklen > PSK_MAX_PSK_LEN) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); + 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); + return 0; + } - if (s->s3->tmp.psk == NULL) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE); - goto f_err; - } + OPENSSL_free(s->s3->tmp.psk); + s->s3->tmp.psk = OPENSSL_memdup(psk, psklen); + OPENSSL_cleanse(psk, psklen); - s->s3->tmp.psklen = psklen; + if (s->s3->tmp.psk == NULL) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE); + return 0; } - 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; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); - goto f_err; - } - } else + + s->s3->tmp.psklen = psklen; + + return 1; +#else + /* Should never happen */ + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); + 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; - size_t j, padding_len; - PACKET enc_premaster; - RSA *rsa = NULL; - unsigned char *rsa_decrypt = NULL; - - rsa = EVP_PKEY_get0_RSA(s->cert->pkeys[SSL_PKEY_RSA_ENC].privatekey); - if (rsa == NULL) { - al = SSL_AD_HANDSHAKE_FAILURE; + unsigned char rand_premaster_secret[SSL_MAX_MASTER_KEY_LENGTH]; + int decrypt_len; + unsigned char decrypt_good, version_good; + size_t j, padding_len; + 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; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, + SSL_R_MISSING_RSA_CERTIFICATE); + return 0; + } + + /* SSLv3 and pre-standard DTLS omit the length bytes. */ + if (s->version == SSL3_VERSION || s->version == DTLS1_BAD_VER) { + enc_premaster = *pkt; + } else { + if (!PACKET_get_length_prefixed_2(pkt, &enc_premaster) + || PACKET_remaining(pkt) != 0) { + *al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, - SSL_R_MISSING_RSA_CERTIFICATE); - goto f_err; + SSL_R_LENGTH_MISMATCH); + return 0; } + } - /* SSLv3 and pre-standard DTLS omit the length bytes. */ - if (s->version == SSL3_VERSION || s->version == DTLS1_BAD_VER) { - enc_premaster = *pkt; - } else { - if (!PACKET_get_length_prefixed_2(pkt, &enc_premaster) - || PACKET_remaining(pkt) != 0) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, - SSL_R_LENGTH_MISMATCH); - goto f_err; - } - } + /* + * 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 (RSA_size(rsa) < SSL_MAX_MASTER_KEY_LENGTH) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, + RSA_R_KEY_SIZE_TOO_SMALL); + return 0; + } - /* - * 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 (RSA_size(rsa) < SSL_MAX_MASTER_KEY_LENGTH) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, - RSA_R_KEY_SIZE_TOO_SMALL); - goto f_err; - } + rsa_decrypt = OPENSSL_malloc(RSA_size(rsa)); + if (rsa_decrypt == NULL) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE); + return 0; + } - rsa_decrypt = OPENSSL_malloc(RSA_size(rsa)); - if (rsa_decrypt == NULL) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE); - OPENSSL_free(rsa_decrypt); - 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 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 + */ - /* - * 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)) <= 0) + goto err; - if (RAND_bytes(rand_premaster_secret, - sizeof(rand_premaster_secret)) <= 0) { - OPENSSL_free(rsa_decrypt); - 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_NO_PADDING); + if (decrypt_len < 0) + 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_NO_PADDING); - if (decrypt_len < 0) { - OPENSSL_free(rsa_decrypt); - goto err; - } + /* Check the padding. See RFC 3447, section 7.2.2. */ - /* Check the padding. See RFC 3447, section 7.2.2. */ + /* + * 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. + */ + 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 err; + } - /* - * 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. - */ - 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); - OPENSSL_free(rsa_decrypt); - 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]); - 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 + * 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. + */ + version_good = + constant_time_eq_8(rsa_decrypt[padding_len], + (unsigned)(s->client_version >> 8)); + version_good &= + constant_time_eq_8(rsa_decrypt[padding_len + 1], + (unsigned)(s->client_version & 0xff)); - /* - * 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. - */ - version_good = - constant_time_eq_8(rsa_decrypt[padding_len], - (unsigned)(s->client_version >> 8)); - version_good &= + /* + * The premaster secret must contain the same version number as the + * ClientHello to detect version rollback attacks (strangely, the + * protocol does not offer such protection for DH ciphersuites). + * However, buggy clients exist that send the negotiated protocol + * version instead if the server does not support the requested + * protocol version. If SSL_OP_TLS_ROLLBACK_BUG is set, tolerate such + * clients. + */ + if (s->options & SSL_OP_TLS_ROLLBACK_BUG) { + unsigned char workaround_good; + workaround_good = constant_time_eq_8(rsa_decrypt[padding_len], + (unsigned)(s->version >> 8)); + workaround_good &= constant_time_eq_8(rsa_decrypt[padding_len + 1], - (unsigned)(s->client_version & 0xff)); + (unsigned)(s->version & 0xff)); + version_good |= workaround_good; + } - /* - * The premaster secret must contain the same version number as the - * ClientHello to detect version rollback attacks (strangely, the - * protocol does not offer such protection for DH ciphersuites). - * However, buggy clients exist that send the negotiated protocol - * version instead if the server does not support the requested - * protocol version. If SSL_OP_TLS_ROLLBACK_BUG is set, tolerate such - * clients. - */ - if (s->options & SSL_OP_TLS_ROLLBACK_BUG) { - unsigned char workaround_good; - workaround_good = constant_time_eq_8(rsa_decrypt[padding_len], - (unsigned)(s->version >> 8)); - workaround_good &= - constant_time_eq_8(rsa_decrypt[padding_len + 1], - (unsigned)(s->version & 0xff)); - version_good |= workaround_good; - } + /* + * Both decryption and version must be good for decrypt_good to + * remain non-zero (0xff). + */ + decrypt_good &= version_good; - /* - * Both decryption and version must be good for decrypt_good to - * remain non-zero (0xff). - */ - decrypt_good &= version_good; + /* + * 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++) { + rsa_decrypt[padding_len + j] = + constant_time_select_8(decrypt_good, + rsa_decrypt[padding_len + j], + rand_premaster_secret[j]); + } - /* - * 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++) { - 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 + padding_len, + sizeof(rand_premaster_secret), 0)) { + *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 +} - if (!ssl_generate_master_secret(s, rsa_decrypt + padding_len, - sizeof(rand_premaster_secret), 0)) { +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; + } + /* 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); - OPENSSL_free(rsa_decrypt); goto f_err; } - OPENSSL_free(rsa_decrypt); + } 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; - f_err: - 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 + f_err: + if (al != -1) + ssl3_send_alert(s, SSL3_AL_FATAL, al); #ifndef OPENSSL_NO_PSK OPENSSL_clear_free(s->s3->tmp.psk, s->s3->tmp.psklen); s->s3->tmp.psk = NULL; -- 2.34.1