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@

diff --git a/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h
index e542385..44afd17 100644 (file)
--- a/lib/libssl/ssl_locl.h
+++ b/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/lib/libssl/ssl_sess.c b/lib/libssl/ssl_sess.c
index 8ebeb27..52a1a0c 100644 (file)
--- a/lib/libssl/ssl_sess.c
+++ b/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/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c
index b9b2c58..f06491e 100644 (file)
--- a/lib/libssl/ssl_srvr.c
+++ b/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/lib/libssl/t1_lib.c b/lib/libssl/t1_lib.c
index 1b2e084..0a00e4d 100644 (file)
--- a/lib/libssl/t1_lib.c
+++ b/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 */