Convert ServerHello construction to WPACKET

Reviewed-by: NRich Salz <rsalz@openssl.org>

ssl/d1_srtp.c

@@ -203,30 +203,6 @@ int ssl_parse_clienthello_use_srtp_ext(SSL *s, PACKET *pkt, int *al)
     return 0;
 }
 
-int ssl_add_serverhello_use_srtp_ext(SSL *s, unsigned char *p, int *len,
-                                     int maxlen)
-{
-    if (p) {
-        if (maxlen < 5) {
-            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_USE_SRTP_EXT,
-                   SSL_R_SRTP_PROTECTION_PROFILE_LIST_TOO_LONG);
-            return 1;
-        }
-
-        if (s->srtp_profile == 0) {
-            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_USE_SRTP_EXT,
-                   SSL_R_USE_SRTP_NOT_NEGOTIATED);
-            return 1;
-        }
-        s2n(2, p);
-        s2n(s->srtp_profile->id, p);
-        *p++ = 0;
-    }
-    *len = 5;
-
-    return 0;
-}
-
 int ssl_parse_serverhello_use_srtp_ext(SSL *s, PACKET *pkt, int *al)
 {
     unsigned int id, ct, mki;

ssl/ssl_locl.h

@@ -2017,8 +2017,7 @@ __owur int tls1_shared_list(SSL *s,
                             const unsigned char *l1, size_t l1len,
                             const unsigned char *l2, size_t l2len, int nmatch);
 __owur int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al);
-__owur unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
-                                                 unsigned char *limit, int *al);
+__owur int ssl_add_serverhello_tlsext(SSL *s, WPACKET *pkt, int *al);
 __owur int ssl_parse_clienthello_tlsext(SSL *s, PACKET *pkt);
 void ssl_set_default_md(SSL *s);
 __owur int tls1_set_server_sigalgs(SSL *s);
