Simplify session ticket parsing/handling.

The original implementation is rather crazy and means that we effectively
have two lots of code that parse a ClientHello and two lots of code that
parse TLS extensions. Partially simplify this by passing a CBS containing
the extension block through to the session handling functions, removing the
need to reimplement the ClientHello parsing.

While here standarise on naming for session_id and session_id_len.

ok inoguchi@ tb@

4 files changed, 46 insertions, 69 deletions

diff --git a/src/lib/libssl/ssl_locl.h b/src/lib/libssl/ssl_locl.h
index e5423859af..44afd1717e 100644
--- a/src/lib/libssl/ssl_locl.h
+++ b/src/lib/libssl/ssl_locl.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_locl.h,v 1.208 2018/08/24 17:30:32 jsing Exp $ */
+/* $OpenBSD: ssl_locl.h,v 1.209 2018/08/24 18:10:25 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1056,10 +1056,11 @@ void ssl_cert_free(CERT *c);
 SESS_CERT *ssl_sess_cert_new(void);
 void ssl_sess_cert_free(SESS_CERT *sc);
 int ssl_get_new_session(SSL *s, int session);
-int ssl_get_prev_session(SSL *s, unsigned char *session, int len,
-    const unsigned char *limit);
+int ssl_get_prev_session(SSL *s, const unsigned char *session_id,
+    int session_id_len, CBS *ext_block);
 int ssl_cipher_id_cmp(const SSL_CIPHER *a, const SSL_CIPHER *b);
-SSL_CIPHER *OBJ_bsearch_ssl_cipher_id(SSL_CIPHER *key, SSL_CIPHER const *base, int num);
+SSL_CIPHER *OBJ_bsearch_ssl_cipher_id(SSL_CIPHER *key, SSL_CIPHER const *base,
+    int num);
 int ssl_cipher_ptr_id_cmp(const SSL_CIPHER * const *ap,
     const SSL_CIPHER * const *bp);
 int ssl_cipher_list_to_bytes(SSL *s, STACK_OF(SSL_CIPHER) *ciphers, CBB *cbb);
@@ -1278,8 +1279,8 @@ int ssl_check_clienthello_tlsext_late(SSL *s);
 int ssl_check_serverhello_tlsext(SSL *s);
 
 #define tlsext_tick_md  EVP_sha256
-int tls1_process_ticket(SSL *s, const unsigned char *session_id, int len,
-    const unsigned char *limit, SSL_SESSION **ret);
+int tls1_process_ticket(SSL *s, const unsigned char *session_id,
+    int session_id_len, CBS *ext_block, SSL_SESSION **ret);
 int tls12_get_hashid(const EVP_MD *md);
 int tls12_get_sigid(const EVP_PKEY *pk);
 int tls12_get_hashandsig(CBB *cbb, const EVP_PKEY *pk, const EVP_MD *md);
diff --git a/src/lib/libssl/ssl_sess.c b/src/lib/libssl/ssl_sess.c
index 8ebeb273fe..52a1a0cc47 100644
--- a/src/lib/libssl/ssl_sess.c
+++ b/src/lib/libssl/ssl_sess.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_sess.c,v 1.80 2018/04/25 07:10:39 tb Exp $ */
+/* $OpenBSD: ssl_sess.c,v 1.81 2018/08/24 18:10:25 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -420,8 +420,8 @@ sess_id_done:
  *   session_id: points at the session ID in the ClientHello. This code will
  *       read past the end of this in order to parse out the session ticket
  *       extension, if any.
- *   len: the length of the session ID.
- *   limit: a pointer to the first byte after the ClientHello.
+ *   session_id_len: the length of the session ID.
+ *   ext_block: a CBS for the ClientHello extensions block.
  *
  * Returns:
  *   -1: error
@@ -435,8 +435,8 @@ sess_id_done:
  *     to 1 if the server should issue a new session ticket (to 0 otherwise).
  */
 int
