Add API to configure server's NamedGroups with equal preference This adds an API to associate each named group with a "flags" value, and adds a flag that indicates that the group has equal preference with the next group in the list. This is analogous to "in_group" flags used with SSL ciphers (but is renamed to avoid using the overloaded term "group"). When SSL_OP_CIPHER_SERVER_PREFERENCE is specified, the server's supported group list is used, so these "with next" flags allow the server to express equal preference between consecutively listed groups, so that a choice among them respects the client's preference. Bug: 437414714 Change-Id: I73ec6523d6a6e3d852a5664d49cf1f0a6a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/85787 Commit-Queue: David Benjamin <davidben@google.com> Auto-Submit: Lily Chen <chlily@google.com> Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index a7d5954..0e99279 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -2574,6 +2574,40 @@ OPENSSL_EXPORT int SSL_set1_group_ids(SSL *ssl, const uint16_t *group_ids, size_t num_group_ids); +// SSL_GROUP_FLAG_* define flags used with SSL_CTX_set1_group_ids_with_flags +// and SSL_set1_group_ids_with_flags. +// +// If configuring a server, SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT indicates +// that the corresponding group has equal preference with the next member of the +// list of groups being configured. Assigning equal preference to a range of +// consecutively listed groups allows a server to partially respect the +// client's preferences when |SSL_OP_CIPHER_SERVER_PREFERENCE| is enabled. +#define SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT 0x01 + +// SSL_CTX_set1_group_ids_with_flags sets the preferred groups for |ctx| to +// |group_ids|, using the corresponding |flags| for each element, which is a set +// of SSL_GROUP_FLAG_* values ORed together. Each element of |group_ids| should +// be a unique one of the |SSL_GROUP_*| constants. If |group_ids| is empty, a +// default list of groups and flags defaulting to zero will be set instead. +// |group_ids| and |flags| should both have |num_group_ids| elements. It returns +// one on success and zero on failure. +OPENSSL_EXPORT int SSL_CTX_set1_group_ids_with_flags(SSL_CTX *ctx, + const uint16_t *group_ids, + const uint32_t *flags, + size_t num_group_ids); + +// SSL_set1_group_ids_with_flags sets the preferred groups for |ssl| to +// |group_ids|, using the corresponding |flags| for each element, which is a set +// of SSL_GROUP_FLAG_* values ORed toegether. Each element of |group_ids| should +// be a unique one of the |SSL_GROUP_*| constants. If |group_ids| is empty, a +// default list of groups and flags defaulting to zero will be set instead. +// |group_ids| and |flags| should both have |num_group_ids| elements. It +// returns one on success and zero on failure. +OPENSSL_EXPORT int SSL_set1_group_ids_with_flags(SSL *ssl, + const uint16_t *group_ids, + const uint32_t *flags, + size_t num_group_ids); + // SSL_get_group_id returns the ID of the group used by |ssl|'s most recently // completed handshake, or 0 if not applicable. OPENSSL_EXPORT uint16_t SSL_get_group_id(const SSL *ssl);
diff --git a/ssl/extensions.cc b/ssl/extensions.cc index fcb0bf8..077acb4 100644 --- a/ssl/extensions.cc +++ b/ssl/extensions.cc
@@ -20,6 +20,7 @@ #include <string.h> #include <algorithm> +#include <optional> #include <utility> #include <openssl/aead.h> @@ -216,28 +217,48 @@ Span<const uint16_t> groups = hs->config->supported_group_list; Span<const uint16_t> pref, supp; + Span<const uint32_t> pref_flags; if (ssl->options & SSL_OP_CIPHER_SERVER_PREFERENCE) { pref = groups; + pref_flags = hs->config->supported_group_list_flags; supp = hs->peer_supported_group_list; } else { pref = hs->peer_supported_group_list; supp = groups; } - for (uint16_t pref_group : pref) { - for (uint16_t supp_group : supp) { - if (pref_group == supp_group && - // Post-quantum key agreements don't fit in the u8-length-prefixed - // ECPoint field in TLS 1.2 and below. - (ssl_protocol_version(ssl) >= TLS1_3_VERSION || - !is_post_quantum_group(pref_group))) { - *out_group_id = pref_group; - return true; + // Index within `supp` of the best matching group so far. + std::optional<size_t> best_match; + for (size_t i = 0u; i < pref.size(); ++i) { + uint16_t candidate_group = pref[i]; + auto match_it = std::find(supp.begin(), supp.end(), candidate_group); + // Post-quantum key agreements don't fit in the u8-length-prefixed + // ECPoint field in TLS 1.2 and below. + if (ssl_protocol_version(ssl) < TLS1_3_VERSION && + is_post_quantum_group(candidate_group)) { + match_it = supp.end(); + } + if (match_it != supp.end()) { + size_t match_idx = match_it - supp.begin(); + if (!best_match.has_value() || match_idx < *best_match) { + best_match = match_idx; } } + + // If we found a match, stop at the current equipreference group. + bool equal_with_next = + !pref_flags.empty() && + (pref_flags[i] & SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT); + if (!equal_with_next && best_match.has_value()) { + break; + } } - return false; + if (!best_match.has_value()) { + return false; + } + *out_group_id = supp[*best_match]; + return true; } bool tls1_check_group_id(const SSL_HANDSHAKE *hs, uint16_t group_id) {
diff --git a/ssl/handoff.cc b/ssl/handoff.cc index 8dfc5f9..9f56ca5 100644 --- a/ssl/handoff.cc +++ b/ssl/handoff.cc
@@ -18,6 +18,7 @@ #include <openssl/err.h> #include <algorithm> +#include <optional> #include "../crypto/internal.h" #include "internal.h" @@ -189,22 +190,51 @@ } Span<const uint16_t> configured_groups = ssl->s3->hs->config->supported_group_list; + Span<const uint32_t> configured_groups_flags = + ssl->s3->hs->config->supported_group_list_flags; Array<uint16_t> new_configured_groups; - if (!new_configured_groups.InitForOverwrite(configured_groups.size())) { + // Temporary scratch space to keep track of consecutive runs of groups of + // equal preference, so they can be relabeled after filtering for groups + // present in the remote features and copying to the new configuration. + Array<size_t> new_groups_preference_ranks; + Array<uint32_t> new_groups_flags; + if (!new_configured_groups.InitForOverwrite(configured_groups.size()) || + !new_groups_preference_ranks.Init(configured_groups.size()) || + !new_groups_flags.Init(configured_groups.size())) { return false; } - idx = 0; - for (uint16_t configured_group : configured_groups) { - if (std::find(supported_groups.begin(), supported_groups.end(), - configured_group) != supported_groups.end()) { - new_configured_groups[idx++] = configured_group; + // Populate temporary array containing "preference rank" of each group. + for (size_t i = 1; i < configured_groups.size(); ++i) { + new_groups_preference_ranks[i] = new_groups_preference_ranks[i - 1]; + if ((configured_groups_flags[i - 1] & + SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT) == 0) { + ++new_groups_preference_ranks[i]; } } + idx = 0; + std::optional<size_t> last_rank; + for (size_t i = 0u; i < configured_groups.size(); ++i) { + uint16_t configured_group = configured_groups[i]; + if (std::find(supported_groups.begin(), supported_groups.end(), + configured_group) == supported_groups.end()) { + continue; + } + new_configured_groups[idx] = configured_group; + // Set the "equal preference with next" flag on the previous group, if + // appropriate. + if (last_rank.has_value() && new_groups_preference_ranks[i] == *last_rank) { + new_groups_flags[idx - 1] |= SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT; + } + last_rank = new_groups_preference_ranks[i]; + ++idx; + } if (idx == 0) { return false; } new_configured_groups.Shrink(idx); + new_groups_flags.Shrink(idx); ssl->config->supported_group_list = std::move(new_configured_groups); + ssl->config->supported_group_list_flags = std::move(new_groups_flags); CBS alps; CBS_init(&alps, nullptr, 0);
diff --git a/ssl/internal.h b/ssl/internal.h index abb1302..0673f5f 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -3288,6 +3288,10 @@ // a valid subsequence of the supported group list. Array<uint16_t> supported_group_list; + // Contains flags corresponding to `supported_group_list`, which are + // SSL_GROUP_FLAG_* values ORed together. + Array<uint32_t> supported_group_list_flags; + // For a client, this may contain a subsequence of the group IDs in // |suppported_group_list|, which gives the groups for which key shares should // be sent in the client's key_share extension. This is non-nullopt iff @@ -3942,8 +3946,9 @@ // Defined compression algorithms for certificates. bssl::Vector<bssl::CertCompressionAlg> cert_compression_algs; - // Supported group values inherited by SSL structure + // Supported group values and flags inherited by SSL structure bssl::Array<uint16_t> supported_group_list; + bssl::Array<uint32_t> supported_group_list_flags; // channel_id_private is the client's Channel ID private key, or null if // Channel ID should not be offered on this connection.
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index f64b103..3c98a7c 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -457,6 +457,9 @@ if (!ret->supported_group_list.CopyFrom(DefaultSupportedGroupIds())) { return nullptr; } + if (!ret->supported_group_list_flags.Init(ret->supported_group_list.size())) { + return nullptr; + } return ret.release(); } @@ -532,6 +535,8 @@ ssl->config->compliance_policy = ctx->compliance_policy; if (!ssl->config->supported_group_list.CopyFrom(ctx->supported_group_list) || + !ssl->config->supported_group_list_flags.CopyFrom( + ctx->supported_group_list_flags) || !ssl->config->alpn_client_proto_list.CopyFrom( ctx->alpn_client_proto_list) || !ssl->config->verify_sigalgs.CopyFrom(ctx->verify_sigalgs)) { @@ -1875,26 +1880,65 @@ } } +static bool check_group_flags(Span<const uint32_t> flags) { + if (flags.empty()) { + return true; + } + // The last element must not have the "equal preference with next" flag, + // because there is no next element. + return (flags.back() & SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT) == 0; +} + +static bool set_group_ids_and_flags(const uint16_t *group_ids, + const uint32_t *flags, size_t num_group_ids, + Array<uint16_t> *groups_out, + Array<uint32_t> *flags_out) { + Span<const uint16_t> groups_span = num_group_ids == 0 + ? DefaultSupportedGroupIds() + : Span(group_ids, num_group_ids); + if (!check_group_ids(groups_span)) { + return false; + } + // If using default groups, always use default flags. + if (flags == nullptr || num_group_ids == 0) { + return groups_out->CopyFrom(groups_span) && + flags_out->Init(groups_span.size()); + } + Span<const uint32_t> flags_span = Span(flags, num_group_ids); + if (!check_group_flags(flags_span)) { + return false; + } + return groups_out->CopyFrom(groups_span) && flags_out->CopyFrom(flags_span); +} + int SSL_CTX_set1_group_ids(SSL_CTX *ctx, const uint16_t *group_ids, size_t num_group_ids) { - auto span = Span(group_ids, num_group_ids); - if (span.empty()) { - span = DefaultSupportedGroupIds(); - } - return check_group_ids(span) && ctx->supported_group_list.CopyFrom(span); + return SSL_CTX_set1_group_ids_with_flags(ctx, group_ids, /*flags=*/nullptr, + num_group_ids); +} + +int SSL_CTX_set1_group_ids_with_flags(SSL_CTX *ctx, const uint16_t *group_ids, + const uint32_t *flags, + size_t num_group_ids) { + return set_group_ids_and_flags(group_ids, flags, num_group_ids, + &ctx->supported_group_list, + &ctx->supported_group_list_flags); } int SSL_set1_group_ids(SSL *ssl, const uint16_t *group_ids, size_t num_group_ids) { + return SSL_set1_group_ids_with_flags(ssl, group_ids, /*flags=*/nullptr, + num_group_ids); +} + +int SSL_set1_group_ids_with_flags(SSL *ssl, const uint16_t *group_ids, + const uint32_t *flags, size_t num_group_ids) { if (!ssl->config) { return 0; } - auto span = Span(group_ids, num_group_ids); - if (span.empty()) { - span = DefaultSupportedGroupIds(); - } - if (check_group_ids(span) && - ssl->config->supported_group_list.CopyFrom(span)) { + if (set_group_ids_and_flags(group_ids, flags, num_group_ids, + &ssl->config->supported_group_list, + &ssl->config->supported_group_list_flags)) { clear_key_shares_if_invalid(ssl->config.get()); return 1; } @@ -1927,7 +1971,8 @@ int SSL_CTX_set1_groups(SSL_CTX *ctx, const int *groups, size_t num_groups) { return ssl_nids_to_group_ids(&ctx->supported_group_list, - Span(groups, num_groups)); + Span(groups, num_groups)) && + ctx->supported_group_list_flags.Init(ctx->supported_group_list.size()); } int SSL_set1_groups(SSL *ssl, const int *groups, size_t num_groups) { @@ -1935,7 +1980,9 @@ return 0; } if (ssl_nids_to_group_ids(&ssl->config->supported_group_list, - Span(groups, num_groups))) { + Span(groups, num_groups)) && + ssl->config->supported_group_list_flags.Init( + ssl->config->supported_group_list.size())) { clear_key_shares_if_invalid(ssl->config.get()); return 1; } @@ -1983,14 +2030,17 @@ } int SSL_CTX_set1_groups_list(SSL_CTX *ctx, const char *groups) { - return ssl_str_to_group_ids(&ctx->supported_group_list, groups); + return ssl_str_to_group_ids(&ctx->supported_group_list, groups) && + ctx->supported_group_list_flags.Init(ctx->supported_group_list.size()); } int SSL_set1_groups_list(SSL *ssl, const char *groups) { if (!ssl->config) { return 0; } - if (ssl_str_to_group_ids(&ssl->config->supported_group_list, groups)) { + if (ssl_str_to_group_ids(&ssl->config->supported_group_list, groups) && + ssl->config->supported_group_list_flags.Init( + ssl->config->supported_group_list.size())) { clear_key_shares_if_invalid(ssl->config.get()); return 1; }
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 6c2a382..bd5759c 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -2132,6 +2132,101 @@ EXPECT_EQ(0, X509_NAME_cmp(sk_X509_NAME_value(list, 2), name1)); } +TEST(SSLTest, SetGroupIdsWithEqualPreference) { + const struct { + const char *description; + std::vector<uint16_t> groups; + std::vector<uint32_t> flags; + bool expected_success; + } kTests[] = { + { + "Empty groups / default.", + {}, + {}, + true, + }, + { + "Single group.", + {SSL_GROUP_X25519}, + {0}, + true, + }, + { + "Multiple groups with equal preference.", + {SSL_GROUP_X25519_MLKEM768, SSL_GROUP_MLKEM1024}, + {SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT, 0}, + true, + }, + { + "Singleton followed by multiple groups with equal preference.", + {SSL_GROUP_SECP256R1, SSL_GROUP_X25519_MLKEM768, SSL_GROUP_MLKEM1024}, + {0, SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT, 0}, + true, + }, + { + "Multiple groups with equal preference followed by singleton.", + {SSL_GROUP_X25519_MLKEM768, SSL_GROUP_MLKEM1024, SSL_GROUP_SECP256R1}, + {SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT, 0, 0}, + true, + }, + { + "Config error (last group has equal preference flag).", + {SSL_GROUP_X25519_MLKEM768, SSL_GROUP_MLKEM1024}, + {0, SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT}, + false, + }, + }; + + for (const auto &t : kTests) { + SCOPED_TRACE(t.description); + ASSERT_EQ(t.groups.size(), t.flags.size()) << "Test setup error."; + bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); + EXPECT_EQ(t.expected_success, + SSL_CTX_set1_group_ids_with_flags( + ctx.get(), t.groups.data(), t.flags.data(), t.groups.size())); + bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get())); + ASSERT_TRUE(ssl); + EXPECT_EQ(t.expected_success, + SSL_set1_group_ids_with_flags(ssl.get(), t.groups.data(), + t.flags.data(), t.groups.size())); + } +} + +// Test that the SSL group flags are defaulted to zero when zero groups are set +// (i.e. using the default groups). +TEST(SSLTest, SetGroupIdsWithFlags_DefaultGroups) { + const uint16_t kDefaultGroups[] = {SSL_GROUP_X25519, SSL_GROUP_SECP256R1, + SSL_GROUP_SECP384R1}; + const uint32_t kBogusFlags[] = {SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT, + SSL_GROUP_FLAG_EQUAL_PREFERENCE_WITH_NEXT, 0}; + bssl::UniquePtr<SSL_CTX> server_ctx = + CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(server_ctx); + bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(client_ctx); + bssl::UniquePtr<SSL> client, server; + CreateClientAndServer(&client, &server, client_ctx.get(), server_ctx.get()); + + // Should set the default groups, and corresponding default (zero) flags. + EXPECT_TRUE( + SSL_set1_group_ids_with_flags(server.get(), nullptr, kBogusFlags, 0)); + EXPECT_THAT(server->config->supported_group_list, + ElementsAreArray(kDefaultGroups)); + + // Set up and run the handshake to show that the bogus "equal preference with + // next" flags aren't used, and we simply use the default groups with default + // flags configured on the server side. + SSL_set_options(server.get(), SSL_OP_CIPHER_SERVER_PREFERENCE); + const uint16_t kClientGroups[] = {SSL_GROUP_SECP384R1, SSL_GROUP_SECP256R1, + SSL_GROUP_X25519}; + EXPECT_TRUE(SSL_set1_group_ids(client.get(), kClientGroups, + std::size(kClientGroups))); + + ASSERT_TRUE(CompleteHandshakes(client.get(), server.get())); + EXPECT_EQ(SSL_get_group_id(client.get()), SSL_GROUP_X25519); +} + struct ECHConfigParams { uint16_t version = TLSEXT_TYPE_encrypted_client_hello; uint16_t config_id = 1;
diff --git a/ssl/test/runner/curve_tests.go b/ssl/test/runner/curve_tests.go index 8e7b0a4..9bc1311 100644 --- a/ssl/test/runner/curve_tests.go +++ b/ssl/test/runner/curve_tests.go
@@ -34,6 +34,8 @@ const bogusCurve = 0x1234 +const curveEqualPreferenceWithNextFlag = 0x01 + func isPqGroup(r CurveID) bool { return r == CurveX25519Kyber768 || isMLKEMGroup(r) } @@ -762,4 +764,25 @@ }, shimCertificate: &ecdsaP256Certificate, }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "CurveTest-Server-EqualPreference-TLS13", + config: Config{ + MinVersion: VersionTLS13, + MaxVersion: VersionTLS13, + CurvePreferences: []CurveID{CurveX25519, CurveMLKEM1024, CurveX25519MLKEM768}, + }, + flags: append(append( + []string{ + // Set the -cipher flag to force SSL_OP_CIPHER_SERVER_PREFERENCE. + "-cipher", "ALL:3DES", + "-expect-curve-id", strconv.Itoa(int(CurveMLKEM1024))}, + flagCurves("-curves", []CurveID{CurveX25519MLKEM768, CurveMLKEM1024, CurveX25519})...), + flagInts("-curves-flags", []int{curveEqualPreferenceWithNextFlag, 0, 0})..., + ), + expectations: connectionExpectations{ + curveID: CurveMLKEM1024, + }, + }) }
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 1f3c309..01dbea0 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -365,6 +365,7 @@ IntVectorFlag("-expect-peer-verify-pref", &TestConfig::expect_peer_verify_prefs), IntVectorFlag("-curves", &TestConfig::curves), + IntVectorFlag("-curves-flags", &TestConfig::curves_flags), OptionalIntVectorFlag("-key-shares", &TestConfig::key_shares), OptionalDefaultInitFlag("-no-key-shares", &TestConfig::key_shares), IntVectorFlag("-server-supported-groups-hint", @@ -2527,9 +2528,18 @@ if (!check_close_notify) { SSL_set_quiet_shutdown(ssl.get(), 1); } - if (!curves.empty() && - !SSL_set1_group_ids(ssl.get(), curves.data(), curves.size())) { - return nullptr; + if (!curves.empty()) { + if (!curves_flags.empty()) { + if (curves.size() != curves_flags.size() || + !SSL_set1_group_ids_with_flags(ssl.get(), curves.data(), + curves_flags.data(), curves.size())) { + return nullptr; + } + } else { + if (!SSL_set1_group_ids(ssl.get(), curves.data(), curves.size())) { + return nullptr; + } + } } if (key_shares.has_value() && !SSL_set1_client_key_shares(ssl.get(), key_shares->data(),
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 9745de0..440a6a4 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h
@@ -65,6 +65,7 @@ std::vector<uint16_t> verify_prefs; std::vector<uint16_t> expect_peer_verify_prefs; std::vector<uint16_t> curves; + std::vector<uint32_t> curves_flags; std::optional<std::vector<uint16_t>> key_shares; std::vector<uint16_t> server_supported_groups_hint; std::string key_file;