@@ -2066,8 +2065,7 @@ __owur int ssl_security_cert_chain(SSL *s, STACK_OF(X509) *sk, X509 *ex,
 
 __owur EVP_MD_CTX *ssl_replace_hash(EVP_MD_CTX **hash, const EVP_MD *md);
 void ssl_clear_hash_ctx(EVP_MD_CTX **hash);
-__owur int ssl_add_serverhello_renegotiate_ext(SSL *s, unsigned char *p,
-                                               int *len, int maxlen);
+__owur int ssl_add_serverhello_renegotiate_ext(SSL *s, WPACKET *pkt);
 __owur int ssl_parse_serverhello_renegotiate_ext(SSL *s, PACKET *pkt, int *al);
 __owur int ssl_parse_clienthello_renegotiate_ext(SSL *s, PACKET *pkt, int *al);
 __owur long ssl_get_algorithm2(SSL *s);
@@ -2084,8 +2082,6 @@ void ssl_set_client_disabled(SSL *s);
 __owur int ssl_cipher_disabled(SSL *s, const SSL_CIPHER *c, int op);
 
 __owur int ssl_parse_clienthello_use_srtp_ext(SSL *s, PACKET *pkt, int *al);
-__owur int ssl_add_serverhello_use_srtp_ext(SSL *s, unsigned char *p, int *len,
-                                            int maxlen);
 __owur int ssl_parse_serverhello_use_srtp_ext(SSL *s, PACKET *pkt, int *al);
 
 __owur int ssl_handshake_hash(SSL *s, unsigned char *out, int outlen);

ssl/statem/statem_srvr.c

@@ -1499,26 +1499,23 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
 
 int tls_construct_server_hello(SSL *s)
 {
-    unsigned char *buf;
-    unsigned char *p, *d;
-    int i, sl;
-    int al = 0;
-    unsigned long l;
-
-    buf = (unsigned char *)s->init_buf->data;
-
-    /* Do the message type and length last */
-    d = p = ssl_handshake_start(s);
-
-    *(p++) = s->version >> 8;
-    *(p++) = s->version & 0xff;
+    int sl;
+    int al = SSL_AD_INTERNAL_ERROR;
+    int compm;
+    size_t len;
+    WPACKET pkt;
 
+    if (!WPACKET_init(&pkt, s->init_buf)
+            || !ssl_set_handshake_header2(s, &pkt, SSL3_MT_SERVER_HELLO)
+            || !WPACKET_put_bytes_u16(&pkt, s->version)
                /*
                 * Random stuff. Filling of the server_random takes place in
                 * tls_process_client_hello()
                 */
-    memcpy(p, s->s3->server_random, SSL3_RANDOM_SIZE);
-    p += SSL3_RANDOM_SIZE;
+            || !WPACKET_memcpy(&pkt, s->s3->server_random, SSL3_RANDOM_SIZE)) {
+        SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR);
+        goto err;
+    }
 
     /*-
      * There are several cases for the session ID to send
@@ -1544,50 +1541,35 @@ int tls_construct_server_hello(SSL *s)
     sl = s->session->session_id_length;
     if (sl > (int)sizeof(s->session->session_id)) {
         SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR);
-        ossl_statem_set_error(s);
-        return 0;
+        goto err;
     }
-    *(p++) = sl;
-    memcpy(p, s->session->session_id, sl);
-    p += sl;
-
-    /* put the cipher */
-    i = ssl3_put_cipher_by_char_old(s->s3->tmp.new_cipher, p);
-    p += i;
 
-    /* put the compression method */
+    /* set up the compression method */
 #ifdef OPENSSL_NO_COMP
-    *(p++) = 0;
+    compm = 0;
 #else
     if (s->s3->tmp.new_compression == NULL)
-        *(p++) = 0;
+        compm = 0;
     else
-        *(p++) = s->s3->tmp.new_compression->id;
+        compm = s->s3->tmp.new_compression->id;
 #endif
 
-    if (ssl_prepare_serverhello_tlsext(s) <= 0) {
-        SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, SSL_R_SERVERHELLO_TLSEXT);
-        ossl_statem_set_error(s);
-        return 0;
-    }
-    if ((p =
-         ssl_add_serverhello_tlsext(s, p, buf + SSL3_RT_MAX_PLAIN_LENGTH,
-                                    &al)) == NULL) {
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);
+    if (!WPACKET_sub_memcpy_u8(&pkt, s->session->session_id, sl)
+            || !s->method->put_cipher_by_char(s->s3->tmp.new_cipher, &pkt, &len)
+            || !WPACKET_put_bytes_u8(&pkt, compm)
+            || !ssl_prepare_serverhello_tlsext(s)
+            || !ssl_add_serverhello_tlsext(s, &pkt, &al)
+            || !ssl_close_construct_packet(s, &pkt)) {
         SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR);
-        ossl_statem_set_error(s);
-        return 0;
+        goto err;
     }
 
-    /* do the header */
-    l = (p - d);
-    if (!ssl_set_handshake_header(s, SSL3_MT_SERVER_HELLO, l)) {
-        SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR);
+    return 1;
+ err:
+    WPACKET_cleanup(&pkt);
+    ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
     ossl_statem_set_error(s);
     return 0;
-    }
-
-    return 1;
 }
 
 int tls_construct_server_done(SSL *s)

ssl/t1_lib.c

@@ -1392,12 +1392,8 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
     return 1;
 }
 
-unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
-                                          unsigned char *limit, int *al)
+int ssl_add_serverhello_tlsext(SSL *s, WPACKET *pkt, int *al)
 {
-    int extdatalen = 0;
-    unsigned char *orig = buf;
-    unsigned char *ret = buf;
 #ifndef OPENSSL_NO_NEXTPROTONEG
     int next_proto_neg_seen;
 #endif
@@ -1408,30 +1404,16 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
     using_ecc = using_ecc && (s->session->tlsext_ecpointformatlist != NULL);
 #endif
 
-    ret += 2;
-    if (ret >= limit)
-        return NULL;            /* this really never occurs, but ... */
-
-    if (s->s3->send_connection_binding) {
-        int el;
-
-        if (!ssl_add_serverhello_renegotiate_ext(s, 0, &el, 0)) {
+    if (!WPACKET_start_sub_packet_u16(pkt)
+            || !WPACKET_set_flags(pkt, WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH)) {
         SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-            return NULL;
+        return 0;
     }
 
-        if ((limit - ret - 4 - el) < 0)
-            return NULL;
-
-        s2n(TLSEXT_TYPE_renegotiate, ret);
-        s2n(el, ret);
-
-        if (!ssl_add_serverhello_renegotiate_ext(s, ret, &el, el)) {
+    if (s->s3->send_connection_binding &&
+            !ssl_add_serverhello_renegotiate_ext(s, pkt)) {
         SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-            return NULL;
-        }
-
-        ret += el;
+        return 0;
     }
 
     /* Only add RI for SSLv3 */
@@ -1440,11 +1422,11 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
 
     if (!s->hit && s->servername_done == 1
             && s->session->tlsext_hostname != NULL) {
-        if ((long)(limit - ret - 4) < 0)
-            return NULL;
-
-        s2n(TLSEXT_TYPE_server_name, ret);
-        s2n(0, ret);
+        if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_server_name)
+                || !WPACKET_put_bytes_u16(pkt, 0)) {
+            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+            return 0;
+        }
     }
 #ifndef OPENSSL_NO_EC
     if (using_ecc) {
@@ -1453,25 +1435,15 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
         /*
          * Add TLS extension ECPointFormats to the ServerHello message
          */
-        long lenmax;
-
         tls1_get_formatlist(s, &plist, &plistlen);
 
-        if ((lenmax = limit - ret - 5) < 0)
-            return NULL;
-        if (plistlen > (size_t)lenmax)
-            return NULL;
-        if (plistlen > 255) {
+        if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_ec_point_formats)
+                || !WPACKET_start_sub_packet_u16(pkt)
+                || !WPACKET_sub_memcpy_u8(pkt, plist, plistlen)
+                || !WPACKET_close(pkt)) {
             SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-            return NULL;
+            return 0;
         }
-
-        s2n(TLSEXT_TYPE_ec_point_formats, ret);
-        s2n(plistlen + 1, ret);
-        *(ret++) = (unsigned char)plistlen;
-        memcpy(ret, plist, plistlen);
-        ret += plistlen;
-
     }
     /*
      * Currently the server should not respond with a SupportedCurves
@@ -1480,10 +1452,11 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
 #endif                          /* OPENSSL_NO_EC */
 
     if (s->tlsext_ticket_expected && tls_use_ticket(s)) {
-        if ((long)(limit - ret - 4) < 0)
-            return NULL;
-        s2n(TLSEXT_TYPE_session_ticket, ret);
-        s2n(0, ret);
+        if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_session_ticket)
+                || !WPACKET_put_bytes_u16(pkt, 0)) {
+            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+            return 0;
+        }
     } else {
         /*
          * if we don't add the above TLSEXT, we can't add a session ticket
@@ -1493,31 +1466,23 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
     }
 
     if (s->tlsext_status_expected) {
-        if ((long)(limit - ret - 4) < 0)
-            return NULL;
-        s2n(TLSEXT_TYPE_status_request, ret);
-        s2n(0, ret);
+        if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_status_request)
+                || !WPACKET_put_bytes_u16(pkt, 0)) {
+            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+            return 0;
+        }
     }
 #ifndef OPENSSL_NO_SRTP
     if (SSL_IS_DTLS(s) && s->srtp_profile) {
-        int el;
-
-        /* Returns 0 on success!! */
-        if (ssl_add_serverhello_use_srtp_ext(s, 0, &el, 0)) {
-            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-            return NULL;
-        }
-        if ((limit - ret - 4 - el) < 0)
-            return NULL;
-
-        s2n(TLSEXT_TYPE_use_srtp, ret);
-        s2n(el, ret);
-
-        if (ssl_add_serverhello_use_srtp_ext(s, ret, &el, el)) {
+        if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_use_srtp)
+                || !WPACKET_start_sub_packet_u16(pkt)
+                || !WPACKET_put_bytes_u16(pkt, 2)
+                || !WPACKET_put_bytes_u16(pkt, s->srtp_profile->id)
+                || !WPACKET_put_bytes_u8(pkt, 0)
+                || !WPACKET_close(pkt)) {
             SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
-            return NULL;
+            return 0;
         }
