From be89428ce5fb176ef7adb0cb4f297af5f2b0fb91 Mon Sep 17 00:00:00 2001 From: beck Date: Wed, 27 Mar 2024 22:27:09 +0000 Subject: [PATCH] Fix up server processing of key shares. Ensure that the client can not provide a duplicate key share for any group, or send more key shares than groups they support. Ensure that the key shares must be provided in the same order as the client preference order specified in supported_groups. Ensure we only will choose to use a key share that is for the most preferred group by the client that we also support, to avoid the client being downgraded by sending a less preferred key share. If we do not end up with a key share for the most preferred mutually supported group, will then do a hello retry request selecting that group. Add regress for this to regress/tlsext/tlsexttest.c ok jsing@ --- lib/libssl/ssl_tlsext.c | 85 +++++++++++++++++++-- regress/lib/libssl/tlsext/tlsexttest.c | 101 +++++++++++++++++++++++-- 2 files changed, 173 insertions(+), 13 deletions(-) diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c index e4ba5498142..14cf6fce84b 100644 --- a/lib/libssl/ssl_tlsext.c +++ b/lib/libssl/ssl_tlsext.c @@ -1,8 +1,8 @@ -/* $OpenBSD: ssl_tlsext.c,v 1.144 2024/03/27 10:44:17 beck Exp $ */ +/* $OpenBSD: ssl_tlsext.c,v 1.145 2024/03/27 22:27:09 beck Exp $ */ /* * Copyright (c) 2016, 2017, 2019 Joel Sing * Copyright (c) 2017 Doug Hogan - * Copyright (c) 2018-2019 Bob Beck + * Copyright (c) 2018-2019, 2024 Bob Beck * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above @@ -1464,14 +1464,65 @@ tlsext_keyshare_client_build(SSL *s, uint16_t msg_type, CBB *cbb) static int tlsext_keyshare_server_process(SSL *s, uint16_t msg_type, CBS *cbs, int *alert) { - CBS client_shares, key_exchange; + const uint16_t *client_groups = NULL, *server_groups = NULL; + size_t client_groups_len = 0, server_groups_len = 0; + size_t i, j, client_groups_index; + int preferred_group_found = 0; int decode_error; - uint16_t group; + uint16_t group, client_preferred_group; + CBS client_shares, key_exchange; + + /* + * RFC 8446 section 4.2.8: + * + * Each KeyShareEntry value MUST correspond to a group offered in the + * "supported_groups" extension and MUST appear in the same order. + * However, the values MAY be a non-contiguous subset of the + * "supported_groups". + */ + + if (!tlsext_extension_seen(s, TLSEXT_TYPE_supported_groups)) { + *alert = SSL_AD_ILLEGAL_PARAMETER; + return 0; + } + if (!tlsext_extension_processed(s, TLSEXT_TYPE_supported_groups)) { + *alert = SSL_AD_INTERNAL_ERROR; + return 0; + } + + /* + * XXX similar to tls1_get_supported_group, but client pref + * only - consider deduping later. + */ + /* + * We are now assured of at least one client group. + * Get the client and server group preference orders. + */ + tls1_get_group_list(s, 0, &server_groups, &server_groups_len); + tls1_get_group_list(s, 1, &client_groups, &client_groups_len); + + /* + * Find the group that is most preferred by the client that + * we also support. + */ + for (i = 0; i < client_groups_len && !preferred_group_found; i++) { + if (!ssl_security_supported_group(s, client_groups[i])) + continue; + for (j = 0; j < server_groups_len; j++) { + if (server_groups[j] == client_groups[i]) { + client_preferred_group = client_groups[i]; + preferred_group_found = 1; + break; + } + } + } if (!CBS_get_u16_length_prefixed(cbs, &client_shares)) return 0; + client_groups_index = 0; while (CBS_len(&client_shares) > 0) { + int client_sent_group; /* Unpack client share. */ if (!CBS_get_u16(&client_shares, &group)) @@ -1480,9 +1531,21 @@ tlsext_keyshare_server_process(SSL *s, uint16_t msg_type, CBS *cbs, int *alert) return 0; /* - * XXX - check key exchange against supported groups from client. - * XXX - check that groups only appear once. + * Ensure the client share group was sent in supported groups, + * and was sent in the same order as supported groups. The + * supported groups has already been checked for duplicates. */ + client_sent_group = 0; + while (client_groups_index < client_groups_len) { + if (group == client_groups[client_groups_index++]) { + client_sent_group = 1; + break; + } + } + if (!client_sent_group) { + *alert = SSL_AD_ILLEGAL_PARAMETER; + return 0; + } /* * Ignore this client share if we're using earlier than TLSv1.3 @@ -1493,8 +1556,14 @@ tlsext_keyshare_server_process(SSL *s, uint16_t msg_type, CBS *cbs, int *alert) if (s->s3->hs.key_share != NULL) continue; - /* XXX - consider implementing server preference. */ - if (!tls1_check_group(s, group)) + /* + * Ignore this client share if it is not for the most client + * preferred supported group. This avoids a potential downgrade + * situation where the client sends a client share for something + * less preferred, and we choose to to use it instead of + * requesting the more preferred group. + */ + if (!preferred_group_found || group != client_preferred_group) continue; /* Decode and store the selected key share. */ diff --git a/regress/lib/libssl/tlsext/tlsexttest.c b/regress/lib/libssl/tlsext/tlsexttest.c index c7983833dcc..e61722542ed 100644 --- a/regress/lib/libssl/tlsext/tlsexttest.c +++ b/regress/lib/libssl/tlsext/tlsexttest.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tlsexttest.c,v 1.86 2024/03/26 02:43:56 beck Exp $ */ +/* $OpenBSD: tlsexttest.c,v 1.87 2024/03/27 22:27:09 beck Exp $ */ /* * Copyright (c) 2017 Joel Sing * Copyright (c) 2017 Doug Hogan @@ -1109,7 +1109,6 @@ test_tlsext_ecpf_client(void) goto err; } - failure = 0; err: @@ -3648,6 +3647,7 @@ test_tlsext_keyshare_client(void) const struct tls_extension_funcs *server_funcs; int failure; size_t dlen; + size_t idx; int alert; CBB cbb; CBS cbs; @@ -3701,18 +3701,109 @@ test_tlsext_keyshare_client(void) goto done; } - (ssl)->version = TLS1_3_VERSION; - CBS_init(&cbs, data, dlen); + ssl->version = TLS1_3_VERSION; + + /* Fake up the ssl enough so the key share can process */ + ssl->session = SSL_SESSION_new(); + ssl->s3 = calloc(1, sizeof(*ssl->s3)); + ssl->session->tlsext_supportedgroups = calloc(4, + sizeof(unsigned short)); + if (ssl->session == NULL || ssl->s3 == NULL || + ssl->session->tlsext_supportedgroups == NULL) { + FAIL("malloc"); + goto done; + } + ssl->session->tlsext_supportedgroups[0] = 29; + ssl->session->tlsext_supportedgroups[1] = 23; + ssl->session->tlsext_supportedgroups[2] = 24; + ssl->session->tlsext_supportedgroups[3] = 25; + ssl->session->tlsext_supportedgroups_length = 4; + tls_extension_find(TLSEXT_TYPE_supported_groups, &idx); + ssl->s3->hs.extensions_processed |= (1 << idx); + ssl->s3->hs.extensions_seen |= (1 << idx); + ssl->s3->hs.our_max_tls_version = TLS1_3_VERSION; + /* + * We should select the key share for group 29, when group 29 + * is the most preferred group + */ + CBS_init(&cbs, data, dlen); if (!server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) { - FAIL("failed to parse client keyshare\n"); + FAIL("failed to process client keyshare\n"); + goto done; + } + if (CBS_len(&cbs) != 0) { + FAIL("extension data remaining\n"); + goto done; + } + if (ssl->s3->hs.key_share == NULL) { + FAIL("Did not select a key share"); goto done; } + /* + * Pretend the client did not send the supported groups extension. We + * should fail to process. + */ + ssl->s3->hs.extensions_seen = 0; + free(ssl->s3->hs.key_share); + ssl->s3->hs.key_share = NULL; + CBS_init(&cbs, data, dlen); + if (server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) { + FAIL("Processed key share when supported groups not provided"); + goto done; + } + ssl->s3->hs.extensions_seen |= (1 << idx); + + /* + * Pretend supported groups did not get processed. We should fail to + * process + */ + ssl->s3->hs.extensions_processed = 0; + ssl->s3->hs.key_share = NULL; + CBS_init(&cbs, data, dlen); + if (server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) { + FAIL("Processed key share when supported groups unprocesed"); + goto done; + } + ssl->s3->hs.extensions_processed |= (1 << idx); + + /* + * Remove group 29 by making it 0xbeef, meaning 29 has not been sent in + * supported groups. This should fail to process. + */ + ssl->session->tlsext_supportedgroups[0] = 0xbeef; + ssl->s3->hs.key_share = NULL; + CBS_init(&cbs, data, dlen); + if (server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) { + FAIL("Processed key share with invalid group!"); + goto done; + } + ssl->session->tlsext_supportedgroups[0] = 29; + + /* + * Make 29 least preferred, while server supports both 29 and 25. + * Client key share is for 29 but it prefers 25. We should successfully + * process, but should not select this key share. + */ + ssl->session->tlsext_supportedgroups[0] = 25; + ssl->session->tlsext_supportedgroups[3] = 29; + ssl->s3->hs.key_share = NULL; + CBS_init(&cbs, data, dlen); + if (!server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) { + FAIL("failed to process client keyshare\n"); + goto done; + } if (CBS_len(&cbs) != 0) { FAIL("extension data remaining\n"); goto done; } + if (ssl->s3->hs.key_share != NULL) { + FAIL("Selected a key share when I should not have!"); + goto done; + } + ssl->session->tlsext_supportedgroups[0] = 29; + ssl->session->tlsext_supportedgroups[3] = 25; failure = 0; -- 2.20.1