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

diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c
index 6d8f518..64fa52e 100644 (file)
--- 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 <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -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.