From: Matt Caswell Date: Fri, 10 Jan 2020 14:16:30 +0000 (+0000) Subject: libssl: Eliminate as much use of EVP_PKEY_size() as possible X-Git-Tag: openssl-3.0.0-alpha1~638 X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=bddbfae1cd667f0c1458deff98cbc03171e90d5a;p=oweals%2Fopenssl.git libssl: Eliminate as much use of EVP_PKEY_size() as possible Some uses were going against documented recommendations. Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/10798) --- diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 13610ba1b7..a13d2708b1 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -2301,7 +2301,6 @@ MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt) /* if it was signed, check the signature */ if (pkey != NULL) { PACKET params; - int maxsig; const EVP_MD *md = NULL; unsigned char *tbs; size_t tbslen; @@ -2352,22 +2351,6 @@ MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt) SSL_R_LENGTH_MISMATCH); goto err; } - maxsig = EVP_PKEY_size(pkey); - if (maxsig < 0) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_KEY_EXCHANGE, - ERR_R_INTERNAL_ERROR); - goto err; - } - - /* - * Check signature length - */ - if (PACKET_remaining(&signature) > (size_t)maxsig) { - /* wrong packet length */ - SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PROCESS_KEY_EXCHANGE, - SSL_R_WRONG_SIGNATURE_LENGTH); - goto err; - } md_ctx = EVP_MD_CTX_new(); if (md_ctx == NULL) { diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 960b345268..c478bb47aa 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -271,13 +271,6 @@ int tls_construct_cert_verify(SSL *s, WPACKET *pkt) ERR_R_INTERNAL_ERROR); goto err; } - siglen = EVP_PKEY_size(pkey); - sig = OPENSSL_malloc(siglen); - if (sig == NULL) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CERT_VERIFY, - ERR_R_MALLOC_FAILURE); - goto err; - } if (EVP_DigestSignInit(mctx, &pctx, md, NULL, pkey) <= 0) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CERT_VERIFY, @@ -295,6 +288,10 @@ int tls_construct_cert_verify(SSL *s, WPACKET *pkt) } } if (s->version == SSL3_VERSION) { + /* + * Here we use EVP_DigestSignUpdate followed by EVP_DigestSignFinal + * in order to add the EVP_CTRL_SSL3_MASTER_SECRET call between them. + */ if (EVP_DigestSignUpdate(mctx, hdata, hdatalen) <= 0 /* * TODO(3.0) Replace this when EVP_MD_CTX_ctrl() is deprecated @@ -303,16 +300,36 @@ int tls_construct_cert_verify(SSL *s, WPACKET *pkt) || EVP_MD_CTX_ctrl(mctx, EVP_CTRL_SSL3_MASTER_SECRET, (int)s->session->master_key_length, s->session->master_key) <= 0 - || EVP_DigestSignFinal(mctx, sig, &siglen) <= 0) { + || EVP_DigestSignFinal(mctx, NULL, &siglen) <= 0) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CERT_VERIFY, ERR_R_EVP_LIB); goto err; } - } else if (EVP_DigestSign(mctx, sig, &siglen, hdata, hdatalen) <= 0) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CERT_VERIFY, - ERR_R_EVP_LIB); - goto err; + sig = OPENSSL_malloc(siglen); + if (sig == NULL + || EVP_DigestSignFinal(mctx, sig, &siglen) <= 0) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CERT_VERIFY, + ERR_R_EVP_LIB); + goto err; + } + } else { + /* + * Here we *must* use EVP_DigestSign() because Ed25519/Ed448 does not + * support streaming via EVP_DigestSignUpdate/EVP_DigestSignFinal + */ + if (EVP_DigestSign(mctx, NULL, &siglen, hdata, hdatalen) <= 0) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CERT_VERIFY, + ERR_R_EVP_LIB); + goto err; + } + sig = OPENSSL_malloc(siglen); + if (sig == NULL + || EVP_DigestSign(mctx, sig, &siglen, hdata, hdatalen) <= 0) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CERT_VERIFY, + ERR_R_EVP_LIB); + goto err; + } } #ifndef OPENSSL_NO_GOST @@ -434,13 +451,6 @@ MSG_PROCESS_RETURN tls_process_cert_verify(SSL *s, PACKET *pkt) goto err; } - j = EVP_PKEY_size(pkey); - if (((int)len > j) || ((int)PACKET_remaining(pkt) > j) - || (PACKET_remaining(pkt) == 0)) { - SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PROCESS_CERT_VERIFY, - SSL_R_WRONG_SIGNATURE_SIZE); - goto err; - } if (!PACKET_get_bytes(pkt, &data, len)) { SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PROCESS_CERT_VERIFY, SSL_R_LENGTH_MISMATCH); diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 4e4005f61d..c744bf64eb 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -2762,8 +2762,7 @@ int tls_construct_server_key_exchange(SSL *s, WPACKET *pkt) EVP_PKEY *pkey = s->s3.tmp.cert->privatekey; const EVP_MD *md; unsigned char *sigbytes1, *sigbytes2, *tbs; - size_t siglen, tbslen; - int rv; + size_t siglen = 0, tbslen; if (pkey == NULL || !tls1_lookup_md(lu, &md)) { /* Should never happen */ @@ -2786,15 +2785,8 @@ int tls_construct_server_key_exchange(SSL *s, WPACKET *pkt) ERR_R_INTERNAL_ERROR); goto err; } - /* - * Create the signature. We don't know the actual length of the sig - * until after we've created it, so we reserve enough bytes for it - * up front, and then properly allocate them in the WPACKET - * afterwards. - */ - siglen = EVP_PKEY_size(pkey); - if (!WPACKET_sub_reserve_bytes_u16(pkt, siglen, &sigbytes1) - || EVP_DigestSignInit(md_ctx, &pctx, md, NULL, pkey) <= 0) { + + if (EVP_DigestSignInit(md_ctx, &pctx, md, NULL, pkey) <= 0) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); @@ -2816,15 +2808,19 @@ int tls_construct_server_key_exchange(SSL *s, WPACKET *pkt) /* SSLfatal() already called */ goto err; } - rv = EVP_DigestSign(md_ctx, sigbytes1, &siglen, tbs, tbslen); - OPENSSL_free(tbs); - if (rv <= 0 || !WPACKET_sub_allocate_bytes_u16(pkt, siglen, &sigbytes2) - || sigbytes1 != sigbytes2) { + + if (EVP_DigestSign(md_ctx, NULL, &siglen, tbs, tbslen) <=0 + || !WPACKET_sub_reserve_bytes_u16(pkt, siglen, &sigbytes1) + || EVP_DigestSign(md_ctx, sigbytes1, &siglen, tbs, tbslen) <= 0 + || !WPACKET_sub_allocate_bytes_u16(pkt, siglen, &sigbytes2) + || sigbytes1 != sigbytes2) { + OPENSSL_free(tbs); SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); goto err; } + OPENSSL_free(tbs); } EVP_MD_CTX_free(md_ctx); @@ -3009,19 +3005,6 @@ static int tls_process_cke_rsa(SSL *s, PACKET *pkt) } } - /* - * 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 (EVP_PKEY_size(rsa) < RSA_PKCS1_PADDING_SIZE - + SSL_MAX_MASTER_KEY_LENGTH) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_CKE_RSA, - RSA_R_KEY_SIZE_TOO_SMALL); - return 0; - } - outlen = SSL_MAX_MASTER_KEY_LENGTH; rsa_decrypt = OPENSSL_malloc(outlen); if (rsa_decrypt == NULL) {