From 600b8d8760845e0bc65bf4bd057ede30af717a4b Mon Sep 17 00:00:00 2001 From: tb <> Date: Sat, 23 May 2020 08:47:19 +0000 Subject: Do not assume that server_group != 0 or tlsext_supportedgroups != NULL implies that we're dealing with a HRR in the extension handling code. Explicitly check that we're in this situation by inspecting the flag in the handshake context. Add missing error checks and send the appropriate alerts. The hrr flag needs to be unset after parsing the client hello retry to avoid breaking the server hello handling. All this is far from ideal, but better than nothing. The correct fix would likely be to make the message type available but that would need to be part of a more extensive rearchitecture of the extension handling. Discussed at length with jsing --- src/lib/libssl/ssl_tlsext.c | 20 ++++++++++++-------- src/lib/libssl/tls13_server.c | 4 +++- 2 files changed, 15 insertions(+), 9 deletions(-) (limited to 'src/lib') diff --git a/src/lib/libssl/ssl_tlsext.c b/src/lib/libssl/ssl_tlsext.c index 8949dc3a26..f5343c81ab 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.70 2020/05/19 02:16:16 beck Exp $ */ +/* $OpenBSD: ssl_tlsext.c,v 1.71 2020/05/23 08:47:19 tb Exp $ */ /* * Copyright (c) 2016, 2017, 2019 Joel Sing * Copyright (c) 2017 Doug Hogan @@ -222,13 +222,15 @@ tlsext_supportedgroups_server_parse(SSL *s, CBS *cbs, int *alert) uint16_t *groups; int i; - if (SSI(s)->tlsext_supportedgroups != NULL) { + if (S3I(s)->hs_tls13.hrr) { + if (SSI(s)->tlsext_supportedgroups == NULL) { + *alert = SSL_AD_HANDSHAKE_FAILURE; + return 0; + } /* - * We should only end up here in the case of a TLSv1.3 - * HelloRetryRequest... and the client cannot change - * supported groups. + * In the case of TLSv1.3 the client cannot change + * the supported groups. */ - /* XXX - we should know this is a HRR. */ if (groups_len != SSI(s)->tlsext_supportedgroups_length) { *alert = SSL_AD_ILLEGAL_PARAMETER; return 0; @@ -1410,9 +1412,11 @@ int tlsext_keyshare_server_build(SSL *s, CBB *cbb) { /* In the case of a HRR, we only send the server selected group. */ - /* XXX - we should know this is a HRR. */ - if (S3I(s)->hs_tls13.server_group != 0) + if (S3I(s)->hs_tls13.hrr) { + if (S3I(s)->hs_tls13.server_group == 0) + return 0; return CBB_add_u16(cbb, S3I(s)->hs_tls13.server_group); + } if (S3I(s)->hs_tls13.key_share == NULL) return 0; diff --git a/src/lib/libssl/tls13_server.c b/src/lib/libssl/tls13_server.c index e0ea6b564d..e605ccd90f 100644 --- a/src/lib/libssl/tls13_server.c +++ b/src/lib/libssl/tls13_server.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tls13_server.c,v 1.51 2020/05/22 02:37:27 beck Exp $ */ +/* $OpenBSD: tls13_server.c,v 1.52 2020/05/23 08:47:19 tb Exp $ */ /* * Copyright (c) 2019, 2020 Joel Sing * Copyright (c) 2020 Bob Beck @@ -365,6 +365,8 @@ tls13_client_hello_retry_recv(struct tls13_ctx *ctx, CBS *cbs) if (s->method->internal->version < TLS1_3_VERSION) return 0; + ctx->hs->hrr = 0; + return 1; } -- cgit v1.2.3-55-g6feb