Complete the TLS extension rewrite on the client-side.

The RI logic gets pulled up into ssl3_get_server_hello() and
ssl_parse_serverhello_tlsext() gets replaced by tlsext_client_parse(),
which allows a CBS to be passed all the way down.

This also deduplicates the tlsext_client_build() and tlsext_server_build()
code.

ok beck@

4 files changed, 93 insertions, 156 deletions

diff --git a/src/lib/libssl/ssl_clnt.c b/src/lib/libssl/ssl_clnt.c
index 56ea99d82e..10dbe83cd5 100644
--- a/src/lib/libssl/ssl_clnt.c
+++ b/src/lib/libssl/ssl_clnt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_clnt.c,v 1.22 2017/10/12 16:06:32 jsing Exp $ */
+/* $OpenBSD: ssl_clnt.c,v 1.23 2018/02/08 11:30:30 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -813,7 +813,6 @@ ssl3_get_server_hello(SSL *s)
         STACK_OF(SSL_CIPHER) *sk;
         const SSL_CIPHER *cipher;
         const SSL_METHOD *method;
-        unsigned char *p;
         unsigned long alg_k;
         size_t outlen;
         int i, al, ok;
@@ -1011,22 +1010,31 @@ ssl3_get_server_hello(SSL *s)
                 goto f_err;
         }
 
-        /* TLS extensions. */
-        p = (unsigned char *)CBS_data(&cbs);
-        if (!ssl_parse_serverhello_tlsext(s, &p, CBS_len(&cbs), &al)) {
-                /* 'al' set by ssl_parse_serverhello_tlsext */
+        if (!tlsext_serverhello_parse(s, &cbs, &al)) {
                 SSLerror(s, SSL_R_PARSE_TLSEXT);
                 goto f_err;
         }
+
+        /*
+         * Determine if we need to see RI. Strictly speaking if we want to
+         * avoid an attack we should *always* see RI even on initial server
+         * hello because the client doesn't see any renegotiation during an
+         * attack. However this would mean we could not connect to any server
+         * which doesn't support RI so for the immediate future tolerate RI
+         * absence on initial connect only.
+         */
+        if (!S3I(s)->renegotiate_seen &&
+            !(s->internal->options & SSL_OP_LEGACY_SERVER_CONNECT)) {
+                al = SSL_AD_HANDSHAKE_FAILURE;
+                SSLerror(s, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED);
+                goto f_err;
+        }
+
         if (ssl_check_serverhello_tlsext(s) <= 0) {
                 SSLerror(s, SSL_R_SERVERHELLO_TLSEXT);
                 goto err;
         }
 
-        /* See if any data remains... */
-        if (p - CBS_data(&cbs) != CBS_len(&cbs))
-                goto truncated;
-
         return (1);
 
 truncated:
diff --git a/src/lib/libssl/ssl_tlsext.c b/src/lib/libssl/ssl_tlsext.c
index 0e3ef7da15..3735b719db 100644
--- a/src/lib/libssl/ssl_tlsext.c
+++ b/src/lib/libssl/ssl_tlsext.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.20 2018/01/27 15:30:05 jsing Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.21 2018/02/08 11:30:30 jsing Exp $ */
 /*
  * Copyright (c) 2016, 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -1310,19 +1310,34 @@ tls_extension_find(uint16_t type, size_t *tls_extensions_idx)
         return NULL;
 }
 
-static void
-tlsext_clienthello_reset_state(SSL *s)
+static int
+tls_extension_needs(struct tls_extension *tlsext, int is_serverhello, SSL *s)
 {
-        s->internal->servername_done = 0;
-        s->tlsext_status_type = -1;
-        S3I(s)->renegotiate_seen = 0;
-        free(S3I(s)->alpn_selected);
-        S3I(s)->alpn_selected = NULL;
-        s->internal->srtp_profile = NULL;
+        if (is_serverhello)
+                return tlsext->serverhello_needs(s);
+        return tlsext->clienthello_needs(s);
 }
 
-int
-tlsext_clienthello_build(SSL *s, CBB *cbb)
+static int
+tls_extension_build(struct tls_extension *tlsext, int is_serverhello, SSL *s,
+    CBB *cbb)
+{
+        if (is_serverhello)
+                return tlsext->serverhello_build(s, cbb);
+        return tlsext->clienthello_build(s, cbb);
+}
+
+static int
+tls_extension_parse(struct tls_extension *tlsext, int is_serverhello, SSL *s,
+    CBS *cbs, int *alert)
+{
+        if (is_serverhello)
+                return tlsext->serverhello_parse(s, cbs, alert);
+        return tlsext->clienthello_parse(s, cbs, alert);
+}
+
+static int
+tlsext_build(SSL *s, CBB *cbb, int is_serverhello)
 {
         CBB extensions, extension_data;
         struct tls_extension *tlsext;
@@ -1335,14 +1350,16 @@ tlsext_clienthello_build(SSL *s, CBB *cbb)
         for (i = 0; i < N_TLS_EXTENSIONS; i++) {
                 tlsext = &tls_extensions[i];
 
-                if (!tlsext->clienthello_needs(s))
+                if (!tls_extension_needs(tlsext, is_serverhello, s))
                         continue;
 
                 if (!CBB_add_u16(&extensions, tlsext->type))
                         return 0;
                 if (!CBB_add_u16_length_prefixed(&extensions, &extension_data))
                         return 0;
-                if (!tls_extensions[i].clienthello_build(s, &extension_data))
+
+                if (!tls_extension_build(tlsext, is_serverhello, s,
+                    &extension_data))
                         return 0;
 
                 extensions_present = 1;
@@ -1357,8 +1374,8 @@ tlsext_clienthello_build(SSL *s, CBB *cbb)
         return 1;
 }
 
-int
-tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert)
+static int
+tlsext_parse(SSL *s, CBS *cbs, int *alert, int is_serverhello)
 {
         CBS extensions, extension_data;
         struct tls_extension *tlsext;
@@ -1366,9 +1383,6 @@ tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert)
         uint16_t type;
         size_t idx;
 
-        /* XXX - this possibly should be done by the caller... */
-        tlsext_clienthello_reset_state(s);
-
         /* An empty extensions block is valid. */
         if (CBS_len(cbs) == 0)
                 return 1;
@@ -1385,7 +1399,7 @@ tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert)
                         return 0;
 
                 if (s->internal->tlsext_debug_cb != NULL)
