From 84fe391fb9021a6be2b592ffb9543ccc421a80be Mon Sep 17 00:00:00 2001
From: jsing <>
Date: Fri, 24 Aug 2018 18:10:25 +0000
Subject: 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@
---
src/lib/libssl/ssl_locl.h | 13 +++++-----
src/lib/libssl/ssl_sess.c | 22 ++++++++--------
src/lib/libssl/ssl_srvr.c | 15 +++++------
src/lib/libssl/t1_lib.c | 65 +++++++++++++++--------------------------------
4 files changed, 46 insertions(+), 69 deletions(-)
(limited to 'src')
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, ©))) {
+ session_id, session_id_len, ©))) {
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 */
--
cgit v1.2.3-55-g6feb