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@

diff --git a/lib/libssl/ssl_asn1.c b/lib/libssl/ssl_asn1.c
index 0ca442f..94fc868 100644 (file)
--- a/lib/libssl/ssl_asn1.c
+++ b/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/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h
index 44afd17..39aabb0 100644 (file)
--- a/lib/libssl/ssl_locl.h
+++ b/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/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c
index f06491e..b099fdb 100644 (file)
--- a/lib/libssl/ssl_srvr.c
+++ b/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);
 }