-ssl_get_prev_session(SSL *s, unsigned char *session_id, int len,
-    const unsigned char *limit)
+ssl_get_prev_session(SSL *s, const unsigned char *session_id,
+    int session_id_len, CBS *ext_block)
 {
         SSL_SESSION *ret = NULL;
         int fatal = 0;
@@ -445,14 +445,14 @@ ssl_get_prev_session(SSL *s, unsigned char *session_id, int len,
 
         /* This is used only by servers. */
 
-        if (len > SSL_MAX_SSL_SESSION_ID_LENGTH)
+        if (session_id_len > SSL_MAX_SSL_SESSION_ID_LENGTH)
                 goto err;
 
-        if (len == 0)
+        if (session_id_len == 0)
                 try_session_cache = 0;
 
         /* Sets s->internal->tlsext_ticket_expected. */
-        r = tls1_process_ticket(s, session_id, len, limit, &ret);
+        r = tls1_process_ticket(s, session_id, session_id_len, ext_block, &ret);
         switch (r) {
         case -1: /* Error during processing */
                 fatal = 1;
@@ -473,8 +473,8 @@ ssl_get_prev_session(SSL *s, unsigned char *session_id, int len,
              SSL_SESS_CACHE_NO_INTERNAL_LOOKUP)) {
                 SSL_SESSION data;
                 data.ssl_version = s->version;
-                data.session_id_length = len;
-                memcpy(data.session_id, session_id, len);
+                data.session_id_length = session_id_len;
+                memcpy(data.session_id, session_id, session_id_len);
 
                 CRYPTO_r_lock(CRYPTO_LOCK_SSL_CTX);
                 ret = lh_SSL_SESSION_retrieve(s->session_ctx->internal->sessions, &data);
@@ -494,7 +494,7 @@ ssl_get_prev_session(SSL *s, unsigned char *session_id, int len,
                 int copy = 1;
 
                 if ((ret = s->session_ctx->internal->get_session_cb(s,
-                    session_id, len, &copy))) {
+                    session_id, session_id_len, &copy))) {
                         s->session_ctx->internal->stats.sess_cb_hit++;
 
                         /*
diff --git a/src/lib/libssl/ssl_srvr.c b/src/lib/libssl/ssl_srvr.c
index b9b2c58705..f06491e558 100644
--- a/src/lib/libssl/ssl_srvr.c
+++ b/src/lib/libssl/ssl_srvr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_srvr.c,v 1.44 2018/08/24 17:44:22 jsing Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.45 2018/08/24 18:10:25 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -818,7 +818,6 @@ ssl3_get_client_hello(SSL *s)
         unsigned long alg_k;
         const SSL_METHOD *method;
         uint16_t shared_version;
-        unsigned char *end;
 
         /*
          * We do this so that we will respond with our native type.
@@ -842,8 +841,6 @@ ssl3_get_client_hello(SSL *s)
         if (n < 0)
                 goto err;
 
-        end = (unsigned char *)s->internal->init_msg + n;
-
         CBS_init(&cbs, s->internal->init_msg, n);
 
         /* Parse client hello up until the extensions (if any). */
@@ -928,10 +925,12 @@ ssl3_get_client_hello(SSL *s)
                 if (!ssl_get_new_session(s, 1))
                         goto err;
         } else {
-                /* XXX - pass CBS through instead... */
-                i = ssl_get_prev_session(s,
-                    (unsigned char *)CBS_data(&session_id),
-                    CBS_len(&session_id), end);
+                CBS ext_block;
+
+                CBS_dup(&cbs, &ext_block);
+
+                i = ssl_get_prev_session(s, CBS_data(&session_id),
+                    CBS_len(&session_id), &ext_block);
                 if (i == 1) { /* previous session */
                         s->internal->hit = 1;
                 } else if (i == -1)
diff --git a/src/lib/libssl/t1_lib.c b/src/lib/libssl/t1_lib.c
index 1b2e0844fb..0a00e4da7f 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.143 2018/08/19 15:38:03 jsing Exp $ */
+/* $OpenBSD: t1_lib.c,v 1.144 2018/08/24 18:10:25 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -815,11 +815,9 @@ ssl_check_serverhello_tlsext(SSL *s)
  * ClientHello, and other operations depend on the result, we need to handle
  * any TLS session ticket extension at the same time.
  *
- *   session_id: points at the session ID in the ClientHello. This code will
- *       read past the end of this in order to parse out the session ticket
- *       extension, if any.
- *   len: the length of the session ID.
- *   limit: a pointer to the first byte after the ClientHello.
+ *   session_id: points at the session ID in the ClientHello.
+ *   session_id_len: the length of the session ID.
+ *   ext_block: a CBS for the ClientHello extensions block.
  *   ret: (output) on return, if a ticket was decrypted, then this is set to
  *       point to the resulting session.
  *
@@ -845,55 +843,34 @@ ssl_check_serverhello_tlsext(SSL *s)
  *   Otherwise, s->internal->tlsext_ticket_expected is set to 0.
  */
 int
-tls1_process_ticket(SSL *s, const unsigned char *session, int session_len,
-    const unsigned char *limit, SSL_SESSION **ret)
+tls1_process_ticket(SSL *s, const unsigned char *session_id, int session_id_len,
+    CBS *ext_block, SSL_SESSION **ret)
 {
-        /* Point after session ID in client hello */
-        CBS session_id, cookie, cipher_list, compress_algo, extensions;
+        CBS extensions;
 
-        *ret = NULL;
         s->internal->tlsext_ticket_expected = 0;
+        *ret = NULL;
 
-        /* If tickets disabled behave as if no ticket present
-         * to permit stateful resumption.
+        /*
+         * If tickets disabled behave as if no ticket present to permit stateful
+         * resumption.
          */
         if (SSL_get_options(s) & SSL_OP_NO_TICKET)
                 return 0;
-        if (!limit)
-                return 0;
-
-        if (limit < session)
-                return -1;
 
-        CBS_init(&session_id, session, limit - session);
-
-        /* Skip past the session id */
-        if (!CBS_skip(&session_id, session_len))
-                return -1;
-
-        /* Skip past DTLS cookie */
-        if (SSL_IS_DTLS(s)) {
-                if (!CBS_get_u8_length_prefixed(&session_id, &cookie))
-                        return -1;
-        }
-
-        /* Skip past cipher list */
-        if (!CBS_get_u16_length_prefixed(&session_id, &cipher_list))
-                return -1;
-
-        /* Skip past compression algorithm list */
-        if (!CBS_get_u8_length_prefixed(&session_id, &compress_algo))
-                return -1;
-
-        /* Now at start of extensions */
-        if (CBS_len(&session_id) == 0)
+        /*
+         * An empty extensions block is valid, but obviously does not contain
+         * a session ticket.
+         */
+        if (CBS_len(ext_block) == 0)
                 return 0;
-        if (!CBS_get_u16_length_prefixed(&session_id, &extensions))
+
+        if (!CBS_get_u16_length_prefixed(ext_block, &extensions))
                 return -1;
 
         while (CBS_len(&extensions) > 0) {
-                CBS ext_data;
                 uint16_t ext_type;
+                CBS ext_data;
 
                 if (!CBS_get_u16(&extensions, &ext_type) ||
                     !CBS_get_u16_length_prefixed(&extensions, &ext_data))
@@ -907,7 +884,7 @@ tls1_process_ticket(SSL *s, const unsigned char *session, int session_len,
                                 s->internal->tlsext_ticket_expected = 1;
                                 return 1;
                         }
-                        if (s->internal->tls_session_secret_cb) {
+                        if (s->internal->tls_session_secret_cb != NULL) {
                                 /* Indicate that the ticket couldn't be
                                  * decrypted rather than generating the session
                                  * from ticket now, trigger abbreviated
@@ -917,7 +894,7 @@ tls1_process_ticket(SSL *s, const unsigned char *session, int session_len,
                         }
 
                         r = tls_decrypt_ticket(s, CBS_data(&ext_data),
-                            CBS_len(&ext_data), session, session_len, ret);
+                            CBS_len(&ext_data), session_id, session_id_len, ret);
 
                         switch (r) {
                         case 2: /* ticket couldn't be decrypted */