From 12fe7be032d9773f215e29e6d37815f9da949ef4 Mon Sep 17 00:00:00 2001 From: jsing Date: Mon, 27 Aug 2018 16:42:48 +0000 Subject: [PATCH] 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@ --- lib/libssl/ssl_asn1.c | 69 ++++++++++++++++++++--------- lib/libssl/ssl_locl.h | 4 +- lib/libssl/ssl_srvr.c | 101 ++++++++++++++++-------------------------- 3 files changed, 90 insertions(+), 84 deletions(-) diff --git a/lib/libssl/ssl_asn1.c b/lib/libssl/ssl_asn1.c index 0ca442faa0c..94fc8685fc9 100644 --- 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 * @@ -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 44afd1717ec..39aabb05fb2 100644 --- 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 f06491e558b..b099fdb8b18 100644 --- 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); } -- 2.20.1