-                        s->internal->tlsext_debug_cb(s, 0, type,
+                        s->internal->tlsext_debug_cb(s, is_serverhello, type,
                             (unsigned char *)CBS_data(&extension_data),
                             CBS_len(&extension_data),
                             s->internal->tlsext_debug_arg);
@@ -1394,12 +1408,13 @@ tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert)
                 if ((tlsext = tls_extension_find(type, &idx)) == NULL)
                         continue;
 
-                /* Check for duplicate extensions. */
+                /* Check for duplicate known extensions. */
                 if ((extensions_seen & (1 << idx)) != 0)
                         return 0;
                 extensions_seen |= (1 << idx);
 
-                if (!tlsext->clienthello_parse(s, &extension_data, alert))
+                if (!tls_extension_parse(tlsext, is_serverhello, s,
+                    &extension_data, alert))
                         return 0;
 
                 if (CBS_len(&extension_data) != 0)
@@ -1409,63 +1424,51 @@ tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert)
         return 1;
 }
 
-int
-tlsext_serverhello_build(SSL *s, CBB *cbb)
+static void
+tlsext_clienthello_reset_state(SSL *s)
 {
-        CBB extensions, extension_data;
-        struct tls_extension *tlsext;
-        int extensions_present = 0;
-        size_t i;
-
-        if (!CBB_add_u16_length_prefixed(cbb, &extensions))
-                return 0;
-
-        for (i = 0; i < N_TLS_EXTENSIONS; i++) {
-                tlsext = &tls_extensions[i];
-
-                if (!tlsext->serverhello_needs(s))
-                        continue;
-
-                if (!CBB_add_u16(&extensions, tlsext->type))
-                        return 0;
-                if (!CBB_add_u16_length_prefixed(&extensions, &extension_data))
-                        return 0;
-                if (!tlsext->serverhello_build(s, &extension_data))
-                        return 0;
+        s->internal->servername_done = 0;
+        s->tlsext_status_type = -1;
+        S3I(s)->renegotiate_seen = 0;
+        free(S3I(s)->alpn_selected);
+        S3I(s)->alpn_selected = NULL;
+        s->internal->srtp_profile = NULL;
+}
 
-                extensions_present = 1;
-        }
+int
+tlsext_clienthello_build(SSL *s, CBB *cbb)
+{
+        return tlsext_build(s, cbb, 0);
+}
 
-        if (!extensions_present)
-                CBB_discard_child(cbb);
+int
+tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert)
+{
+        /* XXX - this possibly should be done by the caller... */
+        tlsext_clienthello_reset_state(s);
 
-        if (!CBB_flush(cbb))
-                return 0;
+        return tlsext_parse(s, cbs, alert, 0);
+}
 
-        return 1;
+static void
+tlsext_serverhello_reset_state(SSL *s)
+{
+        S3I(s)->renegotiate_seen = 0;   
+        free(S3I(s)->alpn_selected);
+        S3I(s)->alpn_selected = NULL;
 }
 
 int