-        ret += el;
     }
 #endif
 
@@ -1532,28 +1497,31 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
             0x2a, 0x85, 0x03, 0x02, 0x02, 0x16, 0x30, 0x08,
             0x06, 0x06, 0x2a, 0x85, 0x03, 0x02, 0x02, 0x17
         };
-        if (limit - ret < 36)
-            return NULL;
-        memcpy(ret, cryptopro_ext, 36);
-        ret += 36;
-
+        if (!WPACKET_memcpy(pkt, cryptopro_ext, sizeof(cryptopro_ext))) {
+            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+            return 0;
+        }
     }
 #ifndef OPENSSL_NO_HEARTBEATS
     /* Add Heartbeat extension if we've received one */
     if (SSL_IS_DTLS(s) && (s->tlsext_heartbeat & SSL_DTLSEXT_HB_ENABLED)) {
-        if ((limit - ret - 4 - 1) < 0)
-            return NULL;
-        s2n(TLSEXT_TYPE_heartbeat, ret);
-        s2n(1, ret);
         /*-
          * Set mode:
          * 1: peer may send requests
          * 2: peer not allowed to send requests
          */
         if (s->tlsext_heartbeat & SSL_DTLSEXT_HB_DONT_RECV_REQUESTS)
-            *(ret++) = SSL_DTLSEXT_HB_DONT_SEND_REQUESTS;
+            mode = SSL_DTLSEXT_HB_DONT_SEND_REQUESTS;
         else
-            *(ret++) = SSL_DTLSEXT_HB_ENABLED;
+            mode = SSL_DTLSEXT_HB_ENABLED;
+
+        if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_heartbeat)
+                || !WPACKET_start_sub_packet_u16(pkt)
+                || !WPACKET_put_bytes_u8(pkt, mode)
+                || !WPACKET_close(pkt)) {
+            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+            return 0;
+        }
 
     }
 #endif
@@ -1570,18 +1538,20 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
                                               s->
                                               ctx->next_protos_advertised_cb_arg);
         if (r == SSL_TLSEXT_ERR_OK) {
-            if ((long)(limit - ret - 4 - npalen) < 0)
-                return NULL;
-            s2n(TLSEXT_TYPE_next_proto_neg, ret);
-            s2n(npalen, ret);
-            memcpy(ret, npa, npalen);
-            ret += npalen;
+            if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_next_proto_neg)
+                    || !WPACKET_sub_memcpy_u16(pkt, npa, npalen)) {
+                SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+                return 0;
+            }
             s->s3->next_proto_neg_seen = 1;
         }
     }
 #endif
