Always populate supported_group_list
SSL_CONFIG and SSL_CTX have a supported_group_list field which stores
the ordered list of group IDs of groups supported for key exchange.
These can be set by the caller via several setters (that either take
group IDs, NIDs, or a string list), but there is a default list that is
used if the caller hasn't configured the supported groups.
Currently the default is evaluated by a getter, tls1_get_grouplist(),
but it is more convenient if the supported_group_list field can be
treated as a source of truth, i.e. if it contains the applicable groups
at all times.
This CL eliminates the getter, and instead populates
supported_group_list with the default groups in SSL_CTX_new.
The behavior of the various setters is preserved. The group ID and NID
based setters now special-case an empty input to restore the default
list. The string list based setter never accepted an empty string, so
no changes are made to that. The public API documentation now mentions
these behaviors explicitly.
Update-Note: There should not be any behavior change. The documentation
is updated to clarify what was existing behavior and is still the
behavior of providing an empty list when setting supported groups.
Change-Id: Iced20f80deb79fbd4b8bd560a00bf395e9b0e86b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81529
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Lily Chen <chlily@google.com>
diff --git a/crypto/mem_internal.h b/crypto/mem_internal.h
index 80d61bb..20b45fd 100644
--- a/crypto/mem_internal.h
+++ b/crypto/mem_internal.h
@@ -86,6 +86,8 @@
template <typename T>
class Array {
public:
+ using value_type = std::remove_cv_t<T>;
+
// Array's default constructor creates an empty array.
Array() {}
Array(const Array &) = delete;
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index cef9884..79eb0c7 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2532,14 +2532,16 @@
// SSL_CTX_set1_group_ids sets the preferred groups for |ctx| to |group_ids|.
// Each element of |group_ids| should be a unique one of the |SSL_GROUP_*|
-// constants. It returns one on success and zero on failure.
+// constants. If |group_ids| is empty, a default list will be set instead. It
+// returns one on success and zero on failure.
OPENSSL_EXPORT int SSL_CTX_set1_group_ids(SSL_CTX *ctx,
const uint16_t *group_ids,
size_t num_group_ids);
// SSL_set1_group_ids sets the preferred groups for |ssl| to |group_ids|. Each
// element of |group_ids| should be a unique one of the |SSL_GROUP_*| constants.
-// It returns one on success and zero on failure.
+// If |group_ids| is empty, a default list will be set instead. It returns one
+// on success and zero on failure.
OPENSSL_EXPORT int SSL_set1_group_ids(SSL *ssl, const uint16_t *group_ids,
size_t num_group_ids);
@@ -2573,27 +2575,29 @@
// library.
// SSL_CTX_set1_groups sets the preferred groups for |ctx| to be |groups|. Each
-// element of |groups| should be a unique |NID_*| constant from nid.h. It
-// returns one on success and zero on failure.
+// element of |groups| should be a unique |NID_*| constant from nid.h. If
+// |groups| is empty, a default list will be set instead. It returns one on
+// success and zero on failure.
OPENSSL_EXPORT int SSL_CTX_set1_groups(SSL_CTX *ctx, const int *groups,
size_t num_groups);
// SSL_set1_groups sets the preferred groups for |ssl| to be |groups|. Each
-// element of |groups| should be a unique |NID_*| constant from nid.h. It
-// returns one on success and zero on failure.
+// element of |groups| should be a unique |NID_*| constant from nid.h. If
+// |groups| is empty, a default list will be set instead. It returns one on
+// success and zero on failure.
OPENSSL_EXPORT int SSL_set1_groups(SSL *ssl, const int *groups,
size_t num_groups);
-// SSL_CTX_set1_groups_list decodes |groups| as a colon-separated list of group
-// names (e.g. "X25519" or "P-256") and sets |ctx|'s preferred groups to the
-// result. The list must not contain duplicates. It returns one on success and
-// zero on failure.
+// SSL_CTX_set1_groups_list decodes |groups| as a non-empty colon-separated list
+// of group names (e.g. "X25519" or "P-256") and sets |ctx|'s preferred groups
+// to the result. The list must not contain duplicates. It returns one on
+// success and zero on failure.
OPENSSL_EXPORT int SSL_CTX_set1_groups_list(SSL_CTX *ctx, const char *groups);
-// SSL_set1_groups_list decodes |groups| as a colon-separated list of group
-// names (e.g. "X25519" or "P-256") and sets |ssl|'s preferred groups to the
-// result. The list must not contain duplicates. It returns one on success and
-// zero on failure.
+// SSL_set1_groups_list decodes |groups| as a non-empty colon-separated list of
+// group names (e.g. "X25519" or "P-256") and sets |ssl|'s preferred groups to
+// the result. The list must not contain duplicates. It returns one on success
+// and zero on failure.
OPENSSL_EXPORT int SSL_set1_groups_list(SSL *ssl, const char *groups);
// SSL_get_negotiated_group returns the NID of the group used by |ssl|'s most
diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index 7be8a3c..dcdd5f0 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -200,19 +200,6 @@
return false;
}
-static const uint16_t kDefaultGroups[] = {
- SSL_GROUP_X25519,
- SSL_GROUP_SECP256R1,
- SSL_GROUP_SECP384R1,
-};
-
-Span<const uint16_t> tls1_get_grouplist(const SSL_HANDSHAKE *hs) {
- if (!hs->config->supported_group_list.empty()) {
- return hs->config->supported_group_list;
- }
- return Span<const uint16_t>(kDefaultGroups);
-}
-
bool tls1_get_shared_group(SSL_HANDSHAKE *hs, uint16_t *out_group_id) {
SSL *const ssl = hs->ssl;
assert(ssl->server);
@@ -226,7 +213,7 @@
// support our favoured group. Thus we do not special-case an emtpy
// |peer_supported_group_list|.
- Span<const uint16_t> groups = tls1_get_grouplist(hs);
+ Span<const uint16_t> groups = hs->config->supported_group_list;
Span<const uint16_t> pref, supp;
if (ssl->options & SSL_OP_CIPHER_SERVER_PREFERENCE) {
pref = groups;
@@ -264,7 +251,7 @@
return false;
}
- for (uint16_t supported : tls1_get_grouplist(hs)) {
+ for (uint16_t supported : hs->config->supported_group_list) {
if (supported == group_id) {
return true;
}
@@ -2229,7 +2216,7 @@
uint16_t second_group_id = 0;
if (override_group_id == 0) {
// Predict the most preferred group.
- Span<const uint16_t> groups = tls1_get_grouplist(hs);
+ Span<const uint16_t> groups = hs->config->supported_group_list;
if (groups.empty()) {
OPENSSL_PUT_ERROR(SSL, SSL_R_NO_GROUPS_SPECIFIED);
return false;
@@ -2521,7 +2508,7 @@
return false;
}
- for (uint16_t group : tls1_get_grouplist(hs)) {
+ for (uint16_t group : hs->config->supported_group_list) {
if (is_post_quantum_group(group) && hs->max_version < TLS1_3_VERSION) {
continue;
}
diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index a24d741..4c4ad35 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -17,6 +17,8 @@
#include <openssl/bytestring.h>
#include <openssl/err.h>
+#include <algorithm>
+
#include "../crypto/internal.h"
#include "internal.h"
@@ -186,21 +188,15 @@
supported_groups[idx++] = group;
}
Span<const uint16_t> configured_groups =
- tls1_get_grouplist(ssl->s3->hs.get());
+ ssl->s3->hs->config->supported_group_list;
Array<uint16_t> new_configured_groups;
if (!new_configured_groups.InitForOverwrite(configured_groups.size())) {
return false;
}
idx = 0;
for (uint16_t configured_group : configured_groups) {
- bool ok = false;
- for (uint16_t supported_group : supported_groups) {
- if (supported_group == configured_group) {
- ok = true;
- break;
- }
- }
- if (ok) {
+ if (std::find(supported_groups.begin(), supported_groups.end(),
+ configured_group) != supported_groups.end()) {
new_configured_groups[idx++] = configured_group;
}
}
diff --git a/ssl/internal.h b/ssl/internal.h
index 18e2494..9cff133 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -954,6 +954,10 @@
// NamedGroups returns all supported groups.
Span<const NamedGroup> NamedGroups();
+// DefaultSupportedGroupIds returns the list of IDs for the default groups that
+// are supported when the caller hasn't explicitly configured supported groups.
+Span<const uint16_t> DefaultSupportedGroupIds();
+
// ssl_nid_to_group_id looks up the group corresponding to |nid|. On success, it
// sets |*out_group_id| to the group ID and returns true. Otherwise, it returns
// false.
@@ -3589,9 +3593,6 @@
bool tls1_generate_master_secret(SSL_HANDSHAKE *hs, Span<uint8_t> out,
Span<const uint8_t> premaster);
-// tls1_get_grouplist returns the locally-configured group preference list.
-Span<const uint16_t> tls1_get_grouplist(const SSL_HANDSHAKE *ssl);
-
// tls1_check_group_id returns whether |group_id| is consistent with locally-
// configured group preferences.
bool tls1_check_group_id(const SSL_HANDSHAKE *ssl, uint16_t group_id);
diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc
index 411a6ab..0014d0c 100644
--- a/ssl/ssl_key_share.cc
+++ b/ssl/ssl_key_share.cc
@@ -449,6 +449,15 @@
Span<const NamedGroup> NamedGroups() { return kNamedGroups; }
+Span<const uint16_t> DefaultSupportedGroupIds() {
+ static const uint16_t kDefaultSupportedGroupIds[] = {
+ SSL_GROUP_X25519,
+ SSL_GROUP_SECP256R1,
+ SSL_GROUP_SECP384R1,
+ };
+ return Span(kDefaultSupportedGroupIds);
+}
+
UniquePtr<SSLKeyShare> SSLKeyShare::Create(uint16_t group_id) {
switch (group_id) {
case SSL_GROUP_SECP256R1:
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 3188317..2e0db35 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -454,6 +454,10 @@
return nullptr;
}
+ if (!ret->supported_group_list.CopyFrom(DefaultSupportedGroupIds())) {
+ return nullptr;
+ }
+
return ret.release();
}
@@ -1843,6 +1847,9 @@
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);
}
@@ -1852,12 +1859,18 @@
return 0;
}
auto span = Span(group_ids, num_group_ids);
+ if (span.empty()) {
+ span = DefaultSupportedGroupIds();
+ }
return check_group_ids(span) &&
ssl->config->supported_group_list.CopyFrom(span);
}
static bool ssl_nids_to_group_ids(Array<uint16_t> *out_group_ids,
Span<const int> nids) {
+ if (nids.empty()) {
+ return out_group_ids->CopyFrom(DefaultSupportedGroupIds());
+ }
Array<uint16_t> group_ids;
if (!group_ids.InitForOverwrite(nids.size())) {
return false;
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 84385fb..96edb61 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -61,7 +61,9 @@
using testing::ElementsAre;
+using testing::ElementsAreArray;
using testing::Key;
+using testing::Not;
BSSL_NAMESPACE_BEGIN
@@ -665,6 +667,61 @@
}
}
+TEST(SSLTest, DefaultCurves) {
+ const uint16_t kDefaults[] = {SSL_GROUP_X25519, SSL_GROUP_SECP256R1,
+ SSL_GROUP_SECP384R1};
+
+ // Test the group ID APIs.
+ {
+ bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+ ASSERT_TRUE(ctx);
+ // The new context is populated with the default group list.
+ EXPECT_THAT(ctx->supported_group_list, ElementsAreArray(kDefaults));
+
+ // Set some other list to check that it is set away from the default.
+ const uint16_t kArbitraryGroupIds[] = {SSL_GROUP_X25519};
+ ASSERT_TRUE(SSL_CTX_set1_group_ids(ctx.get(), kArbitraryGroupIds,
+ std::size(kArbitraryGroupIds)));
+ EXPECT_THAT(ctx->supported_group_list, Not(ElementsAreArray(kDefaults)));
+
+ bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get()));
+ ASSERT_TRUE(ssl);
+ EXPECT_THAT(ssl->config->supported_group_list,
+ Not(ElementsAreArray(kDefaults)));
+
+ // Setting an empty list restores the defaults.
+ ASSERT_TRUE(SSL_set1_group_ids(ssl.get(), nullptr, 0));
+ EXPECT_THAT(ssl->config->supported_group_list, ElementsAreArray(kDefaults));
+ ASSERT_TRUE(SSL_CTX_set1_group_ids(ctx.get(), nullptr, 0));
+ EXPECT_THAT(ctx->supported_group_list, ElementsAreArray(kDefaults));
+ }
+
+ // Test the NID APIs.
+ {
+ bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+ ASSERT_TRUE(ctx);
+ // The new context is populated with the default group list.
+ EXPECT_THAT(ctx->supported_group_list, ElementsAreArray(kDefaults));
+
+ // Set some other list to check that it is set away from the default.
+ const int kArbitraryNids[] = {NID_X9_62_prime256v1};
+ ASSERT_TRUE(SSL_CTX_set1_groups(ctx.get(), kArbitraryNids,
+ std::size(kArbitraryNids)));
+ EXPECT_THAT(ctx->supported_group_list, Not(ElementsAreArray(kDefaults)));
+
+ bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get()));
+ ASSERT_TRUE(ssl);
+ EXPECT_THAT(ssl->config->supported_group_list,
+ Not(ElementsAreArray(kDefaults)));
+
+ // Setting an empty list restores the defaults.
+ ASSERT_TRUE(SSL_set1_groups(ssl.get(), nullptr, 0));
+ EXPECT_THAT(ssl->config->supported_group_list, ElementsAreArray(kDefaults));
+ ASSERT_TRUE(SSL_CTX_set1_groups(ctx.get(), nullptr, 0));
+ EXPECT_THAT(ctx->supported_group_list, ElementsAreArray(kDefaults));
+ }
+}
+
// kOpenSSLSession is a serialized SSL_SESSION.
static const char kOpenSSLSession[] =
"MIIFqgIBAQICAwMEAsAvBCAG5Q1ndq4Yfmbeo1zwLkNRKmCXGdNgWvGT3cskV0yQ"
@@ -6482,10 +6539,13 @@
0x02, 0x00, 0x17,
};
- // The zero length means that the default list of groups is used.
- EXPECT_EQ(0u, server->config->supported_group_list.size());
+ // The default list of groups is used before applying the handoff.
+ EXPECT_THAT(server->config->supported_group_list,
+ ElementsAreArray({SSL_GROUP_X25519, SSL_GROUP_SECP256R1,
+ SSL_GROUP_SECP384R1}));
ASSERT_TRUE(SSL_apply_handoff(server.get(), handoff));
EXPECT_EQ(1u, server->config->supported_group_list.size());
+ EXPECT_EQ(SSL_GROUP_SECP256R1, server->config->supported_group_list[0]);
}
TEST(SSLTest, ZeroSizedWiteFlushesHandshakeMessages) {
