From 1f9f9c4b5127b54b9363cac43a05c8eff27f102e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 28 Aug 2015 16:17:59 -0400 Subject: [PATCH] Tidy up the ssl3_send_server_key_exchange slightly. The handshake state machine is still rather messy (we should switch to CBB, split the key exchanges apart, and also pull reading and writing out), but this version makes it more obvious to the compiler that |p| and |sig_len| are initialized. The old logic created a synchronous-only state which, if enterred directly, resulted in some variables being uninitialized. Change-Id: Ia3ac9397d523fe299c50a95dc82a9b26304cea96 Reviewed-on: https://boringssl-review.googlesource.com/5765 Reviewed-by: Adam Langley --- include/openssl/ssl3.h | 1 - ssl/d1_srvr.c | 1 - ssl/s3_srvr.c | 75 +++++++++++++++++++++--------------------- 3 files changed, 37 insertions(+), 40 deletions(-) diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h index 9c272a67..769fbf7c 100644 --- a/include/openssl/ssl3.h +++ b/include/openssl/ssl3.h @@ -628,7 +628,6 @@ typedef struct ssl3_state_st { #define SSL3_ST_SW_KEY_EXCH_A (0x150 | SSL_ST_ACCEPT) #define SSL3_ST_SW_KEY_EXCH_B (0x151 | SSL_ST_ACCEPT) #define SSL3_ST_SW_KEY_EXCH_C (0x152 | SSL_ST_ACCEPT) -#define SSL3_ST_SW_KEY_EXCH_D (0x153 | SSL_ST_ACCEPT) #define SSL3_ST_SW_CERT_REQ_A (0x160 | SSL_ST_ACCEPT) #define SSL3_ST_SW_CERT_REQ_B (0x161 | SSL_ST_ACCEPT) #define SSL3_ST_SW_SRVR_DONE_A (0x170 | SSL_ST_ACCEPT) diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c index ca086512..e3e3f179 100644 --- a/ssl/d1_srvr.c +++ b/ssl/d1_srvr.c @@ -252,7 +252,6 @@ int dtls1_accept(SSL *s) { case SSL3_ST_SW_KEY_EXCH_A: case SSL3_ST_SW_KEY_EXCH_B: case SSL3_ST_SW_KEY_EXCH_C: - case SSL3_ST_SW_KEY_EXCH_D: alg_a = s->s3->tmp.new_cipher->algorithm_auth; /* Send a ServerKeyExchange message if: diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 037f2320..7c2d3fa9 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -319,7 +319,6 @@ int ssl3_accept(SSL *s) { case SSL3_ST_SW_KEY_EXCH_A: case SSL3_ST_SW_KEY_EXCH_B: case SSL3_ST_SW_KEY_EXCH_C: - case SSL3_ST_SW_KEY_EXCH_D: alg_a = s->s3->tmp.new_cipher->algorithm_auth; /* Send a ServerKeyExchange message if: @@ -1251,6 +1250,10 @@ int ssl3_send_server_key_exchange(SSL *s) { BUF_MEM *buf; EVP_MD_CTX md_ctx; + if (s->state == SSL3_ST_SW_KEY_EXCH_C) { + return ssl_do_write(s); + } + if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher)) { if (!ssl_has_private_key(s)) { al = SSL_AD_INTERNAL_ERROR; @@ -1262,6 +1265,7 @@ int ssl3_send_server_key_exchange(SSL *s) { } EVP_MD_CTX_init(&md_ctx); + enum ssl_private_key_result_t sign_result; if (s->state == SSL3_ST_SW_KEY_EXCH_A) { alg_k = s->s3->tmp.new_cipher->algorithm_mkey; alg_a = s->s3->tmp.new_cipher->algorithm_auth; @@ -1442,7 +1446,6 @@ int ssl3_send_server_key_exchange(SSL *s) { encodedPoint = NULL; } - /* not anonymous */ if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher)) { /* n is the length of the params, they start at d and p points to * the space at the end. */ @@ -1477,51 +1480,47 @@ int ssl3_send_server_key_exchange(SSL *s) { goto err; } - const enum ssl_private_key_result_t sign_result = ssl_private_key_sign( - s, &p[2], &sig_len, max_sig_len, EVP_MD_CTX_md(&md_ctx), - digest, digest_length); - if (sign_result == ssl_private_key_retry) { - s->rwstate = SSL_PRIVATE_KEY_OPERATION; - /* Stash away |p|. */ - s->init_num = p - ssl_handshake_start(s) + SSL_HM_HEADER_LENGTH(s); - s->state = SSL3_ST_SW_KEY_EXCH_B; - goto err; - } else if (sign_result != ssl_private_key_success) { - goto err; - } + sign_result = ssl_private_key_sign(s, &p[2], &sig_len, max_sig_len, + EVP_MD_CTX_md(&md_ctx), digest, + digest_length); + } else { + /* This key exchange doesn't involve a signature. */ + sign_result = ssl_private_key_success; + sig_len = 0; } - - s->state = SSL3_ST_SW_KEY_EXCH_C; - } else if (s->state == SSL3_ST_SW_KEY_EXCH_B) { - /* Complete async sign. */ + } else { + assert(s->state == SSL3_ST_SW_KEY_EXCH_B); /* Restore |p|. */ p = ssl_handshake_start(s) + s->init_num - SSL_HM_HEADER_LENGTH(s); - const enum ssl_private_key_result_t sign_result = - ssl_private_key_sign_complete(s, &p[2], &sig_len, max_sig_len); - if (sign_result == ssl_private_key_retry) { + sign_result = ssl_private_key_sign_complete(s, &p[2], &sig_len, + max_sig_len); + } + + switch (sign_result) { + case ssl_private_key_success: + s->rwstate = SSL_NOTHING; + break; + case ssl_private_key_failure: + s->rwstate = SSL_NOTHING; + goto err; + case ssl_private_key_retry: s->rwstate = SSL_PRIVATE_KEY_OPERATION; + /* Stash away |p|. */ + s->init_num = p - ssl_handshake_start(s) + SSL_HM_HEADER_LENGTH(s); + s->state = SSL3_ST_SW_KEY_EXCH_B; goto err; - } else if (sign_result != ssl_private_key_success) { - goto err; - } - - s->rwstate = SSL_NOTHING; - s->state = SSL3_ST_SW_KEY_EXCH_C; } - if (s->state == SSL3_ST_SW_KEY_EXCH_C) { - if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher)) { - s2n(sig_len, p); - p += sig_len; - } - if (!ssl_set_handshake_header(s, SSL3_MT_SERVER_KEY_EXCHANGE, - p - ssl_handshake_start(s))) { - goto err; - } - s->state = SSL3_ST_SW_KEY_EXCH_D; + if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher)) { + s2n(sig_len, p); + p += sig_len; } + if (!ssl_set_handshake_header(s, SSL3_MT_SERVER_KEY_EXCHANGE, + p - ssl_handshake_start(s))) { + goto err; + } + s->state = SSL3_ST_SW_KEY_EXCH_C; - /* state SSL3_ST_SW_KEY_EXCH_D */ EVP_MD_CTX_cleanup(&md_ctx); return ssl_do_write(s);