From 5829f96efa11f059e99ec190377c21b0d1c6d273 Mon Sep 17 00:00:00 2001 From: tb Date: Tue, 16 Apr 2024 17:46:30 +0000 Subject: [PATCH] Fix key share negotiation in HRR case In the ClientHello retrying the handshake after a HelloRetryRequest, the client must send a single key share matching the group selected by the server in the HRR. This is not necessarily the mutually preferred group. Incorrect logic added in ssl_tlsect.c r1.134 would potentially reject such a key share because of that. Instead, add logic to ensure on the server side that there is a single share matching the group we selected in the HRR. Fixes a regress test in p5-IO-Socket-SSL where server is configured with P-521:P-384 and the client with P-256:P-384:P-521. Since the client sends an initial P-256 key share, a HRR is triggered which the faulty logic rejected because it was not the mutually preferred P-384 but rather matching the server-selected P-521. This will need some deduplication in subsequent commits. We may also want to consider honoring the mutual preference and request a key accordingly in the HRR. reported by bluhm, fix suggested by jsing ok beck jsing --- lib/libssl/ssl_tlsext.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c index 6d8f51833b2..64fa52e20cc 100644 --- a/lib/libssl/ssl_tlsext.c +++ b/lib/libssl/ssl_tlsext.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_tlsext.c,v 1.148 2024/04/04 08:02:21 tb Exp $ */ +/* $OpenBSD: ssl_tlsext.c,v 1.149 2024/04/16 17:46:30 tb Exp $ */ /* * Copyright (c) 2016, 2017, 2019 Joel Sing * Copyright (c) 2017 Doug Hogan @@ -1493,6 +1493,45 @@ tlsext_keyshare_server_process(SSL *s, uint16_t msg_type, CBS *cbs, int *alert) return 0; } + if (s->s3->hs.tls13.hrr) { + if (!CBS_get_u16_length_prefixed(cbs, &client_shares)) + return 0; + + /* Unpack client share. */ + if (!CBS_get_u16(&client_shares, &group)) + return 0; + if (!CBS_get_u16_length_prefixed(&client_shares, &key_exchange)) + return 0; + + /* There should only be one share. */ + if (CBS_len(&client_shares) != 0) + return 0; + + if (group != s->s3->hs.tls13.server_group) { + *alert = SSL_AD_ILLEGAL_PARAMETER; + return 0; + } + + if (s->s3->hs.key_share != NULL) { + *alert = SSL_AD_INTERNAL_ERROR; + return 0; + } + + /* Decode and store the selected key share. */ + if ((s->s3->hs.key_share = tls_key_share_new(group)) == NULL) { + *alert = SSL_AD_INTERNAL_ERROR; + return 0; + } + if (!tls_key_share_peer_public(s->s3->hs.key_share, + &key_exchange, &decode_error, NULL)) { + if (!decode_error) + *alert = SSL_AD_INTERNAL_ERROR; + return 0; + } + + return 1; + } + /* * XXX similar to tls1_get_supported_group, but client pref * only - consider deduping later. -- 2.20.1