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: NKurt Roeckx <kurt@openssl.org>

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

include/openssl/ssl.h

@@ -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

include/openssl/tls1.h

@@ -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

ssl/packet_locl.h

@@ -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|.

ssl/s3_enc.c

@@ -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);
     }

ssl/t1_enc.c

@@ -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);
     }

test/packettest.c

@@ -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");

test/recipes/80-test_ssl.t

@@ -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