Refactor ClientHello extension parsing
authorEmilia Kasper <emilia@openssl.org>
Tue, 22 Sep 2015 13:20:26 +0000 (15:20 +0200)
committerEmilia Kasper <emilia@openssl.org>
Thu, 3 Mar 2016 12:53:26 +0000 (13:53 +0100)
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>
CHANGES
include/openssl/ssl.h
include/openssl/tls1.h
ssl/packet_locl.h
ssl/s3_enc.c
ssl/t1_enc.c
ssl/t1_lib.c
test/packettest.c
test/recipes/80-test_ssl.t

diff --git a/CHANGES b/CHANGES
index 0b8c558b0670bcb7378ad203aff3cb667329f000..618655816f94bd154a40ee0c0e381ceb17d008c8 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,12 @@
 
  Changes between 1.0.2g and 1.1.0  [xx XXX xxxx]
 
+  *) If the server has ALPN configured, but supports no protocols that the
+     client advertises, send a fatal "no_application_protocol" alert.
+     This behaviour is SHALL in RFC 7301, though it isn't universally
+     implemented by other servers.
+     [Emilia Käsper]
+
   *) Add X25519 support.
      Integrate support for X25519 into EC library. This includes support
      for public and private key encoding using the format documented in
index c9119e345e4aa3f193ccae857db36130075c6ec6..6e223960a9002c415dfcec01b0ada112a0aa8334 100644 (file)
@@ -1109,6 +1109,7 @@ DECLARE_PEM_rw(SSL_SESSION, SSL_SESSION)
 # define SSL_AD_UNKNOWN_PSK_IDENTITY     TLS1_AD_UNKNOWN_PSK_IDENTITY
 /* fatal */
 # define SSL_AD_INAPPROPRIATE_FALLBACK   TLS1_AD_INAPPROPRIATE_FALLBACK
+# define SSL_AD_NO_APPLICATION_PROTOCOL  TLS1_AD_NO_APPLICATION_PROTOCOL
 # define SSL_ERROR_NONE                  0
 # define SSL_ERROR_SSL                   1
 # define SSL_ERROR_WANT_READ             2
index 0f0d4a3713d6a9d5230fb679aa7778c23a781bf3..c2fe36430b94eaeeba4c6f40bbe2a3f9c5e89ff1 100644 (file)
@@ -204,6 +204,7 @@ extern "C" {
 # define TLS1_AD_BAD_CERTIFICATE_STATUS_RESPONSE 113
 # define TLS1_AD_BAD_CERTIFICATE_HASH_VALUE 114
 # define TLS1_AD_UNKNOWN_PSK_IDENTITY    115/* fatal */
+# define TLS1_AD_NO_APPLICATION_PROTOCOL 120 /* fatal */
 
 /* ExtensionType values from RFC3546 / RFC4366 / RFC6066 */
 # define TLSEXT_TYPE_server_name                 0
index 48767b6728d40a0626c5ff198e3d2708ba729c2a..fd1f9f4ecf8bad9dee4e6c338d1ad9efbb0db81a 100644 (file)
@@ -92,6 +92,16 @@ static ossl_inline size_t PACKET_remaining(const PACKET *pkt)
     return pkt->remaining;
 }
 
