Simplify new session ticket encoding/generation.

The original code did a crazy encode/malloc/encode/decode/modify/encode
dance, in order to encode a session in the form needed to encrypt then add
to a session ticket. By modifying the encoding functions slightly, we can
do this entire dance as a single encode.

Inspired by similar changes in BoringSSL.

ok inoguchi@ tb@

3 files changed, 90 insertions, 84 deletions

diff --git a/src/lib/libssl/ssl_asn1.c b/src/lib/libssl/ssl_asn1.c
index 0ca442faa0..94fc8685fc 100644
--- a/src/lib/libssl/ssl_asn1.c
+++ b/src/lib/libssl/ssl_asn1.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_asn1.c,v 1.56 2018/03/20 16:10:57 jsing Exp $ */
+/* $OpenBSD: ssl_asn1.c,v 1.57 2018/08/27 16:42:48 jsing Exp $ */
 /*
  * Copyright (c) 2016 Joel Sing <jsing@openbsd.org>
  *
@@ -44,23 +44,16 @@ time_max(void)
         return 0;
 }
 
-int
-i2d_SSL_SESSION(SSL_SESSION *s, unsigned char **pp)
+static int
+SSL_SESSION_encode(SSL_SESSION *s, unsigned char **out, size_t *out_len,
+    int ticket_encoding)
 {
         CBB cbb, session, cipher_suite, session_id, master_key, time, timeout;
-        CBB peer_cert, sidctx, verify_result, hostname, lifetime, ticket;
-        CBB value;
-        unsigned char *data = NULL, *peer_cert_bytes = NULL;
-        size_t data_len = 0;
-        int len, rv = -1;
+        CBB peer_cert, sidctx, verify_result, hostname, lifetime, ticket, value;
+        unsigned char *peer_cert_bytes = NULL;
+        int len, rv = 0;
         uint16_t cid;
 
-        if (s == NULL)
-                return (0);
-
-        if (s->cipher == NULL && s->cipher_id == 0)
-                return (0);
-
         if (!CBB_init(&cbb, 0))
                 goto err;
 
@@ -87,10 +80,11 @@ i2d_SSL_SESSION(SSL_SESSION *s, unsigned char **pp)
         if (!CBB_add_u16(&cipher_suite, cid))
                 goto err;
 
-        /* Session ID. */
+        /* Session ID - zero length for a ticket. */
         if (!CBB_add_asn1(&session, &session_id, CBS_ASN1_OCTETSTRING))
                 goto err;
-        if (!CBB_add_bytes(&session_id, s->session_id, s->session_id_length))
+        if (!CBB_add_bytes(&session_id, s->session_id,
+            ticket_encoding ? 0 : s->session_id_length))
                 goto err;
 
         /* Master key. */
@@ -173,7 +167,7 @@ i2d_SSL_SESSION(SSL_SESSION *s, unsigned char **pp)
         }
 
         /* Ticket [10]. */
