Refactor ClientHello extension parsing

1) Simplify code with better PACKET methods.

2) Make broken SNI parsing explicit. SNI was intended to be extensible
to new name types but RFC 4366 defined the syntax inextensibly, and
OpenSSL has never parsed SNI in a way that would allow adding a new name
type. RFC 6066 fixed the definition but due to broken implementations
being widespread, it appears impossible to ever extend SNI.

3) Annotate resumption behaviour. OpenSSL doesn't currently handle all
extensions correctly upon resumption. Annotate for further clean-up.

4) Send an alert on ALPN protocol mismatch.

Reviewed-by: Kurt Roeckx <kurt@openssl.org>

1 files changed, 238 insertions, 275 deletions

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index f02317e09f..3aa01db7e5 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1732,63 +1732,62 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
 }
 
 /*
- * tls1_alpn_handle_client_hello is called to process the ALPN extension in a
- * ClientHello.  data: the contents of the extension, not including the type
- * and length.  data_len: the number of bytes in |data| al: a pointer to the
- * alert value to send in the event of a non-zero return.  returns: 0 on
- * success.
+ * Process the ALPN extension in a ClientHello.
+ * pkt: the contents of the ALPN extension, not including type and length.
+ * al: a pointer to the  alert value to send in the event of a failure.
+ * returns: 1 on success, 0 on error.
  */
 static int tls1_alpn_handle_client_hello(SSL *s, PACKET *pkt, int *al)
 {
-    unsigned int data_len;
-    unsigned int proto_len;
     const unsigned char *selected;
-    const unsigned char *data;
     unsigned char selected_len;
     int r;
+    PACKET protocol_list, save_protocol_list, protocol;
 
-    if (s->ctx->alpn_select_cb == NULL)
-        return 0;
+    *al = SSL_AD_DECODE_ERROR;
 
-    /*
-     * data should contain a uint16 length followed by a series of 8-bit,
-     * length-prefixed strings.
-     */
-    if (!PACKET_get_net_2(pkt, &data_len)
-            || PACKET_remaining(pkt) != data_len
-            || !PACKET_peek_bytes(pkt, &data, data_len))
-        goto parse_error;
+    if (!PACKET_as_length_prefixed_2(pkt, &protocol_list)
+        || PACKET_remaining(&protocol_list) < 2) {
+        return 0;
+    }
 
+    save_protocol_list = protocol_list;
     do {
-        if (!PACKET_get_1(pkt, &proto_len)
-                || proto_len == 0
-                || !PACKET_forward(pkt, proto_len))
-            goto parse_error;
-    } while (PACKET_remaining(pkt));
+        /* Protocol names can't be empty. */
+        if (!PACKET_get_length_prefixed_1(&protocol_list, &protocol)
+            || PACKET_remaining(&protocol) == 0) {
+            return 0;
+        }
+    } while (PACKET_remaining(&protocol_list) != 0);
 
