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: NRich Salz <rsalz@openssl.org>

ssl/packet.c

@@ -41,6 +41,17 @@ int WPACKET_allocate_bytes(WPACKET *pkt, size_t len, unsigned char **allocbytes)
     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)

ssl/packet_locl.h

@@ -674,6 +674,27 @@ int WPACKET_start_sub_packet(WPACKET *pkt);
 int WPACKET_allocate_bytes(WPACKET *pkt, size_t bytes,
                            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
@@ -695,6 +716,16 @@ int WPACKET_memcpy(WPACKET *pkt, const void *src, size_t len);
 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

ssl/statem/statem_clnt.c

@@ -804,8 +804,8 @@ int tls_construct_client_hello(SSL *s)
     /* 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 @@ static int tls_construct_cke_psk_preamble(SSL *s, WPACKET *pkt, int *al)
     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 @@ static int tls_construct_cke_dhe(SSL *s, WPACKET *pkt, int *al)
 
     /* 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 @@ static int tls_construct_cke_ecdhe(SSL *s, WPACKET *pkt, int *al)
         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 @@ static int tls_construct_cke_gost(SSL *s, WPACKET *pkt, int *al)
 
     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 @@ static int tls_construct_cke_srp(SSL *s, WPACKET *pkt, int *al)
     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;
     }

ssl/t1_lib.c

@@ -1040,8 +1040,8 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
     /* 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 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
                    /* 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 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
         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 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
             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 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
             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 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
                     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 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
                 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;
-            }
         }
     }
 

test/wpackettest.c

@@ -325,6 +325,22 @@ static int test_WPACKET_allocate_bytes(void)
         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;
 }