From f2fedefdcaf62f10b566f55858c25f35112072ea Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 16 Aug 2014 01:37:34 -0400 Subject: [PATCH] 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 --- include/openssl/dtls1.h | 3 ++ ssl/d1_clnt.c | 64 +++++++++++++++-------------- ssl/s3_clnt.c | 56 +++---------------------- ssl/test/runner/handshake_server.go | 14 +++++-- 4 files changed, 54 insertions(+), 83 deletions(-) diff --git a/include/openssl/dtls1.h b/include/openssl/dtls1.h index 18fd13685..5aef0c45d 100644 --- a/include/openssl/dtls1.h +++ b/include/openssl/dtls1.h @@ -171,7 +171,10 @@ typedef struct hm_fragment_st 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 b34ed426c..282c427a4 100644 --- a/ssl/d1_clnt.c +++ b/ssl/d1_clnt.c @@ -232,8 +232,6 @@ int dtls1_connect(SSL *s) 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 @@ int dtls1_connect(SSL *s) 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 @@ int dtls1_connect(SSL *s) 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 @@ static int dtls1_get_hello_verify(SSL *s) 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 @@ static int dtls1_get_hello_verify(SSL *s) 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 631ed8bdd..45bf9b460 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -683,27 +683,14 @@ int ssl3_client_hello(SSL *s) 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 @@ int ssl3_get_server_hello(SSL *s) 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 3a54eb242..04f7c6ca2 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -142,9 +142,17 @@ func (hs *serverHandshakeState) readClientHello() (isResume bool, err error) { 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