From 229ae49ead0f79b4527f210ea8576c4bd87866e0 Mon Sep 17 00:00:00 2001 From: jsing <> Date: Thu, 8 Feb 2018 11:30:30 +0000 Subject: Complete the TLS extension rewrite on the client-side. The RI logic gets pulled up into ssl3_get_server_hello() and ssl_parse_serverhello_tlsext() gets replaced by tlsext_client_parse(), which allows a CBS to be passed all the way down. This also deduplicates the tlsext_client_build() and tlsext_server_build() code. ok beck@ --- src/lib/libssl/ssl_clnt.c | 28 +++++---- src/lib/libssl/ssl_tlsext.c | 141 ++++++++++++++++++++++---------------------- src/lib/libssl/ssl_tlsext.h | 5 +- src/lib/libssl/t1_lib.c | 75 +---------------------- 4 files changed, 93 insertions(+), 156 deletions(-) (limited to 'src') diff --git a/src/lib/libssl/ssl_clnt.c b/src/lib/libssl/ssl_clnt.c index 56ea99d82e..10dbe83cd5 100644 --- a/src/lib/libssl/ssl_clnt.c +++ b/src/lib/libssl/ssl_clnt.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_clnt.c,v 1.22 2017/10/12 16:06:32 jsing Exp $ */ +/* $OpenBSD: ssl_clnt.c,v 1.23 2018/02/08 11:30:30 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -813,7 +813,6 @@ ssl3_get_server_hello(SSL *s) STACK_OF(SSL_CIPHER) *sk; const SSL_CIPHER *cipher; const SSL_METHOD *method; - unsigned char *p; unsigned long alg_k; size_t outlen; int i, al, ok; @@ -1011,22 +1010,31 @@ ssl3_get_server_hello(SSL *s) goto f_err; } - /* TLS extensions. */ - p = (unsigned char *)CBS_data(&cbs); - if (!ssl_parse_serverhello_tlsext(s, &p, CBS_len(&cbs), &al)) { - /* 'al' set by ssl_parse_serverhello_tlsext */ + if (!tlsext_serverhello_parse(s, &cbs, &al)) { SSLerror(s, SSL_R_PARSE_TLSEXT); goto f_err; } + + /* + * Determine if we need to see RI. Strictly speaking if we want to + * avoid an attack we should *always* see RI even on initial server + * hello because the client doesn't see any renegotiation during an + * attack. However this would mean we could not connect to any server + * which doesn't support RI so for the immediate future tolerate RI + * absence on initial connect only. + */ + if (!S3I(s)->renegotiate_seen && + !(s->internal->options & SSL_OP_LEGACY_SERVER_CONNECT)) { + al = SSL_AD_HANDSHAKE_FAILURE; + SSLerror(s, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED); + goto f_err; + } + if (ssl_check_serverhello_tlsext(s) <= 0) { SSLerror(s, SSL_R_SERVERHELLO_TLSEXT); goto err; } - /* See if any data remains... */ - if (p - CBS_data(&cbs) != CBS_len(&cbs)) - goto truncated; - return (1); truncated: diff --git a/src/lib/libssl/ssl_tlsext.c b/src/lib/libssl/ssl_tlsext.c index 0e3ef7da15..3735b719db 100644 --- a/src/lib/libssl/ssl_tlsext.c +++ b/src/lib/libssl/ssl_tlsext.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_tlsext.c,v 1.20 2018/01/27 15:30:05 jsing Exp $ */ +/* $OpenBSD: ssl_tlsext.c,v 1.21 2018/02/08 11:30:30 jsing Exp $ */ /* * Copyright (c) 2016, 2017 Joel Sing * Copyright (c) 2017 Doug Hogan @@ -1310,19 +1310,34 @@ tls_extension_find(uint16_t type, size_t *tls_extensions_idx) return NULL; } -static void -tlsext_clienthello_reset_state(SSL *s) +static int +tls_extension_needs(struct tls_extension *tlsext, int is_serverhello, SSL *s) { - s->internal->servername_done = 0; - s->tlsext_status_type = -1; - S3I(s)->renegotiate_seen = 0; - free(S3I(s)->alpn_selected); - S3I(s)->alpn_selected = NULL; - s->internal->srtp_profile = NULL; + if (is_serverhello) + return tlsext->serverhello_needs(s); + return tlsext->clienthello_needs(s); } -int -tlsext_clienthello_build(SSL *s, CBB *cbb) +static int +tls_extension_build(struct tls_extension *tlsext, int is_serverhello, SSL *s, + CBB *cbb) +{ + if (is_serverhello) + return tlsext->serverhello_build(s, cbb); + return tlsext->clienthello_build(s, cbb); +} + +static int +tls_extension_parse(struct tls_extension *tlsext, int is_serverhello, SSL *s, + CBS *cbs, int *alert) +{ + if (is_serverhello) + return tlsext->serverhello_parse(s, cbs, alert); + return tlsext->clienthello_parse(s, cbs, alert); +} + +static int +tlsext_build(SSL *s, CBB *cbb, int is_serverhello) { CBB extensions, extension_data; struct tls_extension *tlsext; @@ -1335,14 +1350,16 @@ tlsext_clienthello_build(SSL *s, CBB *cbb) for (i = 0; i < N_TLS_EXTENSIONS; i++) { tlsext = &tls_extensions[i]; - if (!tlsext->clienthello_needs(s)) + if (!tls_extension_needs(tlsext, is_serverhello, s)) continue; if (!CBB_add_u16(&extensions, tlsext->type)) return 0; if (!CBB_add_u16_length_prefixed(&extensions, &extension_data)) return 0; - if (!tls_extensions[i].clienthello_build(s, &extension_data)) + + if (!tls_extension_build(tlsext, is_serverhello, s, + &extension_data)) return 0; extensions_present = 1; @@ -1357,8 +1374,8 @@ tlsext_clienthello_build(SSL *s, CBB *cbb) return 1; } -int -tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert) +static int +tlsext_parse(SSL *s, CBS *cbs, int *alert, int is_serverhello) { CBS extensions, extension_data; struct tls_extension *tlsext; @@ -1366,9 +1383,6 @@ tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert) uint16_t type; size_t idx; - /* XXX - this possibly should be done by the caller... */ - tlsext_clienthello_reset_state(s); - /* An empty extensions block is valid. */ if (CBS_len(cbs) == 0) return 1; @@ -1385,7 +1399,7 @@ tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert) return 0; if (s->internal->tlsext_debug_cb != NULL) - s->internal->tlsext_debug_cb(s, 0, type, + s->internal->tlsext_debug_cb(s, is_serverhello, type, (unsigned char *)CBS_data(&extension_data), CBS_len(&extension_data), s->internal->tlsext_debug_arg); @@ -1394,12 +1408,13 @@ tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert) if ((tlsext = tls_extension_find(type, &idx)) == NULL) continue; - /* Check for duplicate extensions. */ + /* Check for duplicate known extensions. */ if ((extensions_seen & (1 << idx)) != 0) return 0; extensions_seen |= (1 << idx); - if (!tlsext->clienthello_parse(s, &extension_data, alert)) + if (!tls_extension_parse(tlsext, is_serverhello, s, + &extension_data, alert)) return 0; if (CBS_len(&extension_data) != 0) @@ -1409,63 +1424,51 @@ tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert) return 1; } -int -tlsext_serverhello_build(SSL *s, CBB *cbb) +static void +tlsext_clienthello_reset_state(SSL *s) { - CBB extensions, extension_data; - struct tls_extension *tlsext; - int extensions_present = 0; - size_t i; - - if (!CBB_add_u16_length_prefixed(cbb, &extensions)) - return 0; - - for (i = 0; i < N_TLS_EXTENSIONS; i++) { - tlsext = &tls_extensions[i]; - - if (!tlsext->serverhello_needs(s)) - continue; - - if (!CBB_add_u16(&extensions, tlsext->type)) - return 0; - if (!CBB_add_u16_length_prefixed(&extensions, &extension_data)) - return 0; - if (!tlsext->serverhello_build(s, &extension_data)) - return 0; + s->internal->servername_done = 0; + s->tlsext_status_type = -1; + S3I(s)->renegotiate_seen = 0; + free(S3I(s)->alpn_selected); + S3I(s)->alpn_selected = NULL; + s->internal->srtp_profile = NULL; +} - extensions_present = 1; - } +int +tlsext_clienthello_build(SSL *s, CBB *cbb) +{ + return tlsext_build(s, cbb, 0); +} - if (!extensions_present) - CBB_discard_child(cbb); +int +tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert) +{ + /* XXX - this possibly should be done by the caller... */ + tlsext_clienthello_reset_state(s); - if (!CBB_flush(cbb)) - return 0; + return tlsext_parse(s, cbs, alert, 0); +} - return 1; +static void +tlsext_serverhello_reset_state(SSL *s) +{ + S3I(s)->renegotiate_seen = 0; + free(S3I(s)->alpn_selected); + S3I(s)->alpn_selected = NULL; } int -tlsext_serverhello_parse_one(SSL *s, CBS *cbs, uint16_t type, int *alert) +tlsext_serverhello_build(SSL *s, CBB *cbb) { - struct tls_extension *tlsext; - size_t i; - - for (i = 0; i < N_TLS_EXTENSIONS; i++) { - tlsext = &tls_extensions[i]; - - if (tlsext->type != type) - continue; - if (!tlsext->serverhello_parse(s, cbs, alert)) - return 0; - if (CBS_len(cbs) != 0) { - *alert = SSL_AD_DECODE_ERROR; - return 0; - } + return tlsext_build(s, cbb, 1); +} - return 1; - } +int +tlsext_serverhello_parse(SSL *s, CBS *cbs, int *alert) +{ + /* XXX - this possibly should be done by the caller... */ + tlsext_serverhello_reset_state(s); - /* Not found. */ - return 2; + return tlsext_parse(s, cbs, alert, 1); } diff --git a/src/lib/libssl/ssl_tlsext.h b/src/lib/libssl/ssl_tlsext.h index 1af2e6cb3b..4248932fb2 100644 --- a/src/lib/libssl/ssl_tlsext.h +++ b/src/lib/libssl/ssl_tlsext.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_tlsext.h,v 1.11 2018/01/27 15:30:05 jsing Exp $ */ +/* $OpenBSD: ssl_tlsext.h,v 1.12 2018/02/08 11:30:30 jsing Exp $ */ /* * Copyright (c) 2016, 2017 Joel Sing * Copyright (c) 2017 Doug Hogan @@ -85,5 +85,4 @@ int tlsext_clienthello_build(SSL *s, CBB *cbb); int tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert); int tlsext_serverhello_build(SSL *s, CBB *cbb); -int tlsext_serverhello_parse_one(SSL *s, CBS *cbs, uint16_t tlsext_type, - int *alert); +int tlsext_serverhello_parse(SSL *s, CBS *cbs, int *alert); diff --git a/src/lib/libssl/t1_lib.c b/src/lib/libssl/t1_lib.c index fbd79431db..d92fd70f5b 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.140 2018/01/27 15:30:05 jsing Exp $ */ +/* $OpenBSD: t1_lib.c,v 1.141 2018/02/08 11:30:30 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -661,79 +661,6 @@ tls12_get_req_sig_algs(SSL *s, unsigned char **sigalgs, size_t *sigalgs_len) *sigalgs_len = sizeof(tls12_sigalgs); } -int -ssl_parse_serverhello_tlsext(SSL *s, unsigned char **p, size_t n, int *al) -{ - unsigned short type; - unsigned short size; - unsigned short len; - unsigned char *data = *p; - unsigned char *end = *p + n; - CBS cbs; - - S3I(s)->renegotiate_seen = 0; - free(S3I(s)->alpn_selected); - S3I(s)->alpn_selected = NULL; - - if (data == end) - goto ri_check; - - if (end - data < 2) - goto err; - n2s(data, len); - - if (end - data != len) - goto err; - - while (end - data >= 4) { - n2s(data, type); - n2s(data, size); - - if (end - data < size) - goto err; - - if (s->internal->tlsext_debug_cb) - s->internal->tlsext_debug_cb(s, 1, type, data, size, - s->internal->tlsext_debug_arg); - - CBS_init(&cbs, data, size); - if (!tlsext_serverhello_parse_one(s, &cbs, type, al)) - return 0; - - data += size; - - } - - if (data != end) { - *al = SSL_AD_DECODE_ERROR; - return 0; - } - - *p = data; - -ri_check: - - /* Determine if we need to see RI. Strictly speaking if we want to - * avoid an attack we should *always* see RI even on initial server - * hello because the client doesn't see any renegotiation during an - * attack. However this would mean we could not connect to any server - * which doesn't support RI so for the immediate future tolerate RI - * absence on initial connect only. - */ - if (!S3I(s)->renegotiate_seen && - !(s->internal->options & SSL_OP_LEGACY_SERVER_CONNECT)) { - *al = SSL_AD_HANDSHAKE_FAILURE; - SSLerror(s, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED); - return 0; - } - - return 1; - -err: - *al = SSL_AD_DECODE_ERROR; - return 0; -} - int ssl_check_clienthello_tlsext_early(SSL *s) { -- cgit v1.2.3-55-g6feb