-    r = s->ctx->alpn_select_cb(s, &selected, &selected_len, data, data_len,
+    if (s->ctx->alpn_select_cb == NULL)
+        return 1;
+
+    r = s->ctx->alpn_select_cb(s, &selected, &selected_len,
+                               PACKET_data(&save_protocol_list),
+                               PACKET_remaining(&save_protocol_list),
                                s->ctx->alpn_select_cb_arg);
     if (r == SSL_TLSEXT_ERR_OK) {
         OPENSSL_free(s->s3->alpn_selected);
         s->s3->alpn_selected = OPENSSL_malloc(selected_len);
         if (s->s3->alpn_selected == NULL) {
             *al = SSL_AD_INTERNAL_ERROR;
-            return -1;
+            return 0;
         }
         memcpy(s->s3->alpn_selected, selected, selected_len);
         s->s3->alpn_selected_len = selected_len;
+    } else {
+        *al = SSL_AD_NO_APPLICATION_PROTOCOL;
+        return 0;
     }
-    return 0;
 
- parse_error:
-    *al = SSL_AD_DECODE_ERROR;
-    return -1;
+    return 1;
 }
 
 #ifndef OPENSSL_NO_EC
 /*-
  * ssl_check_for_safari attempts to fingerprint Safari using OS X
- * SecureTransport using the TLS extension block in |d|, of length |n|.
+ * SecureTransport using the TLS extension block in |pkt|.
  * Safari, since 10.6, sends exactly these extensions, in this order:
  *   SNI,
  *   elliptic_curves
@@ -1801,9 +1800,9 @@ static int tls1_alpn_handle_client_hello(SSL *s, PACKET *pkt, int *al)
  */
 static void ssl_check_for_safari(SSL *s, const PACKET *pkt)
 {
-    unsigned int type, size;
-    const unsigned char *eblock1, *eblock2;
-    PACKET tmppkt;
+    unsigned int type;
+    PACKET sni, tmppkt;
+    size_t ext_len;
 
     static const unsigned char kSafariExtensionsBlock[] = {
         0x00, 0x0a,             /* elliptic_curves extension */
@@ -1817,10 +1816,7 @@ static void ssl_check_for_safari(SSL *s, const PACKET *pkt)
         0x00, 0x02,             /* 2 bytes */
         0x01,                   /* 1 point format */
         0x00,                   /* uncompressed */
-    };
-
-    /* The following is only present in TLS 1.2 */
-    static const unsigned char kSafariTLS12ExtensionsBlock[] = {
+        /* The following is only present in TLS 1.2 */
         0x00, 0x0d,             /* signature_algorithms */
         0x00, 0x0c,             /* 12 bytes */
         0x00, 0x0a,             /* 10 bytes */
@@ -1831,51 +1827,46 @@ static void ssl_check_for_safari(SSL *s, const PACKET *pkt)
         0x02, 0x03,             /* SHA-1/ECDSA */
     };
 
+    /* Length of the common prefix (first two extensions). */
+    static const size_t kSafariCommonExtensionsLength = 18;
+
     tmppkt = *pkt;
 
     if (!PACKET_forward(&tmppkt, 2)
-            || !PACKET_get_net_2(&tmppkt, &type)
-            || !PACKET_get_net_2(&tmppkt, &size)
-            || !PACKET_forward(&tmppkt, size))
+        || !PACKET_get_net_2(&tmppkt, &type)
+        || !PACKET_get_length_prefixed_2(&tmppkt, &sni)) {
         return;
+    }
 
     if (type != TLSEXT_TYPE_server_name)
         return;
 
-    if (TLS1_get_client_version(s) >= TLS1_2_VERSION) {
-        const size_t len1 = sizeof(kSafariExtensionsBlock);
-        const size_t len2 = sizeof(kSafariTLS12ExtensionsBlock);
-
-        if (!PACKET_get_bytes(&tmppkt, &eblock1, len1)
-                || !PACKET_get_bytes(&tmppkt, &eblock2, len2)
-                || PACKET_remaining(&tmppkt))
-            return;
-        if (memcmp(eblock1, kSafariExtensionsBlock, len1) != 0)
-            return;
-        if (memcmp(eblock2, kSafariTLS12ExtensionsBlock, len2) != 0)
-            return;
-    } else {
-        const size_t len = sizeof(kSafariExtensionsBlock);
-
-        if (!PACKET_get_bytes(&tmppkt, &eblock1, len)
-                || PACKET_remaining(&tmppkt))
-            return;
-        if (memcmp(eblock1, kSafariExtensionsBlock, len) != 0)
-            return;
-    }
+    ext_len = TLS1_get_client_version(s) >= TLS1_2_VERSION ?
+        sizeof(kSafariExtensionsBlock) : kSafariCommonExtensionsLength;
 
-    s->s3->is_probably_safari = 1;
+    s->s3->is_probably_safari = PACKET_equal(&tmppkt, kSafariExtensionsBlock,
+                                             ext_len);
 }
 #endif                         /* !OPENSSL_NO_EC */
 