-    if (!custom_ext_add_old(s, 1, &ret, limit, al))
-        return NULL;
+    if (!custom_ext_add(s, 1, pkt, al)) {
+        SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+        return 0;
+    }
+
     if (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC) {
         /*
          * Don't use encrypt_then_mac if AEAD or RC4 might want to disable
@@ -1593,36 +1563,41 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
             || s->s3->tmp.new_cipher->algorithm_enc == SSL_eGOST2814789CNT12)
             s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
         else {
-            s2n(TLSEXT_TYPE_encrypt_then_mac, ret);
-            s2n(0, ret);
+            if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_encrypt_then_mac)
+                    || !WPACKET_put_bytes_u16(pkt, 0)) {
+                SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+                return 0;
+            }
         }
     }
     if (s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS) {
-        s2n(TLSEXT_TYPE_extended_master_secret, ret);
-        s2n(0, ret);
+        if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_extended_master_secret)
+                || !WPACKET_put_bytes_u16(pkt, 0)) {
+            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+            return 0;
+        }
     }
 
     if (s->s3->alpn_selected != NULL) {
-        const unsigned char *selected = s->s3->alpn_selected;
-        unsigned int len = s->s3->alpn_selected_len;
-
-        if ((long)(limit - ret - 4 - 2 - 1 - len) < 0)
-            return NULL;
-        s2n(TLSEXT_TYPE_application_layer_protocol_negotiation, ret);
-        s2n(3 + len, ret);
-        s2n(1 + len, ret);
-        *ret++ = len;
-        memcpy(ret, selected, len);
-        ret += len;
+        if (!WPACKET_put_bytes_u16(pkt,
+                    TLSEXT_TYPE_application_layer_protocol_negotiation)
+                || !WPACKET_start_sub_packet_u16(pkt)
+                || !WPACKET_start_sub_packet_u16(pkt)
+                || !WPACKET_sub_memcpy_u8(pkt, s->s3->alpn_selected,
+                                          s->s3->alpn_selected_len)
+                || !WPACKET_close(pkt)
+                || !WPACKET_close(pkt)) {
+            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+            return 0;
+        }
     }
 
  done:
-
-    if ((extdatalen = ret - orig - 2) == 0)
-        return orig;
-
-    s2n(extdatalen, orig);
-    return ret;
+    if (!WPACKET_close(pkt)) {
+        SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR);
+        return 0;
+    }
+    return 1;
 }
 
 /*

ssl/t1_reneg.c

@@ -50,32 +50,18 @@ int ssl_parse_clienthello_renegotiate_ext(SSL *s, PACKET *pkt, int *al)
 }
 
 /* Add the server's renegotiation binding */
-int ssl_add_serverhello_renegotiate_ext(SSL *s, unsigned char *p, int *len,
-                                        int maxlen)
+int ssl_add_serverhello_renegotiate_ext(SSL *s, WPACKET *pkt)
 {
-    if (p) {
-        if ((s->s3->previous_client_finished_len +
-             s->s3->previous_server_finished_len + 1) > maxlen) {
-            SSLerr(SSL_F_SSL_ADD_SERVERHELLO_RENEGOTIATE_EXT,
-                   SSL_R_RENEGOTIATE_EXT_TOO_LONG);
+    if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_renegotiate)
+            || !WPACKET_start_sub_packet_u16(pkt)
+            || !WPACKET_start_sub_packet_u8(pkt)
+            || !WPACKET_memcpy(pkt, s->s3->previous_client_finished,
+                               s->s3->previous_client_finished_len)
+            || !WPACKET_memcpy(pkt, s->s3->previous_server_finished,
+                               s->s3->previous_server_finished_len)
+            || !WPACKET_close(pkt)
+            || !WPACKET_close(pkt))
         return 0;
-        }
-
-        /* Length byte */
-        *p = s->s3->previous_client_finished_len +
-            s->s3->previous_server_finished_len;
-        p++;
-
-        memcpy(p, s->s3->previous_client_finished,
-               s->s3->previous_client_finished_len);
-        p += s->s3->previous_client_finished_len;
-
-        memcpy(p, s->s3->previous_server_finished,
-               s->s3->previous_server_finished_len);
-    }
-
-    *len = s->s3->previous_client_finished_len
-        + s->s3->previous_server_finished_len + 1;
 
     return 1;
 }