From c18a60d45888295bb8cf344e076d84ef817a65a5 Mon Sep 17 00:00:00 2001 From: jsing <> Date: Wed, 22 Apr 2020 17:05:07 +0000 Subject: Improve TLSv1.3 state machine for HelloRetryRequest handling. The state machine currently handles the HelloRetryRequest case by using WITH_HRR - in other words, we're explicitly indicating when we transition to the alternate path. The problem here is that we do not know if we're going to receive a ServerHello or a HelloRetryRequest until we process the message. This means that the ServerHello processing code has to handle both types of messages. The state machine and associated processing code becomes cleaner if we flip this around so that we assume we are going to receive a HelloRetryRequest and upon discovering that it is not, trigger WITHOUT_HRR and hand off to the ServerHello processing function. In particular, this makes the logic much more straight forward on the server side, when adding support for HRR. With feedback from tb@ ok tb@ --- src/lib/libssl/tls13_client.c | 110 ++++++++++++++++++++++++++++-------------- 1 file changed, 73 insertions(+), 37 deletions(-) (limited to 'src/lib/libssl/tls13_client.c') diff --git a/src/lib/libssl/tls13_client.c b/src/lib/libssl/tls13_client.c index 0da08f62c3..dffabf1753 100644 --- a/src/lib/libssl/tls13_client.c +++ b/src/lib/libssl/tls13_client.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tls13_client.c,v 1.50 2020/04/21 16:55:17 jsing Exp $ */ +/* $OpenBSD: tls13_client.c,v 1.51 2020/04/22 17:05:07 jsing Exp $ */ /* * Copyright (c) 2018, 2019 Joel Sing * @@ -280,6 +280,24 @@ tls13_server_hello_is_legacy(CBS *cbs) return (selected_version < TLS1_3_VERSION); } +static int +tls13_server_hello_is_retry(CBS *cbs) +{ + CBS server_hello, server_random; + uint16_t legacy_version; + + CBS_dup(cbs, &server_hello); + + if (!CBS_get_u16(&server_hello, &legacy_version)) + return 0; + if (!CBS_get_bytes(&server_hello, &server_random, SSL3_RANDOM_SIZE)) + return 0; + + /* See if this is a HelloRetryRequest. */ + return CBS_mem_equal(&server_random, tls13_hello_retry_request_hash, + sizeof(tls13_hello_retry_request_hash)); +} + static int tls13_server_hello_process(struct tls13_ctx *ctx, CBS *cbs) { @@ -331,7 +349,8 @@ tls13_server_hello_process(struct tls13_ctx *ctx, CBS *cbs) /* From here on in we know we are doing TLSv1.3. */ tls13_record_layer_allow_legacy_alerts(ctx->rl, 0); - /* See if this is a Hello Retry Request. */ + /* See if this is a HelloRetryRequest. */ + /* XXX - see if we can avoid doing this twice. */ if (CBS_mem_equal(&server_random, tls13_hello_retry_request_hash, sizeof(tls13_hello_retry_request_hash))) { tlsext_msg_type = SSL_TLSEXT_MSG_HRR; @@ -514,41 +533,76 @@ tls13_client_engage_record_protection(struct tls13_ctx *ctx) return ret; } +int +tls13_server_hello_retry_request_recv(struct tls13_ctx *ctx, CBS *cbs) +{ + /* + * The state machine has no way of knowing if we're going to receive a + * HelloRetryRequest or a ServerHello. As such, we have to handle + * this case here and hand off to the appropriate function. + */ + if (!tls13_server_hello_is_retry(cbs)) { + ctx->handshake_stage.hs_type |= WITHOUT_HRR; + return tls13_server_hello_recv(ctx, cbs); + } + + if (!tls13_server_hello_process(ctx, cbs)) + return 0; + + /* + * This may have been a TLSv1.2 or earlier ServerHello that just happened + * to have matching server random... + */ + if (ctx->hs->use_legacy) + return tls13_use_legacy_client(ctx); + + if (!ctx->hs->hrr) + return 0; + + if (!tls13_client_synthetic_handshake_message(ctx)) + return 0; + if (!tls13_handshake_msg_record(ctx)) + return 0; + + ctx->hs->hrr = 0; + + return 1; +} + int tls13_server_hello_recv(struct tls13_ctx *ctx, CBS *cbs) { SSL *s = ctx->ssl; /* - * We may have received a legacy (pre-TLSv1.3) server hello, - * a TLSv1.3 server hello or a TLSv1.3 hello retry request. + * We may have received a legacy (pre-TLSv1.3) ServerHello or a TLSv1.3 + * ServerHello. HelloRetryRequests have already been handled. */ if (!tls13_server_hello_process(ctx, cbs)) return 0; - tls1_transcript_unfreeze(s); - - if (ctx->hs->hrr) { - if (!tls13_client_synthetic_handshake_message(ctx)) + if (ctx->handshake_stage.hs_type & WITHOUT_HRR) { + tls1_transcript_unfreeze(s); + if (!tls13_handshake_msg_record(ctx)) return 0; } - if (!tls13_handshake_msg_record(ctx)) - return 0; - - if (ctx->hs->use_legacy) + if (ctx->hs->use_legacy) { + if (!(ctx->handshake_stage.hs_type & WITHOUT_HRR)) + return 0; return tls13_use_legacy_client(ctx); + } - if (!ctx->hs->hrr) { - if (!tls13_client_engage_record_protection(ctx)) - return 0; + if (ctx->hs->hrr) { + /* The server has sent two HelloRetryRequests. */ + ctx->alert = SSL_AD_ILLEGAL_PARAMETER; + return 0; } - ctx->handshake_stage.hs_type |= NEGOTIATED; - if (ctx->hs->hrr) - ctx->handshake_stage.hs_type |= WITH_HRR; + if (!tls13_client_engage_record_protection(ctx)) + return 0; - ctx->hs->hrr = 0; + ctx->handshake_stage.hs_type |= NEGOTIATED; return 1; } @@ -580,24 +634,6 @@ tls13_client_hello_retry_send(struct tls13_ctx *ctx, CBB *cbb) return 1; } -int -tls13_server_hello_retry_recv(struct tls13_ctx *ctx, CBS *cbs) -{ - if (!tls13_server_hello_process(ctx, cbs)) - return 0; - - if (ctx->hs->use_legacy) - return 0; /* XXX alert */ - - if (ctx->hs->hrr) - return 0; /* XXX alert */ - - if (!tls13_client_engage_record_protection(ctx)) - return 0; - - return 1; -} - int tls13_server_encrypted_extensions_recv(struct tls13_ctx *ctx, CBS *cbs) { -- cgit v1.2.3-55-g6feb