+/*
+ * Parse ClientHello extensions and stash extension info in various parts of
+ * the SSL object. Verify that there are no duplicate extensions.
+ *
+ * Behaviour upon resumption is extension-specific. If the extension has no
+ * effect during resumption, it is parsed (to verify its format) but otherwise
+ * ignored.
+ *
+ * Consumes the entire packet in |pkt|. Returns 1 on success and 0 on failure.
+ * Upon failure, sets |al| to the appropriate alert.
+ */
 static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
 {
     unsigned int type;
-    unsigned int size;
-    unsigned int len;
-    const unsigned char *data;
     int renegotiate_seen = 0;
+    PACKET extensions;
 
+    *al = SSL_AD_DECODE_ERROR;
     s->servername_done = 0;
     s->tlsext_status_type = -1;
 #ifndef OPENSSL_NO_NEXTPROTONEG
@@ -1911,29 +1902,29 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
     if (PACKET_remaining(pkt) == 0)
         goto ri_check;
 
-    if (!PACKET_get_net_2(pkt, &len))
-        goto err;
-
-    if (PACKET_remaining(pkt) != len)
-        goto err;
-
-    if (!tls1_check_duplicate_extensions(pkt))
-        goto err;
+    if (!PACKET_as_length_prefixed_2(pkt, &extensions))
+        return 0;
 
-    while (PACKET_get_net_2(pkt, &type) && PACKET_get_net_2(pkt, &size)) {
-        PACKET subpkt;
+    if (!tls1_check_duplicate_extensions(&extensions))
+        return 0;
 
-        if (!PACKET_peek_bytes(pkt, &data, size))
-            goto err;
+    /*
+     * We parse all extensions to ensure the ClientHello is well-formed but,
+     * unless an extension specifies otherwise, we ignore extensions upon
+     * resumption.
+     */
+    while (PACKET_get_net_2(&extensions, &type)) {
+        PACKET extension;
+        if (!PACKET_get_length_prefixed_2(&extensions, &extension))
+            return 0;
 
         if (s->tlsext_debug_cb)
-            s->tlsext_debug_cb(s, 0, type, data, size, s->tlsext_debug_arg);
-
-        if (!PACKET_get_sub_packet(pkt, &subpkt, size))
-            goto err;
+            s->tlsext_debug_cb(s, 0, type, PACKET_data(&extension),
+                               PACKET_remaining(&extension),
+                               s->tlsext_debug_arg);
 
         if (type == TLSEXT_TYPE_renegotiate) {
-            if (!ssl_parse_clienthello_renegotiate_ext(s, &subpkt, al))
+            if (!ssl_parse_clienthello_renegotiate_ext(s, &extension, al))
                 return 0;
             renegotiate_seen = 1;
         } else if (s->version == SSL3_VERSION) {
@@ -1964,219 +1955,185 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
  */
 
         else if (type == TLSEXT_TYPE_server_name) {
-            const unsigned char *sdata;
             unsigned int servname_type;
-            unsigned int dsize;
-            PACKET ssubpkt;
-
-            if (!PACKET_get_net_2(&subpkt, &dsize)
-                    || !PACKET_get_sub_packet(&subpkt, &ssubpkt, dsize))
-                goto err;
-
-            while (PACKET_remaining(&ssubpkt) > 3) {
-                if (!PACKET_get_1(&ssubpkt, &servname_type)
-                        || !PACKET_get_net_2(&ssubpkt, &len)
-                        || PACKET_remaining(&ssubpkt) < len)
-                    goto err;
-
-                if (s->servername_done == 0)
-                    switch (servname_type) {
-                    case TLSEXT_NAMETYPE_host_name:
-                        if (!s->hit) {
-                            if (s->session->tlsext_hostname)
-                                goto err;
-
-                            if (len > TLSEXT_MAXLEN_host_name) {
-                                *al = TLS1_AD_UNRECOGNIZED_NAME;
-                                return 0;
-                            }
-                            if ((s->session->tlsext_hostname =
-                                 OPENSSL_malloc(len + 1)) == NULL) {
-                                *al = TLS1_AD_INTERNAL_ERROR;
-                                return 0;
-                            }
-                            if (!PACKET_copy_bytes(&ssubpkt,
-                                    (unsigned char *)s->session
-                                        ->tlsext_hostname,
-                                    len)) {
-                                *al = SSL_AD_DECODE_ERROR;
-                                return 0;
-                            }
-                            s->session->tlsext_hostname[len] = '\0';
-                            if (strlen(s->session->tlsext_hostname) != len) {
-                                OPENSSL_free(s->session->tlsext_hostname);
-                                s->session->tlsext_hostname = NULL;
-                                *al = TLS1_AD_UNRECOGNIZED_NAME;
-                                return 0;
-                            }
-                            s->servername_done = 1;
-
-                        } else {
-                            if (!PACKET_get_bytes(&ssubpkt, &sdata, len)) {
-                                *al = SSL_AD_DECODE_ERROR;
-                                return 0;
-                            }
-                            s->servername_done = s->session->tlsext_hostname
-                                && strlen(s->session->tlsext_hostname) == len
-                                && strncmp(s->session->tlsext_hostname,
-                                           (char *)sdata, len) == 0;
-                        }
-
-                        break;
-
-                    default:
-                        break;
-                    }
+            PACKET sni, hostname;
+
+            if (!PACKET_as_length_prefixed_2(&extension, &sni)
+                /* ServerNameList must be at least 1 byte long. */
+                || PACKET_remaining(&sni) == 0) {
+                return 0;
             }
-            /* We shouldn't have any bytes left */
-            if (PACKET_remaining(&ssubpkt) != 0)
-                goto err;
 
+            /*
+             * Although the server_name extension was intended to be
+             * extensible to new name types, RFC 4366 defined the
+             * syntax inextensibly and OpenSSL 1.0.x parses it as
+             * such.
+             * RFC 6066 corrected the mistake but adding new name types
+             * is nevertheless no longer feasible, so act as if no other
+             * SNI types can exist, to simplify parsing.
+             *
+             * Also note that the RFC permits only one SNI value per type,
+             * i.e., we can only have a single hostname.
+             */
+            if (!PACKET_get_1(&sni, &servname_type)
+                || servname_type != TLSEXT_NAMETYPE_host_name
+                || !PACKET_as_length_prefixed_2(&sni, &hostname)) {
+                return 0;
+            }
+
+            if (!s->hit) {
+                if (PACKET_remaining(&hostname) > TLSEXT_MAXLEN_host_name) {
+                    *al = TLS1_AD_UNRECOGNIZED_NAME;
+                    return 0;
+                }
+
+                if (PACKET_contains_zero_byte(&hostname)) {
+                    *al = TLS1_AD_UNRECOGNIZED_NAME;
+                    return 0;
+                }
+
+                if (!PACKET_strndup(&hostname, &s->session->tlsext_hostname)) {
+                    *al = TLS1_AD_INTERNAL_ERROR;
+                    return 0;
+                }
+
+                s->servername_done = 1;
+            } else {
+                /*
+                 * TODO(openssl-team): if the SNI doesn't match, we MUST
+                 * fall back to a full handshake.
+                 */
+                s->servername_done = s->session->tlsext_hostname
+                    && PACKET_equal(&hostname, s->session->tlsext_hostname,
+                                    strlen(s->session->tlsext_hostname));
+            }
         }
 #ifndef OPENSSL_NO_SRP
         else if (type == TLSEXT_TYPE_srp) {
-            if (!PACKET_get_1(&subpkt, &len)
-                    || s->srp_ctx.login != NULL)
-                goto err;
-
-            if ((s->srp_ctx.login = OPENSSL_malloc(len + 1)) == NULL)
-                return -1;
-            if (!PACKET_copy_bytes(&subpkt, (unsigned char *)s->srp_ctx.login,
-                                   len))
-                goto err;
-            s->srp_ctx.login[len] = '\0';
-
-            if (strlen(s->srp_ctx.login) != len
-                    || PACKET_remaining(&subpkt))
-                goto err;
+            PACKET srp_I;
+
+            if (!PACKET_as_length_prefixed_1(&extension, &srp_I))
+                return 0;
+
+            if (PACKET_contains_zero_byte(&srp_I))
+                return 0;
+
+            /*
+             * TODO(openssl-team): currently, we re-authenticate the user
+             * upon resumption. Instead, we MUST ignore the login.
+             */
+            if (!PACKET_strndup(&srp_I, &s->srp_ctx.login)) {
+                *al = TLS1_AD_INTERNAL_ERROR;
+                return 0;
+            }
         }
 #endif
 
 #ifndef OPENSSL_NO_EC
         else if (type == TLSEXT_TYPE_ec_point_formats) {
-            unsigned int ecpointformatlist_length;
+            PACKET ec_point_format_list;
 
-            if (!PACKET_get_1(&subpkt, &ecpointformatlist_length)
-                    || ecpointformatlist_length == 0)
-                goto err;
+            if (!PACKET_as_length_prefixed_1(&extension,
+                                              &ec_point_format_list)
+                || PACKET_remaining(&ec_point_format_list) == 0) {
+                return 0;
+            }
 
             if (!s->hit) {
-                OPENSSL_free(s->session->tlsext_ecpointformatlist);
-                s->session->tlsext_ecpointformatlist = NULL;
-                s->session->tlsext_ecpointformatlist_length = 0;
-                if ((s->session->tlsext_ecpointformatlist =
-                     OPENSSL_malloc(ecpointformatlist_length)) == NULL) {
+                if (!PACKET_memdup(&ec_point_format_list,
+                                   &s->session->tlsext_ecpointformatlist,
+                                   &s->session->tlsext_ecpointformatlist_length)) {
                     *al = TLS1_AD_INTERNAL_ERROR;
                     return 0;
                 }
-                s->session->tlsext_ecpointformatlist_length =
-                    ecpointformatlist_length;
-                if (!PACKET_copy_bytes(&subpkt,
-                        s->session->tlsext_ecpointformatlist,
-                        ecpointformatlist_length))
-                    goto err;
-            } else if (!PACKET_forward(&subpkt, ecpointformatlist_length)) {
-                goto err;
-            }
-            /* We should have consumed all the bytes by now */
-            if (PACKET_remaining(&subpkt)) {
-                *al = TLS1_AD_DECODE_ERROR;
-                return 0;
             }
         } else if (type == TLSEXT_TYPE_elliptic_curves) {
-            unsigned int ellipticcurvelist_length;
+            PACKET elliptic_curve_list;
 
-            /* Each NamedCurve is 2 bytes and we must have at least 1 */
-            if (!PACKET_get_net_2(&subpkt, &ellipticcurvelist_length)
-                    || ellipticcurvelist_length == 0
-                    || (ellipticcurvelist_length & 1) != 0)
-                goto err;
+            /* Each NamedCurve is 2 bytes and we must have at least 1. */
+            if (!PACKET_as_length_prefixed_2(&extension,
+                                             &elliptic_curve_list)
+                || PACKET_remaining(&elliptic_curve_list) == 0
+                || (PACKET_remaining(&elliptic_curve_list) % 2) != 0) {
+                return 0;
+            }
 
             if (!s->hit) {
-                if (s->session->tlsext_ellipticcurvelist)
-                    goto err;
-
-                s->session->tlsext_ellipticcurvelist_length = 0;
-                if ((s->session->tlsext_ellipticcurvelist =
-                     OPENSSL_malloc(ellipticcurvelist_length)) == NULL) {
+                if (!PACKET_memdup(&elliptic_curve_list,
+                                   &s->session->tlsext_ellipticcurvelist,
+                                   &s->session->tlsext_ellipticcurvelist_length)) {
                     *al = TLS1_AD_INTERNAL_ERROR;
                     return 0;
                 }
-                s->session->tlsext_ellipticcurvelist_length =
-                    ellipticcurvelist_length;
-                if (!PACKET_copy_bytes(&subpkt,
-                        s->session->tlsext_ellipticcurvelist,
-                        ellipticcurvelist_length))
-                    goto err;
-            } else if (!PACKET_forward(&subpkt, ellipticcurvelist_length)) {
-                goto err;
-            }
-            /* We should have consumed all the bytes by now */
-            if (PACKET_remaining(&subpkt)) {
-                goto err;
             }
         }
 #endif                         /* OPENSSL_NO_EC */
         else if (type == TLSEXT_TYPE_session_ticket) {
-            if (!PACKET_forward(&subpkt, size)
-                || (s->tls_session_ticket_ext_cb &&
-                    !s->tls_session_ticket_ext_cb(s, data, size,
-                                        s->tls_session_ticket_ext_cb_arg))) {
+            if (s->tls_session_ticket_ext_cb &&
+                !s->tls_session_ticket_ext_cb(s, PACKET_data(&extension),
+                                              PACKET_remaining(&extension),
+                                              s->tls_session_ticket_ext_cb_arg)) {
                 *al = TLS1_AD_INTERNAL_ERROR;
                 return 0;
             }
         } else if (type == TLSEXT_TYPE_signature_algorithms) {
-            unsigned int dsize;
-
-            if (s->s3->tmp.peer_sigalgs
-                    || !PACKET_get_net_2(&subpkt, &dsize)
-                    || (dsize & 1) != 0
-                    || (dsize == 0)
-                    || !PACKET_get_bytes(&subpkt, &data, dsize)
-                    || PACKET_remaining(&subpkt) != 0
-                    || !tls1_save_sigalgs(s, data, dsize)) {
-                goto err;
+            PACKET supported_sig_algs;
+
+            if (!PACKET_as_length_prefixed_2(&extension, &supported_sig_algs)
+                || (PACKET_remaining(&supported_sig_algs) % 2) != 0
+                || PACKET_remaining(&supported_sig_algs) == 0) {
+                return 0;
+            }
+
+            if  (!s->hit) {
+                if (!tls1_save_sigalgs(s, PACKET_data(&supported_sig_algs),
+                                       PACKET_remaining(&supported_sig_algs))) {
+                    return 0;
+                }
             }
         } else if (type == TLSEXT_TYPE_status_request) {
-            PACKET ssubpkt;
+            const unsigned char *ext_data;
 
-            if (!PACKET_get_1(&subpkt,
-                              (unsigned int *)&s->tlsext_status_type))
-                goto err;
+            if (!PACKET_get_1(&extension,
+                              (unsigned int *)&s->tlsext_status_type)) {
+                return 0;
+            }
 
             if (s->tlsext_status_type == TLSEXT_STATUSTYPE_ocsp) {
-                const unsigned char *sdata;
-                unsigned int dsize;
-                /* Read in responder_id_list */
-                if (!PACKET_get_net_2(&subpkt, &dsize)
-                        || !PACKET_get_sub_packet(&subpkt, &ssubpkt, dsize))
-                    goto err;
-
-                while (PACKET_remaining(&ssubpkt)) {
+                PACKET responder_id_list, exts;
+                if (!PACKET_get_length_prefixed_2(&extension, &responder_id_list))
+                    return 0;
+
+                while (PACKET_remaining(&responder_id_list) > 0) {
                     OCSP_RESPID *id;
-                    unsigned int idsize;
+                    PACKET responder_id;
+                    const unsigned char *id_data;
 
-                    if (PACKET_remaining(&ssubpkt) < 4
-                            || !PACKET_get_net_2(&ssubpkt, &idsize)
-                            || !PACKET_get_bytes(&ssubpkt, &data, idsize)) {
-                        goto err;
+                    if (!PACKET_get_length_prefixed_2(&responder_id_list,
+                                                      &responder_id)
+                        || PACKET_remaining(&responder_id) == 0) {
+                        return 0;
                     }
-                    sdata = data;
-                    data += idsize;
-                    id = d2i_OCSP_RESPID(NULL, &sdata, idsize);
-                    if (!id)
-                        goto err;
-                    if (data != sdata) {
-                        OCSP_RESPID_free(id);
-                        goto err;
+
+                    if (s->tlsext_ocsp_ids == NULL
+                        && (s->tlsext_ocsp_ids =
+                            sk_OCSP_RESPID_new_null()) == NULL) {
+                        *al = SSL_AD_INTERNAL_ERROR;
+                        return 0;
                     }
-                    if (!s->tlsext_ocsp_ids
-                        && !(s->tlsext_ocsp_ids =
-                             sk_OCSP_RESPID_new_null())) {
+
+                    id_data = PACKET_data(&responder_id);
+                    id = d2i_OCSP_RESPID(NULL, &id_data,
+                                         PACKET_remaining(&responder_id));
+                    if (id == NULL)
+                        return 0;
+
+                    if (id_data != PACKET_end(&responder_id)) {
                         OCSP_RESPID_free(id);
-                        *al = SSL_AD_INTERNAL_ERROR;
                         return 0;
                     }
+
                     if (!sk_OCSP_RESPID_push(s->tlsext_ocsp_ids, id)) {
                         OCSP_RESPID_free(id);
                         *al = SSL_AD_INTERNAL_ERROR;
@@ -2185,33 +2142,34 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
                 }
 
                 /* Read in request_extensions */
-                if (!PACKET_get_net_2(&subpkt, &dsize)
-                        || !PACKET_get_bytes(&subpkt, &data, dsize)
-                        || PACKET_remaining(&subpkt)) {
-                    goto err;
-                }
-                sdata = data;
-                if (dsize > 0) {
+                if (!PACKET_as_length_prefixed_2(&extension, &exts))
+                    return 0;
+
+                if (PACKET_remaining(&exts) > 0) {
+                    ext_data = PACKET_data(&exts);
                     sk_X509_EXTENSION_pop_free(s->tlsext_ocsp_exts,
                                                X509_EXTENSION_free);
                     s->tlsext_ocsp_exts =
-                        d2i_X509_EXTENSIONS(NULL, &sdata, dsize);
-                    if (!s->tlsext_ocsp_exts || (data + dsize != sdata))
-                        goto err;
+                        d2i_X509_EXTENSIONS(NULL, &ext_data,
+                                            PACKET_remaining(&exts));
+                    if (s->tlsext_ocsp_exts == NULL
+                        || ext_data != PACKET_end(&exts)) {
+                        return 0;
+                    }
                 }
-            }
             /*
              * We don't know what to do with any other type * so ignore it.
              */
-            else
+            } else {
                 s->tlsext_status_type = -1;
+            }
         }
 #ifndef OPENSSL_NO_HEARTBEATS
         else if (SSL_IS_DTLS(s) && type == TLSEXT_TYPE_heartbeat) {
             unsigned int hbtype;
 
-            if (!PACKET_get_1(&subpkt, &hbtype)
-                    || PACKET_remaining(&subpkt)) {
+            if (!PACKET_get_1(&extension, &hbtype)
+                    || PACKET_remaining(&extension)) {
                 *al = SSL_AD_DECODE_ERROR;
                 return 0;
             }
@@ -2255,8 +2213,8 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
 #endif
 
         else if (type == TLSEXT_TYPE_application_layer_protocol_negotiation &&
-                 s->ctx->alpn_select_cb && s->s3->tmp.finish_md_len == 0) {
-            if (tls1_alpn_handle_client_hello(s, &subpkt, al) != 0)
+                 s->s3->tmp.finish_md_len == 0) {
+            if (!tls1_alpn_handle_client_hello(s, &extension, al))
                 return 0;
 #ifndef OPENSSL_NO_NEXTPROTONEG
             /* ALPN takes precedence over NPN. */
@@ -2268,7 +2226,7 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
 #ifndef OPENSSL_NO_SRTP
         else if (SSL_IS_DTLS(s) && SSL_get_srtp_profiles(s)
                  && type == TLSEXT_TYPE_use_srtp) {
-            if (ssl_parse_clienthello_use_srtp_ext(s, &subpkt, al))
+            if (ssl_parse_clienthello_use_srtp_ext(s, &extension, al))
                 return 0;
         }
 #endif
@@ -2289,14 +2247,17 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
          * ServerHello may be later returned.
          */
         else if (!s->hit) {
-            if (custom_ext_parse(s, 1, type, data, size, al) <= 0)
+            if (custom_ext_parse(s, 1, type, PACKET_data(&extension),
+                                 PACKET_remaining(&extension), al) <= 0)
                 return 0;
         }
     }
 
-    /* Spurious data on the end */
-    if (PACKET_remaining(pkt) != 0)
-        goto err;
+    if (PACKET_remaining(pkt) != 0) {
+        /* tls1_check_duplicate_extensions should ensure this never happens. */
+        *al = SSL_AD_INTERNAL_ERROR;
+        return 0;
+    }
 
  ri_check:
 
@@ -2310,10 +2271,13 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
         return 0;
     }
 
+    /*
+     * This function currently has no state to clean up, so it returns directly.
+     * If parsing fails at any point, the function returns early.
+     * The SSL object may be left with partial data from extensions, but it must
+     * then no longer be used, and clearing it up will free the leftovers.
+     */
     return 1;
-err:
-    *al = SSL_AD_DECODE_ERROR;
-    return 0;
 }
 
 int ssl_parse_clienthello_tlsext(SSL *s, PACKET *pkt)
@@ -2324,7 +2288,6 @@ int ssl_parse_clienthello_tlsext(SSL *s, PACKET *pkt)
         ssl3_send_alert(s, SSL3_AL_FATAL, al);
         return 0;
     }
-
     if (ssl_check_clienthello_tlsext_early(s) <= 0) {
         SSLerr(SSL_F_SSL_PARSE_CLIENTHELLO_TLSEXT, SSL_R_CLIENTHELLO_TLSEXT);
         return 0;