[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Thu Sep 22 22:14:54 UTC 2016
The branch master has been updated
via c536b6be1a72aefd632d5530106a67c516cb9f4b (commit)
via 4b0fc9fc7a8767f3e6289b2b9f4527db186b3566 (commit)
from f3b3d7f0033080f86ede5a53e8af2fb313091b5a (commit)
- Log -----------------------------------------------------------------
commit c536b6be1a72aefd632d5530106a67c516cb9f4b
Author: Matt Caswell <matt at openssl.org>
Date: Wed Sep 21 11:26:47 2016 +0100
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 <rsalz at openssl.org>
commit 4b0fc9fc7a8767f3e6289b2b9f4527db186b3566
Author: Matt Caswell <matt at openssl.org>
Date: Wed Sep 21 11:20:18 2016 +0100
Add warning about a potential pitfall with WPACKET_allocate_bytes()
If the underlying BUF_MEM gets realloc'd then the pointer returned could
become invalid. Therefore we should always ensure that the allocated
memory is filled in prior to any more WPACKET_* calls.
Reviewed-by: Rich Salz <rsalz at openssl.org>
-----------------------------------------------------------------------
Summary of changes:
ssl/d1_lib.c | 116 +++++++++++++++++++++++++++--------------------
ssl/packet.c | 1 +
ssl/packet_locl.h | 5 +-
ssl/ssl_locl.h | 5 +-
ssl/statem/statem_srvr.c | 51 +++++++++++----------
5 files changed, 101 insertions(+), 77 deletions(-)
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index 043057f..f34818b 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/packet.c b/ssl/packet.c
index 6199469..0e8e876 100644
--- a/ssl/packet.c
+++ b/ssl/packet.c
@@ -224,6 +224,7 @@ int WPACKET_start_sub_packet_len__(WPACKET *pkt, size_t lenbytes)
if (!WPACKET_allocate_bytes(pkt, lenbytes, &lenchars))
return 0;
+ /* Convert to an offset in case the underlying BUF_MEM gets realloc'd */
sub->packet_len = lenchars - (unsigned char *)pkt->buf->data;
return 1;
diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h
index c51d892..44a8f82 100644
--- a/ssl/packet_locl.h
+++ b/ssl/packet_locl.h
@@ -671,6 +671,9 @@ int WPACKET_start_sub_packet(WPACKET *pkt);
* Allocate bytes in the WPACKET for the output. This reserves the bytes
* and counts them as "written", but doesn't actually do the writing. A pointer
* to the allocated bytes is stored in |*allocbytes|.
+ * WARNING: the allocated bytes must be filled in immediately, without further
+ * WPACKET_* calls. If not then the underlying buffer may be realloc'd and
+ * change its location.
*/
int WPACKET_allocate_bytes(WPACKET *pkt, size_t bytes,
unsigned char **allocbytes);
@@ -715,7 +718,7 @@ int WPACKET_put_bytes__(WPACKET *pkt, unsigned int val, size_t bytes);
#define WPACKET_put_bytes_u16(pkt, val) \
WPACKET_put_bytes__((pkt), (val), 2)
#define WPACKET_put_bytes_u24(pkt, val) \
- WPACKET_put_bytes__((pkt), (val)), 3)
+ WPACKET_put_bytes__((pkt), (val), 3)
#define WPACKET_put_bytes_u32(pkt, val) \
WPACKET_sub_allocate_bytes__((pkt), (val), 4)
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 6ad2735..630fea8 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 587bead..03d75d0 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;
More information about the openssl-commits
mailing list