From fd8e9d0d2ae7d688e66e14924e6ca7211c758d65 Mon Sep 17 00:00:00 2001 From: jsing Date: Sat, 27 Jan 2018 15:30:05 +0000 Subject: [PATCH] Complete the TLS extension handling rewrite for the server-side. This removes ssl_parse_clienthello_tlsext() and allows the CBS to be passed all the way through from ssl3_get_client_hello(). The renegotation check gets pulled up into ssl3_get_client_hello() which is where other such checks exist. The TLS extension parsing now also ensures that we do not get duplicates of any known extensions (the old pre-rewrite code only did this for some extensions). ok inoguchi@ --- lib/libssl/ssl_locl.h | 7 ++-- lib/libssl/ssl_srvr.c | 19 +++++----- lib/libssl/ssl_tlsext.c | 82 ++++++++++++++++++++++++++++++++++------- lib/libssl/ssl_tlsext.h | 5 +-- lib/libssl/t1_lib.c | 71 +---------------------------------- 5 files changed, 86 insertions(+), 98 deletions(-) diff --git a/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h index f6e922e99cb..d2a99afaa49 100644 --- a/lib/libssl/ssl_locl.h +++ b/lib/libssl/ssl_locl.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_locl.h,v 1.201 2017/10/12 16:06:32 jsing Exp $ */ +/* $OpenBSD: ssl_locl.h,v 1.202 2018/01/27 15:30:05 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -164,6 +164,9 @@ __BEGIN_HIDDEN_DECLS +#define CTASSERT(x) extern char _ctassert[(x) ? 1 : -1 ] \ + __attribute__((__unused__)) + #define l2n(l,c) (*((c)++)=(unsigned char)(((l)>>24)&0xff), \ *((c)++)=(unsigned char)(((l)>>16)&0xff), \ *((c)++)=(unsigned char)(((l)>> 8)&0xff), \ @@ -1275,8 +1278,6 @@ uint16_t tls1_ec_nid2curve_id(const int nid); int tls1_check_curve(SSL *s, const uint16_t curve_id); int tls1_get_shared_curve(SSL *s); -int ssl_parse_clienthello_tlsext(SSL *s, unsigned char **data, - unsigned char *d, int n, int *al); int ssl_parse_serverhello_tlsext(SSL *s, unsigned char **data, size_t n, int *al); int ssl_check_clienthello_tlsext_early(SSL *s); diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c index 5d741cdc811..6450623d4a3 100644 --- a/lib/libssl/ssl_srvr.c +++ b/lib/libssl/ssl_srvr.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_srvr.c,v 1.26 2017/10/12 15:52:50 jsing Exp $ */ +/* $OpenBSD: ssl_srvr.c,v 1.27 2018/01/27 15:30:05 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -813,7 +813,6 @@ ssl3_get_client_hello(SSL *s) int i, j, ok, al, ret = -1, cookie_valid = 0; long n; unsigned long id; - unsigned char *p, *d; SSL_CIPHER *c; STACK_OF(SSL_CIPHER) *ciphers = NULL; unsigned long alg_k; @@ -843,8 +842,7 @@ ssl3_get_client_hello(SSL *s) if (n < 0) goto err; - d = p = (unsigned char *)s->internal->init_msg; - end = d + n; + end = (unsigned char *)s->internal->init_msg + n; CBS_init(&cbs, s->internal->init_msg, n); @@ -1038,14 +1036,17 @@ ssl3_get_client_hello(SSL *s) goto f_err; } - p = (unsigned char *)CBS_data(&cbs); - - /* TLS extensions*/ - if (!ssl_parse_clienthello_tlsext(s, &p, d, n, &al)) { - /* 'al' set by ssl_parse_clienthello_tlsext */ + if (!tlsext_clienthello_parse(s, &cbs, &al)) { SSLerror(s, SSL_R_PARSE_TLSEXT); goto f_err; } + + if (!S3I(s)->renegotiate_seen && s->internal->renegotiate) { + al = SSL_AD_HANDSHAKE_FAILURE; + SSLerror(s, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED); + goto f_err; + } + if (ssl_check_clienthello_tlsext_early(s) <= 0) { SSLerror(s, SSL_R_CLIENTHELLO_TLSEXT); goto err; diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c index d0764af3c01..0e3ef7da154 100644 --- a/lib/libssl/ssl_tlsext.c +++ b/lib/libssl/ssl_tlsext.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_tlsext.c,v 1.19 2018/01/27 15:17:13 jsing Exp $ */ +/* $OpenBSD: ssl_tlsext.c,v 1.20 2018/01/27 15:30:05 jsing Exp $ */ /* * Copyright (c) 2016, 2017 Joel Sing * Copyright (c) 2017 Doug Hogan @@ -1292,6 +1292,35 @@ static struct tls_extension tls_extensions[] = { #define N_TLS_EXTENSIONS (sizeof(tls_extensions) / sizeof(*tls_extensions)) +/* Ensure that extensions fit in a uint32_t bitmask. */ +CTASSERT(N_TLS_EXTENSIONS <= (sizeof(uint32_t) * 8)); + +static struct tls_extension * +tls_extension_find(uint16_t type, size_t *tls_extensions_idx) +{ + size_t i; + + for (i = 0; i < N_TLS_EXTENSIONS; i++) { + if (tls_extensions[i].type == type) { + *tls_extensions_idx = i; + return &tls_extensions[i]; + } + } + + return NULL; +} + +static void +tlsext_clienthello_reset_state(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; +} + int tlsext_clienthello_build(SSL *s, CBB *cbb) { @@ -1329,28 +1358,55 @@ tlsext_clienthello_build(SSL *s, CBB *cbb) } int -tlsext_clienthello_parse_one(SSL *s, CBS *cbs, uint16_t type, int *alert) +tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert) { + CBS extensions, extension_data; struct tls_extension *tlsext; - size_t i; + uint32_t extensions_seen = 0; + uint16_t type; + size_t idx; - for (i = 0; i < N_TLS_EXTENSIONS; i++) { - tlsext = &tls_extensions[i]; + /* XXX - this possibly should be done by the caller... */ + tlsext_clienthello_reset_state(s); - if (tlsext->type != type) + /* An empty extensions block is valid. */ + if (CBS_len(cbs) == 0) + return 1; + + *alert = SSL_AD_DECODE_ERROR; + + if (!CBS_get_u16_length_prefixed(cbs, &extensions)) + return 0; + + while (CBS_len(&extensions) > 0) { + if (!CBS_get_u16(&extensions, &type)) + return 0; + if (!CBS_get_u16_length_prefixed(&extensions, &extension_data)) + return 0; + + if (s->internal->tlsext_debug_cb != NULL) + s->internal->tlsext_debug_cb(s, 0, type, + (unsigned char *)CBS_data(&extension_data), + CBS_len(&extension_data), + s->internal->tlsext_debug_arg); + + /* Unknown extensions are ignored. */ + if ((tlsext = tls_extension_find(type, &idx)) == NULL) continue; - if (!tlsext->clienthello_parse(s, cbs, alert)) + + /* Check for duplicate extensions. */ + if ((extensions_seen & (1 << idx)) != 0) return 0; - if (CBS_len(cbs) != 0) { - *alert = SSL_AD_DECODE_ERROR; + extensions_seen |= (1 << idx); + + if (!tlsext->clienthello_parse(s, &extension_data, alert)) return 0; - } - return 1; + if (CBS_len(&extension_data) != 0) + return 0; } - /* Not found. */ - return 2; + return 1; } int diff --git a/lib/libssl/ssl_tlsext.h b/lib/libssl/ssl_tlsext.h index 7c6250a7f7d..1af2e6cb3b1 100644 --- a/lib/libssl/ssl_tlsext.h +++ b/lib/libssl/ssl_tlsext.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_tlsext.h,v 1.10 2017/08/27 02:58:04 doug Exp $ */ +/* $OpenBSD: ssl_tlsext.h,v 1.11 2018/01/27 15:30:05 jsing Exp $ */ /* * Copyright (c) 2016, 2017 Joel Sing * Copyright (c) 2017 Doug Hogan @@ -82,8 +82,7 @@ int tlsext_srtp_serverhello_parse(SSL *s, CBS *cbs, int *alert); #endif int tlsext_clienthello_build(SSL *s, CBB *cbb); -int tlsext_clienthello_parse_one(SSL *s, CBS *cbs, uint16_t tlsext_type, - int *alert); +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, diff --git a/lib/libssl/t1_lib.c b/lib/libssl/t1_lib.c index 1cef08d0946..fbd79431db6 100644 --- a/lib/libssl/t1_lib.c +++ b/lib/libssl/t1_lib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: t1_lib.c,v 1.139 2017/10/11 17:35:00 jsing Exp $ */ +/* $OpenBSD: t1_lib.c,v 1.140 2018/01/27 15:30:05 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -661,75 +661,6 @@ tls12_get_req_sig_algs(SSL *s, unsigned char **sigalgs, size_t *sigalgs_len) *sigalgs_len = sizeof(tls12_sigalgs); } -int -ssl_parse_clienthello_tlsext(SSL *s, unsigned char **p, unsigned char *d, - int n, int *al) -{ - unsigned short type; - unsigned short size; - unsigned short len; - unsigned char *data = *p; - unsigned char *end = d + n; - CBS cbs; - - 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 (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, 0, type, data, size, - s->internal->tlsext_debug_arg); - - CBS_init(&cbs, data, size); - if (!tlsext_clienthello_parse_one(s, &cbs, type, al)) - return 0; - - data += size; - } - - /* Spurious data on the end */ - if (data != end) - goto err; - - *p = data; - -ri_check: - - /* Need RI if renegotiating */ - - if (!S3I(s)->renegotiate_seen && s->internal->renegotiate) { - *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_parse_serverhello_tlsext(SSL *s, unsigned char **p, size_t n, int *al) { -- 2.20.1