+/*
+ * Returns a pointer to the first byte after the packet data.
+ * Useful for integrating with non-PACKET parsing code.
+ * Specifically, we use PACKET_end() to verify that a d2i_... call
+ * has consumed the entire packet contents.
+ */
+static ossl_inline const unsigned char *PACKET_end(const PACKET *pkt)
+{
+    return pkt->curr + pkt->remaining;
+}
 /*
  * Returns a pointer to the PACKET's current position.
  * For use in non-PACKETized APIs.
@@ -452,6 +462,12 @@ __owur static ossl_inline int PACKET_strndup(const PACKET *pkt, char **data)
     return (*data != NULL);
 }
 
+/* Returns 1 if |pkt| contains at least one 0-byte, 0 otherwise. */
+static ossl_inline int PACKET_contains_zero_byte(const PACKET *pkt)
+{
+  return memchr(pkt->curr, 0, pkt->remaining) != NULL;
+}
+
 /* Move the current reading position forward |len| bytes */
 __owur static ossl_inline int PACKET_forward(PACKET *pkt, size_t len)
 {
@@ -488,6 +504,28 @@ __owur static ossl_inline int PACKET_get_length_prefixed_1(PACKET *pkt,
     return 1;
 }
 
+/*
+ * Like PACKET_get_length_prefixed_1, but additionally, fails when there are
+ * leftover bytes in |pkt|.
+ */
+__owur static ossl_inline int PACKET_as_length_prefixed_1(PACKET *pkt, PACKET *subpkt)
+{
+  unsigned int length;
+  const unsigned char *data;
+  PACKET tmp = *pkt;
+  if (!PACKET_get_1(&tmp, &length) ||
+      !PACKET_get_bytes(&tmp, &data, (size_t)length) ||
+      PACKET_remaining(&tmp) != 0) {
+      return 0;
+  }
+
+  *pkt = tmp;
+  subpkt->curr = data;
+  subpkt->remaining = length;
+
+  return 1;
+}
+
 /*
  * Reads a variable-length vector prefixed with a two-byte length, and stores
  * the contents in |subpkt|. |pkt| can equal |subpkt|.
@@ -501,6 +539,7 @@ __owur static ossl_inline int PACKET_get_length_prefixed_2(PACKET *pkt,
     unsigned int length;
     const unsigned char *data;
     PACKET tmp = *pkt;
+
     if (!PACKET_get_net_2(&tmp, &length) ||
         !PACKET_get_bytes(&tmp, &data, (size_t)length)) {
         return 0;
@@ -513,6 +552,30 @@ __owur static ossl_inline int PACKET_get_length_prefixed_2(PACKET *pkt,
     return 1;
 }
 
+/*
+ * Like PACKET_get_length_prefixed_2, but additionally, fails when there are
+ * leftover bytes in |pkt|.
+ */
+__owur static ossl_inline int PACKET_as_length_prefixed_2(PACKET *pkt,
+                                                          PACKET *subpkt)
+{
+  unsigned int length;
+  const unsigned char *data;
+  PACKET tmp = *pkt;
+
+  if (!PACKET_get_net_2(&tmp, &length) ||
+      !PACKET_get_bytes(&tmp, &data, (size_t)length) ||
+      PACKET_remaining(&tmp) != 0) {
+      return 0;
+  }
+
+  *pkt = tmp;
+  subpkt->curr = data;
+  subpkt->remaining = length;
+
+  return 1;
+}
+
 /*
  * Reads a variable-length vector prefixed with a three-byte length, and stores
  * the contents in |subpkt|. |pkt| can equal |subpkt|.
index d4d64d0e5e2b16888ea994c20cd2944428065209..1c493e28074f2a361549f119f6f69a45374ebe8e 100644 (file)
@@ -667,6 +667,8 @@ int ssl3_alert_code(int code)
         return (TLS1_AD_UNKNOWN_PSK_IDENTITY);
     case SSL_AD_INAPPROPRIATE_FALLBACK:
         return (TLS1_AD_INAPPROPRIATE_FALLBACK);
+    case SSL_AD_NO_APPLICATION_PROTOCOL:
+        return (TLS1_AD_NO_APPLICATION_PROTOCOL);
     default:
         return (-1);
     }
index 1b2820bff9afd7966e224123df09e8f70fd13588..21eb3283da6682a7ff610c46d3447a475e0cf5e4 100644 (file)
@@ -792,6 +792,8 @@ int tls1_alert_code(int code)
         return (TLS1_AD_UNKNOWN_PSK_IDENTITY);
     case SSL_AD_INAPPROPRIATE_FALLBACK:
         return (TLS1_AD_INAPPROPRIATE_FALLBACK);
+    case SSL_AD_NO_APPLICATION_PROTOCOL:
+        return (TLS1_AD_NO_APPLICATION_PROTOCOL);
     default:
         return (-1);
     }
index f02317e09f8cd3740aa186e9993f993f3e317848..3aa01db7e57e20c0f503c989acaa60639f2afe53 100644 (file)
@@ -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;
index 0555c7be74955be1f4063b77271011320dc3cf7e..57ef51bcf26d95722d340e7f4b7e4f6aae4ceb08 100644 (file)
@@ -77,6 +77,24 @@ static int test_PACKET_remaining(unsigned char buf[BUF_LEN])
     return 1;
 }
 
+static int test_PACKET_end(unsigned char buf[BUF_LEN])
+{
+    PACKET pkt;
+
+    if (       !PACKET_buf_init(&pkt, buf, BUF_LEN)
+            ||  PACKET_remaining(&pkt) != BUF_LEN
+            ||  PACKET_end(&pkt) != buf + BUF_LEN
+            || !PACKET_forward(&pkt, BUF_LEN - 1)
+            || PACKET_end(&pkt) != buf + BUF_LEN
+            || !PACKET_forward(&pkt, 1)
+            || PACKET_end(&pkt) != buf + BUF_LEN) {
+        fprintf(stderr, "test_PACKET_end() failed\n");
+        return 0;
+    }
+
+    return 1;
+}
+
 static int test_PACKET_get_1(unsigned char buf[BUF_LEN])
 {
     unsigned int i;
@@ -308,6 +326,26 @@ static int test_PACKET_strndup()
     return 1;
 }
 
+static int test_PACKET_contains_zero_byte()
+{
+    char buf[10], buf2[10];
+    PACKET pkt;
+
+    memset(buf, 'x', 10);
+    memset(buf2, 'y', 10);
+    buf2[5] = '\0';
+
+    if (       !PACKET_buf_init(&pkt, (unsigned char*)buf, 10)
+            ||  PACKET_contains_zero_byte(&pkt)
+            || !PACKET_buf_init(&pkt, (unsigned char*)buf2, 10)
+            || !PACKET_contains_zero_byte(&pkt)) {
+        fprintf(stderr, "test_PACKET_contains_zero_byte failed\n");
+        return 0;
+    }
+
+    return 1;
+}
+
 static int test_PACKET_forward(unsigned char buf[BUF_LEN])
 {
     const unsigned char *byte;
@@ -457,6 +495,57 @@ static int test_PACKET_get_length_prefixed_3()
     return 1;
 }
 
+static int test_PACKET_as_length_prefixed_1()
+{
+    unsigned char buf[BUF_LEN];
+    const size_t len = 16;
+    unsigned int i;
+    PACKET pkt, exact_pkt, subpkt;
+
+    buf[0] = len;
+    for (i = 1; i < BUF_LEN; i++) {
+        buf[i] = (i * 2) & 0xff;
+    }
+
+    if (       !PACKET_buf_init(&pkt, buf, BUF_LEN)
+            || !PACKET_buf_init(&exact_pkt, buf, len + 1)
+            ||  PACKET_as_length_prefixed_1(&pkt, &subpkt)
+            ||  PACKET_remaining(&pkt) != BUF_LEN
+            || !PACKET_as_length_prefixed_1(&exact_pkt, &subpkt)
+            ||  PACKET_remaining(&exact_pkt) != 0
+            ||  PACKET_remaining(&subpkt) != len) {
+        fprintf(stderr, "test_PACKET_as_length_prefixed_1() failed\n");
+        return 0;
+    }
+
+    return 1;
+}
+
+static int test_PACKET_as_length_prefixed_2()
+{
+    unsigned char buf[1024];
+    const size_t len = 516;  /* 0x0204 */
+    unsigned int i;
+    PACKET pkt, exact_pkt, subpkt;
+
+    for (i = 1; i <= 1024; i++) {
+        buf[i-1] = (i * 2) & 0xff;
+    }
+
+    if (       !PACKET_buf_init(&pkt, buf, 1024)
+            || !PACKET_buf_init(&exact_pkt, buf, len + 2)
+            ||  PACKET_as_length_prefixed_2(&pkt, &subpkt)
+            ||  PACKET_remaining(&pkt) != 1024
+            || !PACKET_as_length_prefixed_2(&exact_pkt, &subpkt)
+            ||  PACKET_remaining(&exact_pkt) != 0
+            ||  PACKET_remaining(&subpkt) != len) {
+        fprintf(stderr, "test_PACKET_as_length_prefixed_2() failed\n");
+        return 0;
+    }
+
+    return 1;
+}
+
 int main(int argc, char **argv)
 {
     unsigned char buf[BUF_LEN];
@@ -470,6 +559,7 @@ int main(int argc, char **argv)
     if (       !test_PACKET_buf_init()
             || !test_PACKET_null_init()
             || !test_PACKET_remaining(buf)
+            || !test_PACKET_end(buf)
             || !test_PACKET_equal(buf)
             || !test_PACKET_get_1(buf)
             || !test_PACKET_get_4(buf)
@@ -482,10 +572,13 @@ int main(int argc, char **argv)
             || !test_PACKET_copy_all(buf)
             || !test_PACKET_memdup(buf)
             || !test_PACKET_strndup()
+            || !test_PACKET_contains_zero_byte()
             || !test_PACKET_forward(buf)
             || !test_PACKET_get_length_prefixed_1()
             || !test_PACKET_get_length_prefixed_2()
-            || !test_PACKET_get_length_prefixed_3()) {
+            || !test_PACKET_get_length_prefixed_3()
+            || !test_PACKET_as_length_prefixed_1()
+            || !test_PACKET_as_length_prefixed_2()) {
         return 1;
     }
     printf("PASS\n");
index e0f2fc5513e1ea4f8381cd0c7d544ff0c33ee056..bcab4b5f786f3e3bae5d79414e65f686aedbb6e4 100644 (file)
@@ -606,20 +606,25 @@ sub testssl {
     subtest 'ALPN tests' => sub {
        ######################################################################
 
-       plan tests => 12;
+       plan tests => 14;
 
       SKIP: {
          skip "TLSv1.0 is not supported by this OpenSSL build", 12
              if $no_tls1;
 
-         ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "foo", "-alpn_server", "bar"])));
+         ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "foo"])));
+         ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_server", "foo"])));
          ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "foo", "-alpn_server", "foo", "-alpn_expected", "foo"])));
          ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "foo,bar", "-alpn_server", "foo", "-alpn_expected", "foo"])));
          ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "bar,foo", "-alpn_server", "foo", "-alpn_expected", "foo"])));
          ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "bar,foo", "-alpn_server", "foo,bar", "-alpn_expected", "foo"])));
          ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "bar,foo", "-alpn_server", "bar,foo", "-alpn_expected", "bar"])));
          ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "foo,bar", "-alpn_server", "bar,foo", "-alpn_expected", "bar"])));
-         ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "baz", "-alpn_server", "bar,foo"])));
+
+         is(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "foo", "-alpn_server", "bar"])), 0,
+             "Testing ALPN with protocol mismatch, expecting failure");
+         is(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "baz", "-alpn_server", "bar,foo"])), 0,
+             "Testing ALPN with protocol mismatch, expecting failure");
 
        SKIP: {
            skip "skipping SRP tests", 4