From: jsing Date: Fri, 24 Aug 2018 18:10:25 +0000 (+0000) Subject: Simplify session ticket parsing/handling. X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=307759ee4c8d98d4647f86f8ff8b8f7b7fbf3a68;p=openbsd 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 e5423859afc..44afd1717ec 100644 --- 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 8ebeb273feb..52a1a0cc47c 100644 --- 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, ©))) { + session_id, session_id_len, ©))) { s->session_ctx->internal->stats.sess_cb_hit++; /* diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c index b9b2c58705b..f06491e558b 100644 --- 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 1b2e0844fb0..0a00e4da7fb 100644 --- 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 */