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 <agl@google.com>
diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index 9c272a6..769fbf7 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -628,7 +628,6 @@
#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 ca08651..e3e3f17 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -252,7 +252,6 @@
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 037f232..7c2d3fa 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -319,7 +319,6 @@
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 @@
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 @@
}
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 @@
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 @@
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);
