Simplify HelloVerifyRequest processing.
Rather than switching the order of the ServerHello and HelloVerifyRequest
states and processing each twice, have the states follow the protocol order.
HelloVerifyRequest reading is optional and ServerHello is strict. Use the
send_cookie bit to determine whether we're expecting a cookie or not.
Fix the dtls1_stop_timer call in these states to consistently hit the end of a
server flight; the previous flight should not be cleared from the retransmit
buffer until the entire next flight is received. That said, OpenSSL doesn't
appear to implement the part where, on receipt of the previous peer flight, the
buffered flight is retransmitted. (With the exception of a SSL3_MT_FINISHED
special-case in dtls1_read_bytes.) So if the peer is also OpenSSL, this doesn't
do anything.
Also fix the DTLS test which wasn't actually asserting that the ClientHello
matched.
Change-Id: Ia542190972dbffabb837d32c9d453a243caa90b2
Reviewed-on: https://boringssl-review.googlesource.com/1551
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/dtls1.h b/include/openssl/dtls1.h
index 18fd136..5aef0c4 100644
--- a/include/openssl/dtls1.h
+++ b/include/openssl/dtls1.h
@@ -171,7 +171,10 @@
typedef struct dtls1_state_st
{
+ /* send_cookie is true if we are resending the ClientHello
+ * with a cookie from a HelloVerifyRequest. */
unsigned int send_cookie;
+
uint8_t cookie[DTLS1_COOKIE_LENGTH];
size_t cookie_len;
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index b34ed42..282c427 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -232,8 +232,6 @@
s->state=SSL3_ST_CW_CLNT_HELLO_A;
s->ctx->stats.sess_connect++;
s->init_num=0;
- /* mark client_random uninitialized */
- memset(s->s3->client_random,0,sizeof(s->s3->client_random));
s->d1->send_cookie = 0;
s->hit = 0;
break;
@@ -256,7 +254,7 @@
s->s3->tmp.next_state=SSL3_ST_CR_SRVR_HELLO_A;
}
else
- s->state=SSL3_ST_CR_SRVR_HELLO_A;
+ s->state=DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A;
s->init_num=0;
/* turn on buffering for the next lot of output */
@@ -265,41 +263,46 @@
break;
- case SSL3_ST_CR_SRVR_HELLO_A:
- case SSL3_ST_CR_SRVR_HELLO_B:
- ret=ssl3_get_server_hello(s);
- if (ret <= 0) goto end;
- else
- {
- if (s->hit)
- {
- s->state=SSL3_ST_CR_FINISHED_A;
- if (s->tlsext_ticket_expected)
- {
- /* receive renewed session ticket */
- s->state=SSL3_ST_CR_SESSION_TICKET_A;
- }
- }
- else
- s->state=DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A;
- }
- s->init_num=0;
- break;
-
case DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A:
case DTLS1_ST_CR_HELLO_VERIFY_REQUEST_B:
ret = dtls1_get_hello_verify(s);
if ( ret <= 0)
goto end;
- dtls1_stop_timer(s);
- if ( s->d1->send_cookie) /* start again, with a cookie */
- s->state=SSL3_ST_CW_CLNT_HELLO_A;
+ if ( s->d1->send_cookie)
+ {
+ /* start again, with a cookie */
+ dtls1_stop_timer(s);
+ s->state = SSL3_ST_CW_CLNT_HELLO_A;
+ }
else
- s->state = SSL3_ST_CR_CERT_A;
+ {
+ s->state = SSL3_ST_CR_SRVR_HELLO_A;
+ }
s->init_num = 0;
break;
+ case SSL3_ST_CR_SRVR_HELLO_A:
+ case SSL3_ST_CR_SRVR_HELLO_B:
+ ret=ssl3_get_server_hello(s);
+ if (ret <= 0) goto end;
+
+ if (s->hit)
+ {
+ s->state=SSL3_ST_CR_FINISHED_A;
+ if (s->tlsext_ticket_expected)
+ {
+ /* receive renewed session ticket */
+ s->state=SSL3_ST_CR_SESSION_TICKET_A;
+ }
+ }
+ else
+ {
+ s->state=SSL3_ST_CR_CERT_A;
+ }
+ s->init_num=0;
+ break;
+
case SSL3_ST_CR_CERT_A:
case SSL3_ST_CR_CERT_B:
if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher))
@@ -594,7 +597,8 @@
DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A,
DTLS1_ST_CR_HELLO_VERIFY_REQUEST_B,
-1,
- s->max_cert_list,
+ /* Use the same maximum size as ssl3_get_server_hello. */
+ 20000,
&ok);
s->first_packet = 0;
@@ -603,7 +607,7 @@
if (s->s3->tmp.message_type != DTLS1_MT_HELLO_VERIFY_REQUEST)
{
s->d1->send_cookie = 0;
- s->s3->tmp.reuse_message=1;
+ s->s3->tmp.reuse_message = 1;
return(1);
}
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 631ed8b..45bf9b4 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -683,27 +683,14 @@
p=s->s3->client_random;
- /* for DTLS if client_random is initialized, reuse it, we are
- * required to use same upon reply to HelloVerify */
- if (SSL_IS_DTLS(s))
+ /* If resending the ClientHello in DTLS after a
+ * HelloVerifyRequest, don't renegerate the client_random. The
+ * random must be reused. */
+ if (!SSL_IS_DTLS(s) || !s->d1->send_cookie)
{
- size_t idx;
- i = 1;
- for (idx=0; idx < sizeof(s->s3->client_random); idx++)
- {
- if (p[idx])
- {
- i = 0;
- break;
- }
- }
- }
- else
- i = 1;
-
- if (i)
ssl_fill_hello_random(s, 0, p,
sizeof(s->s3->client_random));
+ }
/* Do the message type and length last.
* Note: the final argument to ssl_add_clienthello_tlsext below
@@ -829,47 +816,16 @@
CBS server_hello, server_random, session_id;
uint16_t server_version, cipher_suite;
uint8_t compression_method;
- /* Hello verify request and/or server hello version may not
- * match so set first packet if we're negotiating version.
- */
- if (SSL_IS_DTLS(s))
- s->first_packet = 1;
n=s->method->ssl_get_message(s,
SSL3_ST_CR_SRVR_HELLO_A,
SSL3_ST_CR_SRVR_HELLO_B,
- -1,
+ SSL3_MT_SERVER_HELLO,
20000, /* ?? */
&ok);
if (!ok) return((int)n);
- if (SSL_IS_DTLS(s))
- {
- s->first_packet = 0;
- if ( s->s3->tmp.message_type == DTLS1_MT_HELLO_VERIFY_REQUEST)
- {
- if ( s->d1->send_cookie == 0)
- {
- s->s3->tmp.reuse_message = 1;
- return 1;
- }
- else /* already sent a cookie */
- {
- al=SSL_AD_UNEXPECTED_MESSAGE;
- OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, SSL_R_BAD_MESSAGE_TYPE);
- goto f_err;
- }
- }
- }
-
- if ( s->s3->tmp.message_type != SSL3_MT_SERVER_HELLO)
- {
- al=SSL_AD_UNEXPECTED_MESSAGE;
- OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, SSL_R_BAD_MESSAGE_TYPE);
- goto f_err;
- }
-
CBS_init(&server_hello, s->init_msg, n);
if (!CBS_get_u16(&server_hello, &server_version) ||
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 3a54eb2..04f7c6c 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -142,9 +142,17 @@
if !bytes.Equal(newClientHello.cookie, helloVerifyRequest.cookie) {
return false, errors.New("dtls: invalid cookie")
}
- // Apart from the cookie, client hello must match.
- hs.clientHello.cookie = newClientHello.cookie
- if hs.clientHello.equal(newClientHello) {
+
+ // Apart from the cookie, the two ClientHellos must
+ // match. Note that clientHello.equal compares the
+ // serialization, so we make a copy.
+ oldClientHelloCopy := *hs.clientHello
+ oldClientHelloCopy.raw = nil
+ oldClientHelloCopy.cookie = nil
+ newClientHelloCopy := *newClientHello
+ newClientHelloCopy.raw = nil
+ newClientHelloCopy.cookie = nil
+ if !oldClientHelloCopy.equal(&newClientHelloCopy) {
return false, errors.New("dtls: retransmitted ClientHello does not match")
}
hs.clientHello = newClientHello