-        if (s->tlsext_tick) {
+        if (s->tlsext_tick != NULL) {
                 if (!CBB_add_asn1(&session, &ticket, SSLASN1_TICKET_TAG))
                         goto err;
                 if (!CBB_add_asn1(&ticket, &value, CBS_ASN1_OCTETSTRING))
@@ -185,7 +179,44 @@ i2d_SSL_SESSION(SSL_SESSION *s, unsigned char **pp)
         /* Compression method [11]. */
         /* SRP username [12]. */
 
-        if (!CBB_finish(&cbb, &data, &data_len))
+        if (!CBB_finish(&cbb, out, out_len))
+                goto err;
+
+        rv = 1;
+
+ err:
+        CBB_cleanup(&cbb);
+        free(peer_cert_bytes);
+
+        return rv;
+}
+
+int
+SSL_SESSION_ticket(SSL_SESSION *ss, unsigned char **out, size_t *out_len)
+{
+        if (ss == NULL)
+                return 0;
+
+        if (ss->cipher == NULL && ss->cipher_id == 0)
+                return 0;
+
+        return SSL_SESSION_encode(ss, out, out_len, 1);
+}
+
+int
+i2d_SSL_SESSION(SSL_SESSION *ss, unsigned char **pp)
+{
+        unsigned char *data = NULL;
+        size_t data_len = 0;
+        int rv = -1;
+
+        if (ss == NULL)
+                return 0;
+
+        if (ss->cipher == NULL && ss->cipher_id == 0)
+                return 0;
+
+        if (!SSL_SESSION_encode(ss, &data, &data_len, 0))
                 goto err;
 
         if (data_len > INT_MAX)
@@ -204,9 +235,7 @@ i2d_SSL_SESSION(SSL_SESSION *s, unsigned char **pp)
         rv = (int)data_len;
 
  err:
-        CBB_cleanup(&cbb);
         freezero(data, data_len);
-        free(peer_cert_bytes);
 
         return rv;
 }
diff --git a/src/lib/libssl/ssl_locl.h b/src/lib/libssl/ssl_locl.h
index 44afd1717e..39aabb05fb 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.209 2018/08/24 18:10:25 jsing Exp $ */
+/* $OpenBSD: ssl_locl.h,v 1.210 2018/08/27 16:42:48 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1089,6 +1089,8 @@ int ssl_has_ecc_ciphers(SSL *s);
 int ssl_verify_alarm_type(long type);
 void ssl_load_ciphers(void);
 
+int SSL_SESSION_ticket(SSL_SESSION *ss, unsigned char **out, size_t *out_len);
+
 const SSL_CIPHER *ssl3_get_cipher_by_char(const unsigned char *p);
 int ssl3_put_cipher_by_char(const SSL_CIPHER *c, unsigned char *p);
 int ssl3_send_server_certificate(SSL *s);
diff --git a/src/lib/libssl/ssl_srvr.c b/src/lib/libssl/ssl_srvr.c
index f06491e558..b099fdb8b1 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.45 2018/08/24 18:10:25 jsing Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.46 2018/08/27 16:42:48 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -2526,20 +2526,21 @@ int
 ssl3_send_newsession_ticket(SSL *s)
 {
         CBB cbb, session_ticket, ticket;
-        unsigned char *enc_ticket = NULL;
-        unsigned char *senc = NULL;
-        const unsigned char *const_p;
-        unsigned char *p, *hmac;
-        size_t hmac_len;
-        int enc_ticket_len, len, slen;
-        int slen_full = 0;
-        SSL_SESSION *sess;
-        unsigned int hlen;
-        EVP_CIPHER_CTX ctx;
-        HMAC_CTX hctx;
         SSL_CTX *tctx = s->initial_ctx;
+        size_t enc_session_len, enc_session_max_len, hmac_len;
+        size_t session_len = 0;
+        unsigned char *enc_session = NULL, *session = NULL;
         unsigned char iv[EVP_MAX_IV_LENGTH];
         unsigned char key_name[16];
+        unsigned char *hmac;
+        unsigned int hlen;
+        EVP_CIPHER_CTX ctx;
+        HMAC_CTX hctx;
+        int len;
+
+        /*
+         * New Session Ticket - RFC 5077, section 3.3.
+         */
 
         EVP_CIPHER_CTX_init(&ctx);
         HMAC_CTX_init(&hctx);
@@ -2551,47 +2552,17 @@ ssl3_send_newsession_ticket(SSL *s)
                     SSL3_MT_NEWSESSION_TICKET))
                         goto err;
 
-                /* get session encoding length */
-                slen_full = i2d_SSL_SESSION(s->session, NULL);
-                /*
-                 * Some length values are 16 bits, so forget it if session is
-                 * too long
-                 */
-                if (slen_full > 0xFF00)
+                if (!SSL_SESSION_ticket(s->session, &session, &session_len))
                         goto err;
-                senc = malloc(slen_full);
-                if (!senc)
+                if (session_len > 0xffff)
                         goto err;
-                p = senc;
-                i2d_SSL_SESSION(s->session, &p);
 
                 /*
-                 * Create a fresh copy (not shared with other threads) to
-                 * clean up
+                 * Initialize HMAC and cipher contexts. If callback is present
+                 * it does all the work, otherwise use generated values from
+                 * parent context.
                  */