-tlsext_serverhello_parse_one(SSL *s, CBS *cbs, uint16_t type, int *alert)
+tlsext_serverhello_build(SSL *s, CBB *cbb)
 {
-        struct tls_extension *tlsext;
-        size_t i;
-
-        for (i = 0; i < N_TLS_EXTENSIONS; i++) {
-                tlsext = &tls_extensions[i];
-
-                if (tlsext->type != type)
-                        continue;
-                if (!tlsext->serverhello_parse(s, cbs, alert))
-                        return 0;
-                if (CBS_len(cbs) != 0) {
-                        *alert = SSL_AD_DECODE_ERROR;
-                        return 0;
-                }
+        return tlsext_build(s, cbb, 1);
+}
 
-                return 1;
-        }
+int
+tlsext_serverhello_parse(SSL *s, CBS *cbs, int *alert)
+{
+        /* XXX - this possibly should be done by the caller... */
+        tlsext_serverhello_reset_state(s);
 
-        /* Not found. */
-        return 2;
+        return tlsext_parse(s, cbs, alert, 1);
 }
diff --git a/src/lib/libssl/ssl_tlsext.h b/src/lib/libssl/ssl_tlsext.h
index 1af2e6cb3b..4248932fb2 100644
--- a/src/lib/libssl/ssl_tlsext.h
+++ b/src/lib/libssl/ssl_tlsext.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.h,v 1.11 2018/01/27 15:30:05 jsing Exp $ */
+/* $OpenBSD: ssl_tlsext.h,v 1.12 2018/02/08 11:30:30 jsing Exp $ */
 /*
  * Copyright (c) 2016, 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -85,5 +85,4 @@ int tlsext_clienthello_build(SSL *s, CBB *cbb);
 int tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert);
 
 int tlsext_serverhello_build(SSL *s, CBB *cbb);
-int tlsext_serverhello_parse_one(SSL *s, CBS *cbs, uint16_t tlsext_type,
-    int *alert);
+int tlsext_serverhello_parse(SSL *s, CBS *cbs, int *alert);
diff --git a/src/lib/libssl/t1_lib.c b/src/lib/libssl/t1_lib.c
index fbd79431db..d92fd70f5b 100644
--- a/src/lib/libssl/t1_lib.c
+++ b/src/lib/libssl/t1_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: t1_lib.c,v 1.140 2018/01/27 15:30:05 jsing Exp $ */
+/* $OpenBSD: t1_lib.c,v 1.141 2018/02/08 11:30:30 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -662,79 +662,6 @@ tls12_get_req_sig_algs(SSL *s, unsigned char **sigalgs, size_t *sigalgs_len)
 }
 
 int
-ssl_parse_serverhello_tlsext(SSL *s, unsigned char **p, size_t n, int *al)
-{
-        unsigned short type;
-        unsigned short size;
-        unsigned short len;
-        unsigned char *data = *p;
-        unsigned char *end = *p + n;
-        CBS cbs;
-
-        S3I(s)->renegotiate_seen = 0;
-        free(S3I(s)->alpn_selected);
-        S3I(s)->alpn_selected = NULL;
-
-        if (data == end)
-                goto ri_check;
-
-        if (end - data < 2)
-                goto err;
-        n2s(data, len);
-
-        if (end - data != len)
-                goto err;
-
-        while (end - data >= 4) {
-                n2s(data, type);
-                n2s(data, size);
-
-                if (end - data < size)
-                        goto err;
-
-                if (s->internal->tlsext_debug_cb)
-                        s->internal->tlsext_debug_cb(s, 1, type, data, size,
-                            s->internal->tlsext_debug_arg);
-
-                CBS_init(&cbs, data, size);
-                if (!tlsext_serverhello_parse_one(s, &cbs, type, al))
-                        return 0;
-
-                data += size;
-
-        }
-
-        if (data != end) {
-                *al = SSL_AD_DECODE_ERROR;
-                return 0;
-        }
-
-        *p = data;
-
-ri_check:
-
-        /* Determine if we need to see RI. Strictly speaking if we want to
-         * avoid an attack we should *always* see RI even on initial server
-         * hello because the client doesn't see any renegotiation during an
-         * attack. However this would mean we could not connect to any server
-         * which doesn't support RI so for the immediate future tolerate RI
-         * absence on initial connect only.
-         */
-        if (!S3I(s)->renegotiate_seen &&
-            !(s->internal->options & SSL_OP_LEGACY_SERVER_CONNECT)) {
-                *al = SSL_AD_HANDSHAKE_FAILURE;
-                SSLerror(s, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED);
-                return 0;
-        }
-
-        return 1;
-
-err:
-        *al = SSL_AD_DECODE_ERROR;
-        return 0;
-}
-
-int
 ssl_check_clienthello_tlsext_early(SSL *s)
 {
         int ret = SSL_TLSEXT_ERR_NOACK;