From c536b6be1a72aefd632d5530106a67c516cb9f4b Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 21 Sep 2016 11:26:47 +0100 Subject: [PATCH] Convert HelloVerifyRequest construction to WPACKET We actually construct a HelloVerifyRequest in two places with common code pulled into a single function. This one commit handles both places. Reviewed-by: Rich Salz --- ssl/d1_lib.c | 116 +++++++++++++++++++++++---------------- ssl/ssl_locl.h | 5 +- ssl/statem/statem_srvr.c | 51 ++++++++--------- 3 files changed, 96 insertions(+), 76 deletions(-) diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index 043057f421..f34818bbcd 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c @@ -437,8 +437,8 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) unsigned char cookie[DTLS1_COOKIE_LENGTH]; unsigned char seq[SEQ_NUM_SIZE]; const unsigned char *data; - unsigned char *p, *buf; - unsigned long reclen, fragoff, fraglen, msglen; + unsigned char *buf; + unsigned long fragoff, fraglen, msglen; unsigned int rectype, versmajor, msgseq, msgtype, clientvers, cookielen; BIO *rbio, *wbio; BUF_MEM *bufm; @@ -680,6 +680,10 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) } if (next == LISTEN_SEND_VERIFY_REQUEST) { + WPACKET wpkt; + unsigned int version; + size_t wreclen; + /* * There was no cookie in the ClientHello so we need to send a * HelloVerifyRequest. If this fails we do not worry about trying @@ -703,60 +707,76 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) return -1; } - p = &buf[DTLS1_RT_HEADER_LENGTH]; - msglen = dtls_raw_hello_verify_request(p + DTLS1_HM_HEADER_LENGTH, - cookie, cookielen); - - *p++ = DTLS1_MT_HELLO_VERIFY_REQUEST; - - /* Message length */ - l2n3(msglen, p); - - /* Message sequence number is always 0 for a HelloVerifyRequest */ - s2n(0, p); - - /* - * We never fragment a HelloVerifyRequest, so fragment offset is 0 - * and fragment length is message length - */ - l2n3(0, p); - l2n3(msglen, p); - - /* Set reclen equal to length of whole handshake message */ - reclen = msglen + DTLS1_HM_HEADER_LENGTH; - - /* Add the record header */ - p = buf; - - *(p++) = SSL3_RT_HANDSHAKE; /* * Special case: for hello verify request, client version 1.0 and we * haven't decided which version to use yet send back using version * 1.0 header: otherwise some clients will ignore it. */ - if (s->method->version == DTLS_ANY_VERSION) { - *(p++) = DTLS1_VERSION >> 8; - *(p++) = DTLS1_VERSION & 0xff; - } else { - *(p++) = s->version >> 8; - *(p++) = s->version & 0xff; + version = (s->method->version == DTLS_ANY_VERSION) ? DTLS1_VERSION + : s->version; + + /* Construct the record and message headers */ + if (!WPACKET_init(&wpkt, s->init_buf) + || !WPACKET_put_bytes_u8(&wpkt, SSL3_RT_HANDSHAKE) + || !WPACKET_put_bytes_u16(&wpkt, version) + /* + * Record sequence number is always the same as in the + * received ClientHello + */ + || !WPACKET_memcpy(&wpkt, seq, SEQ_NUM_SIZE) + /* End of record, start sub packet for message */ + || !WPACKET_start_sub_packet_u16(&wpkt) + /* Message type */ + || !WPACKET_put_bytes_u8(&wpkt, + DTLS1_MT_HELLO_VERIFY_REQUEST) + /* + * Message length - doesn't follow normal TLS convention: + * the length isn't the last thing in the message header. + * We'll need to fill this in later when we know the + * length. Set it to zero for now + */ + || !WPACKET_put_bytes_u24(&wpkt, 0) + /* + * Message sequence number is always 0 for a + * HelloVerifyRequest + */ + || !WPACKET_put_bytes_u16(&wpkt, 0) + /* + * We never fragment a HelloVerifyRequest, so fragment + * offset is 0 + */ + || !WPACKET_put_bytes_u24(&wpkt, 0) + /* + * Fragment length is the same as message length, but + * this *is* the last thing in the message header so we + * can just start a sub-packet. No need to come back + * later for this one. + */ + || !WPACKET_start_sub_packet_u24(&wpkt) + /* Create the actual HelloVerifyRequest body */ + || !dtls_raw_hello_verify_request(&wpkt, cookie, cookielen) + /* Close message body */ + || !WPACKET_close(&wpkt) + /* Close record body */ + || !WPACKET_close(&wpkt) + || !WPACKET_get_total_written(&wpkt, &wreclen) + || !WPACKET_finish(&wpkt)) { + SSLerr(SSL_F_DTLSV1_LISTEN, ERR_R_INTERNAL_ERROR); + WPACKET_cleanup(&wpkt); + /* This is fatal */ + return -1; } /* - * Record sequence number is always the same as in the received - * ClientHello - */ - memcpy(p, seq, SEQ_NUM_SIZE); - p += SEQ_NUM_SIZE; - - /* Length */ - s2n(reclen, p); - - /* - * Set reclen equal to length of whole record including record - * header + * Fix up the message len in the message header. Its the same as the + * fragment len which has been filled in by WPACKET, so just copy + * that. Destination for the message len is after the record header + * plus one byte for the message content type. The source is the + * last 3 bytes of the message header */ - reclen += DTLS1_RT_HEADER_LENGTH; + memcpy(&buf[DTLS1_RT_HEADER_LENGTH + 1], + &buf[DTLS1_RT_HEADER_LENGTH + DTLS1_HM_HEADER_LENGTH - 3], + 3); if (s->msg_callback) s->msg_callback(1, 0, SSL3_RT_HEADER, buf, @@ -778,7 +798,7 @@ int DTLSv1_listen(SSL *s, BIO_ADDR *client) BIO_ADDR_free(tmpclient); tmpclient = NULL; - if (BIO_write(wbio, buf, reclen) < (int)reclen) { + if (BIO_write(wbio, buf, wreclen) < (int)wreclen) { if (BIO_should_retry(wbio)) { /* * Non-blocking IO...but we're stateless, so we're just diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 6ad27354d1..630fea892e 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -1950,9 +1950,8 @@ void dtls1_start_timer(SSL *s); void dtls1_stop_timer(SSL *s); __owur int dtls1_is_timer_expired(SSL *s); void dtls1_double_timeout(SSL *s); -__owur unsigned int dtls_raw_hello_verify_request(unsigned char *buf, - unsigned char *cookie, - unsigned char cookie_len); +__owur int dtls_raw_hello_verify_request(WPACKET *pkt, unsigned char *cookie, + unsigned char cookie_len); __owur int dtls1_send_newsession_ticket(SSL *s); __owur unsigned int dtls1_min_mtu(SSL *s); void dtls1_hm_fragment_free(hm_fragment *frag); diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 587beada88..03d75d0c88 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -840,32 +840,21 @@ int tls_construct_hello_request(SSL *s) return 1; } -unsigned int dtls_raw_hello_verify_request(unsigned char *buf, - unsigned char *cookie, - unsigned char cookie_len) +int dtls_raw_hello_verify_request(WPACKET *pkt, unsigned char *cookie, + unsigned char cookie_len) { - unsigned int msg_len; - unsigned char *p; - - p = buf; /* Always use DTLS 1.0 version: see RFC 6347 */ - *(p++) = DTLS1_VERSION >> 8; - *(p++) = DTLS1_VERSION & 0xFF; - - *(p++) = (unsigned char)cookie_len; - memcpy(p, cookie, cookie_len); - p += cookie_len; - msg_len = p - buf; + if (!WPACKET_put_bytes_u16(pkt, DTLS1_VERSION) + || !WPACKET_sub_memcpy_u8(pkt, cookie, cookie_len)) + return 0; - return msg_len; + return 1; } int dtls_construct_hello_verify_request(SSL *s) { - unsigned int len; - unsigned char *buf; - - buf = (unsigned char *)s->init_buf->data; + size_t msglen; + WPACKET pkt; if (s->ctx->app_gen_cookie_cb == NULL || s->ctx->app_gen_cookie_cb(s, s->d1->cookie, @@ -877,14 +866,26 @@ int dtls_construct_hello_verify_request(SSL *s) return 0; } - len = dtls_raw_hello_verify_request(&buf[DTLS1_HM_HEADER_LENGTH], - s->d1->cookie, s->d1->cookie_len); - - dtls1_set_message_header(s, DTLS1_MT_HELLO_VERIFY_REQUEST, len, 0, len); - len += DTLS1_HM_HEADER_LENGTH; + if (!WPACKET_init(&pkt, s->init_buf) + || !ssl_set_handshake_header2(s, &pkt, + DTLS1_MT_HELLO_VERIFY_REQUEST) + || !dtls_raw_hello_verify_request(&pkt, s->d1->cookie, + s->d1->cookie_len) + /* + * We don't call close_construct_packet() because we don't want + * to buffer this message + */ + || !WPACKET_close(&pkt) + || !WPACKET_get_length(&pkt, &msglen) + || !WPACKET_finish(&pkt)) { + SSLerr(SSL_F_DTLS_CONSTRUCT_HELLO_VERIFY_REQUEST, ERR_R_INTERNAL_ERROR); + WPACKET_cleanup(&pkt); + ossl_statem_set_error(s); + return 0; + } /* number of bytes to write */ - s->init_num = len; + s->init_num = (int)msglen; s->init_off = 0; return 1; -- 2.34.1