-                const_p = senc;
-                sess = d2i_SSL_SESSION(NULL, &const_p, slen_full);
-                if (sess == NULL)
-                        goto err;
-
-                /* ID is irrelevant for the ticket */
-                sess->session_id_length = 0;
-
-                slen = i2d_SSL_SESSION(sess, NULL);
-                if (slen > slen_full) {
-                        /* shouldn't ever happen */
-                        goto err;
-                }
-                p = senc;
-                i2d_SSL_SESSION(sess, &p);
-                SSL_SESSION_free(sess);
-
-                /*
-                 * Initialize HMAC and cipher contexts. If callback present
-                 * it does all the work otherwise use generated values
-                 * from parent ctx.
-                 */
-                if (tctx->internal->tlsext_ticket_key_cb) {
+                if (tctx->internal->tlsext_ticket_key_cb != NULL) {
                         if (tctx->internal->tlsext_ticket_key_cb(s,
                             key_name, iv, &ctx, &hctx, 1) < 0) {
                                 EVP_CIPHER_CTX_cleanup(&ctx);
@@ -2606,19 +2577,21 @@ ssl3_send_newsession_ticket(SSL *s)
                         memcpy(key_name, tctx->internal->tlsext_tick_key_name, 16);
                 }
 
-                /* Encrypt the session ticket. */
-                if ((enc_ticket = calloc(1, slen + EVP_MAX_BLOCK_LENGTH)) == NULL)
+                /* Encrypt the session state. */
+                enc_session_max_len = session_len + EVP_MAX_BLOCK_LENGTH;
+                if ((enc_session = calloc(1, enc_session_max_len)) == NULL)
                         goto err;
-                enc_ticket_len = 0;
-                if (!EVP_EncryptUpdate(&ctx, enc_ticket, &len, senc, slen))
+                enc_session_len = 0;
+                if (!EVP_EncryptUpdate(&ctx, enc_session, &len, session,
+                    session_len))
                         goto err;
-                enc_ticket_len += len;
-                if (!EVP_EncryptFinal_ex(&ctx, enc_ticket + enc_ticket_len, &len))
+                enc_session_len += len;
+                if (!EVP_EncryptFinal_ex(&ctx, enc_session + enc_session_len,
+                    &len))
                         goto err;
-                enc_ticket_len += len;
+                enc_session_len += len;
 
-                if (enc_ticket_len < 0 ||
-                    enc_ticket_len > slen + EVP_MAX_BLOCK_LENGTH)
+                if (enc_session_len > enc_session_max_len)
                         goto err;
 
                 /* Generate the HMAC. */
@@ -2626,7 +2599,7 @@ ssl3_send_newsession_ticket(SSL *s)
                         goto err;
                 if (!HMAC_Update(&hctx, iv, EVP_CIPHER_CTX_iv_length(&ctx)))
                         goto err;
-                if (!HMAC_Update(&hctx, enc_ticket, enc_ticket_len))
+                if (!HMAC_Update(&hctx, enc_session, enc_session_len))
                         goto err;
 
                 if ((hmac_len = HMAC_size(&hctx)) <= 0)
@@ -2648,13 +2621,15 @@ ssl3_send_newsession_ticket(SSL *s)
                         goto err;
                 if (!CBB_add_bytes(&ticket, iv, EVP_CIPHER_CTX_iv_length(&ctx)))
                         goto err;
-                if (!CBB_add_bytes(&ticket, enc_ticket, enc_ticket_len))
+                if (!CBB_add_bytes(&ticket, enc_session, enc_session_len))
                         goto err;
                 if (!CBB_add_space(&ticket, &hmac, hmac_len))
                         goto err;
 
                 if (!HMAC_Final(&hctx, hmac, &hlen))
                         goto err;
+                if (hlen != hmac_len)
+                        goto err;
 
                 if (!ssl3_handshake_msg_finish(s, &cbb))
                         goto err;
@@ -2664,8 +2639,8 @@ ssl3_send_newsession_ticket(SSL *s)
 
         EVP_CIPHER_CTX_cleanup(&ctx);
         HMAC_CTX_cleanup(&hctx);
-        freezero(senc, slen_full);
-        free(enc_ticket);
+        freezero(session, session_len);
+        free(enc_session);
 
         /* SSL3_ST_SW_SESSION_TICKET_B */
         return (ssl3_handshake_write(s));
@@ -2674,8 +2649,8 @@ ssl3_send_newsession_ticket(SSL *s)
         CBB_cleanup(&cbb);
         EVP_CIPHER_CTX_cleanup(&ctx);
         HMAC_CTX_cleanup(&hctx);
-        freezero(senc, slen_full);
-        free(enc_ticket);
+        freezero(session, session_len);
+        free(enc_session);
 
         return (-1);
 }