diff options
author | Matt Caswell <matt@openssl.org> | 2016-04-19 23:33:35 +0100 |
---|---|---|
committer | Matt Caswell <matt@openssl.org> | 2016-04-22 15:37:17 +0100 |
commit | ee85fc1dd67faebdeecb8fe8834facaee0566324 (patch) | |
tree | 08073541b84babf37402a56844132bb40d37132d | |
parent | 48c1e15ceb2252e65ba63f93a7bf39c1d368f38f (diff) |
Don't set peer_tmp until we have finished constructing it
If we fail halfway through constructing the peer_tmp EVP_PKEY but we have
already stored it in s->s3->peer_tmp then if anything tries to use it then
it will likely fail. This was causing s_client to core dump in the
sslskewith0p test. s_client was trying to print out the connection
parameters that it had negotiated so far. Arguably s_client should not do
that if the connection has failed...but given it is existing functionality
it's easier to fix libssl.
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
-rw-r--r-- | ssl/statem/statem_clnt.c | 63 |
1 files changed, 36 insertions, 27 deletions
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 5b53b8605d..768cf8378e 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1525,9 +1525,10 @@ MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt) #ifndef OPENSSL_NO_DH else if (alg_k & (SSL_kDHE | SSL_kDHEPSK)) { PACKET prime, generator, pub_key; + EVP_PKEY *peer_tmp = NULL; - DH *dh; - BIGNUM *p, *g, *bnpub_key; + DH *dh = NULL; + BIGNUM *p = NULL, *g = NULL, *bnpub_key = NULL; if (!PACKET_get_length_prefixed_2(pkt, &prime) || !PACKET_get_length_prefixed_2(pkt, &generator) @@ -1536,19 +1537,13 @@ MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt) goto f_err; } - s->s3->peer_tmp = EVP_PKEY_new(); + peer_tmp = EVP_PKEY_new(); dh = DH_new(); - if (s->s3->peer_tmp == NULL || dh == NULL) { + if (peer_tmp == NULL || dh == NULL) { + al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE); - DH_free(dh); - goto err; - } - - if (EVP_PKEY_assign_DH(s->s3->peer_tmp, dh) == 0) { - SSLerr(SSL_F_TLS_PROCESS_KEY_EXCHANGE, ERR_R_EVP_LIB); - DH_free(dh); - goto err; + goto dherr; } p = BN_bin2bn(PACKET_data(&prime), PACKET_remaining(&prime), NULL); @@ -1558,39 +1553,53 @@ MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt) NULL); if (p == NULL || g == NULL || bnpub_key == NULL) { SSLerr(SSL_F_TLS_PROCESS_KEY_EXCHANGE, ERR_R_BN_LIB); - BN_free(p); - BN_free(g); - BN_free(bnpub_key); - goto err; + goto dherr; } if (BN_is_zero(p) || BN_is_zero(g) || BN_is_zero(bnpub_key)) { SSLerr(SSL_F_TLS_PROCESS_KEY_EXCHANGE, SSL_R_BAD_DH_VALUE); - BN_free(p); - BN_free(g); - BN_free(bnpub_key); - goto f_err; + goto dherr; } if (!DH_set0_pqg(dh, p, NULL, g)) { + al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_KEY_EXCHANGE, ERR_R_BN_LIB); - BN_free(p); - BN_free(g); - BN_free(bnpub_key); - goto err; + goto dherr; } if (!DH_set0_key(dh, bnpub_key, NULL)) { + al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_KEY_EXCHANGE, ERR_R_BN_LIB); - BN_free(bnpub_key); - goto err; + goto dherr; } if (!ssl_security(s, SSL_SECOP_TMP_DH, DH_security_bits(dh), 0, dh)) { al = SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_TLS_PROCESS_KEY_EXCHANGE, SSL_R_DH_KEY_TOO_SMALL); - goto f_err; + goto dherr; + } + + if (EVP_PKEY_assign_DH(peer_tmp, dh) == 0) { + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_KEY_EXCHANGE, ERR_R_EVP_LIB); + goto dherr; } + + s->s3->peer_tmp = peer_tmp; + + goto dhend; + dherr: + BN_free(p); + BN_free(g); + BN_free(bnpub_key); + DH_free(dh); + EVP_PKEY_free(peer_tmp); + goto f_err; + dhend: + /* + * FIXME: This makes assumptions about which ciphersuites come with + * public keys. We should have a less ad-hoc way of doing this + */ if (alg_a & (SSL_aRSA|SSL_aDSS)) pkey = X509_get0_pubkey(s->session->peer); /* else anonymous DH, so no certificate or pkey. */ |