Add a WPACKET_sub_allocate_bytes() function Updated the construction code to use the new function. Also added some convenience macros for WPACKET_sub_memcpy(). Reviewed-by: Rich Salz <rsalz@openssl.org>
diff --git a/ssl/packet.c b/ssl/packet.c index b7084b0..52c2476 100644 --- a/ssl/packet.c +++ b/ssl/packet.c
@@ -41,6 +41,17 @@ return 1; } +int WPACKET_sub_allocate_bytes(WPACKET *pkt, size_t len, + unsigned char **allocbytes, size_t lenbytes) +{ + if (!WPACKET_start_sub_packet_len(pkt, lenbytes) + || !WPACKET_allocate_bytes(pkt, len, allocbytes) + || !WPACKET_close(pkt)) + return 0; + + return 1; +} + static size_t maxmaxsize(size_t lenbytes) { if (lenbytes >= sizeof(size_t) || lenbytes == 0)
diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h index daef69e..6f16a7a 100644 --- a/ssl/packet_locl.h +++ b/ssl/packet_locl.h
@@ -675,6 +675,27 @@ unsigned char **allocbytes); /* + * The same as WPACKET_allocate_bytes() except additionally a new sub-packet is + * started for the allocated bytes, and then closed immediately afterwards. The + * number of length bytes for the sub-packet is in |lenbytes|. + */ +int WPACKET_sub_allocate_bytes(WPACKET *pkt, size_t len, + unsigned char **allocbytes, size_t lenbytes); + +/* + * Convenience macros for calling WPACKET_sub_allocate_bytes with different + * lengths + */ +#define WPACKET_sub_allocate_bytes_u8(pkt, len, bytes) \ + WPACKET_sub_allocate_bytes((pkt), (len), (bytes), 1) +#define WPACKET_sub_allocate_bytes_u16(pkt, len, bytes) \ + WPACKET_sub_allocate_bytes((pkt), (len), (bytes), 2) +#define WPACKET_sub_allocate_bytes_u24(pkt, len, bytes) \ + WPACKET_sub_allocate_bytes((pkt), (len), (bytes), 3) +#define WPACKET_sub_allocate_bytes_u32(pkt, len, bytes) \ + WPACKET_sub_allocate_bytes((pkt), (len), (bytes), 4) + +/* * Write the value stored in |val| into the WPACKET. The value will consume * |bytes| amount of storage. An error will occur if |val| cannot be * accommodated in |bytes| storage, e.g. attempting to write the value 256 into @@ -695,6 +716,16 @@ int WPACKET_sub_memcpy(WPACKET *pkt, const void *src, size_t len, size_t lenbytes); +/* Convenience macros for calling WPACKET_sub_memcpy with different lengths */ +#define WPACKET_sub_memcpy_u8(pkt, src, len) \ + WPACKET_sub_memcpy((pkt), (src), (len), 1) +#define WPACKET_sub_memcpy_u16(pkt, src, len) \ + WPACKET_sub_memcpy((pkt), (src), (len), 2) +#define WPACKET_sub_memcpy_bytes_u24(pkt, src, len) \ + WPACKET_sub_memcpy((pkt), (src), (len), 3) +#define WPACKET_sub_memcpy_bytes_u32(pkt, src, len) \ + WPACKET_sub_memcpy((pkt), (src), (len), 4) + /* * Return the total number of bytes written so far to the underlying buffer * including any storage allocated for length bytes
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 412ea1d..703e6a6 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c
@@ -804,8 +804,8 @@ /* cookie stuff for DTLS */ if (SSL_IS_DTLS(s)) { if (s->d1->cookie_len > sizeof(s->d1->cookie) - || !WPACKET_sub_memcpy(&pkt, s->d1->cookie, s->d1->cookie_len, - 1)) { + || !WPACKET_sub_memcpy_u8(&pkt, s->d1->cookie, + s->d1->cookie_len)) { SSLerr(SSL_F_TLS_CONSTRUCT_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); goto err; } @@ -2133,7 +2133,7 @@ s->session->psk_identity = tmpidentity; tmpidentity = NULL; - if (!WPACKET_sub_memcpy(pkt, identity, identitylen, 2)) { + if (!WPACKET_sub_memcpy_u16(pkt, identity, identitylen)) { SSLerr(SSL_F_TLS_CONSTRUCT_CKE_PSK_PREAMBLE, ERR_R_INTERNAL_ERROR); *al = SSL_AD_INTERNAL_ERROR; goto err; @@ -2260,9 +2260,7 @@ /* send off the data */ DH_get0_key(dh_clnt, &pub_key, NULL); - if (!WPACKET_start_sub_packet_u16(pkt) - || !WPACKET_allocate_bytes(pkt, BN_num_bytes(pub_key), &keybytes) - || !WPACKET_close(pkt)) + if (!WPACKET_sub_allocate_bytes_u16(pkt, BN_num_bytes(pub_key), &keybytes)) goto err; BN_bn2bin(pub_key, keybytes); @@ -2306,7 +2304,7 @@ goto err; } - if (!WPACKET_sub_memcpy(pkt, encodedPoint, encoded_pt_len, 1)) { + if (!WPACKET_sub_memcpy_u8(pkt, encodedPoint, encoded_pt_len)) { SSLerr(SSL_F_TLS_CONSTRUCT_CKE_ECDHE, ERR_R_INTERNAL_ERROR); goto err; } @@ -2428,7 +2426,7 @@ if (!WPACKET_put_bytes(pkt, V_ASN1_SEQUENCE | V_ASN1_CONSTRUCTED, 1) || (msglen >= 0x80 && !WPACKET_put_bytes(pkt, 0x81, 1)) - || !WPACKET_sub_memcpy(pkt, tmp, msglen, 1)) { + || !WPACKET_sub_memcpy_u8(pkt, tmp, msglen)) { *al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_CONSTRUCT_CKE_GOST, ERR_R_INTERNAL_ERROR); goto err; @@ -2463,10 +2461,8 @@ unsigned char *abytes = NULL; if (s->srp_ctx.A == NULL - || !WPACKET_start_sub_packet_u16(pkt) - || !WPACKET_allocate_bytes(pkt, BN_num_bytes(s->srp_ctx.A), - &abytes) - || !WPACKET_close(pkt)) { + || !WPACKET_sub_allocate_bytes_u16(pkt, BN_num_bytes(s->srp_ctx.A), + &abytes)) { SSLerr(SSL_F_TLS_CONSTRUCT_CKE_SRP, ERR_R_INTERNAL_ERROR); return 0; }
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 50083a9..696998f 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c
@@ -1040,8 +1040,8 @@ /* Add RI if renegotiating */ if (s->renegotiate) { if (!WPACKET_put_bytes(pkt, TLSEXT_TYPE_renegotiate, 2) - || !WPACKET_sub_memcpy(pkt, s->s3->previous_client_finished, - s->s3->previous_client_finished_len, 2)) { + || !WPACKET_sub_memcpy_u16(pkt, s->s3->previous_client_finished, + s->s3->previous_client_finished_len)) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return 0; } @@ -1058,8 +1058,8 @@ /* Sub-packet for servername list (always 1 hostname)*/ || !WPACKET_start_sub_packet_u16(pkt) || !WPACKET_put_bytes(pkt, TLSEXT_NAMETYPE_host_name, 1) - || !WPACKET_sub_memcpy(pkt, s->tlsext_hostname, - strlen(s->tlsext_hostname), 2) + || !WPACKET_sub_memcpy_u16(pkt, s->tlsext_hostname, + strlen(s->tlsext_hostname)) || !WPACKET_close(pkt) || !WPACKET_close(pkt)) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); @@ -1099,7 +1099,7 @@ if (!WPACKET_put_bytes(pkt, TLSEXT_TYPE_ec_point_formats, 2) /* Sub-packet for formats extension */ || !WPACKET_start_sub_packet_u16(pkt) - || !WPACKET_sub_memcpy(pkt, pformats, num_formats, 1) + || !WPACKET_sub_memcpy_u8(pkt, pformats, num_formats) || !WPACKET_close(pkt)) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return 0; @@ -1161,8 +1161,8 @@ goto skip_ext; if (!WPACKET_put_bytes(pkt, TLSEXT_TYPE_session_ticket, 2) - || !WPACKET_sub_memcpy(pkt, s->session->tlsext_tick, ticklen, - 2)) { + || !WPACKET_sub_memcpy_u16(pkt, s->session->tlsext_tick, + ticklen)) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return 0; } @@ -1209,10 +1209,8 @@ idlen = i2d_OCSP_RESPID(id, NULL); if (idlen <= 0 /* Sub-packet for an individual id */ - || !WPACKET_start_sub_packet_u8(pkt) - || !WPACKET_allocate_bytes(pkt, idlen, &idbytes) - || i2d_OCSP_RESPID(id, &idbytes) != idlen - || !WPACKET_close(pkt)) { + || !WPACKET_sub_allocate_bytes_u8(pkt, idlen, &idbytes) + || i2d_OCSP_RESPID(id, &idbytes) != idlen) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return 0; } @@ -1292,8 +1290,8 @@ TLSEXT_TYPE_application_layer_protocol_negotiation, 2) /* Sub-packet ALPN extension */ || !WPACKET_start_sub_packet_u16(pkt) - || !WPACKET_sub_memcpy(pkt, s->alpn_client_proto_list, - s->alpn_client_proto_list_len, 2) + || !WPACKET_sub_memcpy_u16(pkt, s->alpn_client_proto_list, + s->alpn_client_proto_list_len) || !WPACKET_close(pkt)) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return 0; @@ -1380,16 +1378,11 @@ hlen = 0; if (!WPACKET_put_bytes(pkt, TLSEXT_TYPE_padding, 2) - || !WPACKET_start_sub_packet_u16(pkt) - || !WPACKET_allocate_bytes(pkt, hlen, &padbytes)) { + || !WPACKET_sub_allocate_bytes_u16(pkt, hlen, &padbytes)) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return 0; } memset(padbytes, 0, hlen); - if (!WPACKET_close(pkt)) { - SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return 0; - } } }
diff --git a/test/wpackettest.c b/test/wpackettest.c index ca2a1a7..c2e194e 100644 --- a/test/wpackettest.c +++ b/test/wpackettest.c
@@ -325,6 +325,22 @@ return 0; } + /* Repeat with WPACKET_sub_allocate_bytes */ + if ( !WPACKET_init_len(&pkt, buf, 1) + || !WPACKET_sub_allocate_bytes(&pkt, 2, &bytes, 1)) { + testfail("test_WPACKET_allocate_bytes():3 failed\n", &pkt); + return 0; + } + bytes[0] = 0xfe; + bytes[1] = 0xff; + if ( !WPACKET_finish(&pkt) + || !WPACKET_get_total_written(&pkt, &written) + || written != sizeof(submem) + || memcmp(buf->data, &submem, written) != 0) { + testfail("test_WPACKET_allocate_bytes():4 failed\n", &pkt); + return 0; + } + return 1; }