Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions include/openssl/ssl.h
Original file line number Diff line number Diff line change
Expand Up @@ -2702,6 +2702,19 @@ OPENSSL_EXPORT int SSL_CTX_set1_groups(SSL_CTX *ctx, const int *groups,
OPENSSL_EXPORT int SSL_set1_groups(SSL *ssl, const int *groups,
size_t num_groups);

// SSL_CTX_set1_group_ids sets the preferred groups for |ctx| to be
// |group_ids|. Each element of |group_ids| should be a |SSL_GROUP_*| value. 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 be |group_ids|.
// Each element of |group_ids| should be a |SSL_GROUP_*| value. 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);

// SSL_CTX_set1_groups_list sets the preferred groups for |ctx| to be the
// colon-separated list |groups|. Each element of |groups| should be a curve
// name (e.g. P-256, X25519, ...). It returns one on success and zero on
Expand Down
4 changes: 4 additions & 0 deletions ssl/handshake_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,10 @@ static enum ssl_hs_wait_t do_read_client_hello_after_ech(SSL_HANDSHAKE *hs) {
return ssl_hs_error;

default:
// The callback may have left stale errors on the error queue (e.g.
// from PEM parsing). Clear them so that SSL_get_error does not
// misinterpret them as handshake failures.
ERR_clear_error();
/* fallthrough */;
}
}
Expand Down
2 changes: 2 additions & 0 deletions ssl/libssl.map
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ AWS_LC_1.0 {
SSL_CTX_set1_curves;
SSL_CTX_set1_curves_list;
SSL_CTX_set1_ech_keys;
SSL_CTX_set1_group_ids;
SSL_CTX_set1_groups;
SSL_CTX_set1_groups_list;
SSL_CTX_set1_param;
Expand Down Expand Up @@ -490,6 +491,7 @@ AWS_LC_1.0 {
SSL_set1_curves_list;
SSL_set1_delegated_credential;
SSL_set1_ech_config_list;
SSL_set1_group_ids;
SSL_set1_groups;
SSL_set1_groups_list;
SSL_set1_host;
Expand Down
2 changes: 2 additions & 0 deletions ssl/libssl.txt
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ SSL_CTX_set1_chain AWS_LC_1.0 PUBLIC
SSL_CTX_set1_curves AWS_LC_1.0 PUBLIC
SSL_CTX_set1_curves_list AWS_LC_1.0 PUBLIC
SSL_CTX_set1_ech_keys AWS_LC_1.0 PUBLIC
SSL_CTX_set1_group_ids AWS_LC_1.0 PUBLIC
SSL_CTX_set1_groups AWS_LC_1.0 PUBLIC
SSL_CTX_set1_groups_list AWS_LC_1.0 PUBLIC
SSL_CTX_set1_param AWS_LC_1.0 PUBLIC
Expand Down Expand Up @@ -483,6 +484,7 @@ SSL_set1_curves AWS_LC_1.0 PUBLIC
SSL_set1_curves_list AWS_LC_1.0 PUBLIC
SSL_set1_delegated_credential AWS_LC_1.0 PUBLIC
SSL_set1_ech_config_list AWS_LC_1.0 PUBLIC
SSL_set1_group_ids AWS_LC_1.0 PUBLIC
SSL_set1_groups AWS_LC_1.0 PUBLIC
SSL_set1_groups_list AWS_LC_1.0 PUBLIC
SSL_set1_host AWS_LC_1.0 PUBLIC
Expand Down
25 changes: 25 additions & 0 deletions ssl/ssl_lib.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2028,6 +2028,31 @@ int SSL_set1_groups(SSL *ssl, const int *groups, size_t num_groups) {
MakeConstSpan(groups, num_groups));
}

static bool ssl_check_group_ids(Array<uint16_t> *out_group_ids,
Span<const uint16_t> group_ids) {
for (uint16_t group_id : group_ids) {
if (ssl_group_id_to_nid(group_id) == NID_undef) {
Comment on lines +2031 to +2034

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: BoringSSL also rejects duplicate group IDs here (check_no_duplicates). Duplicates would just get advertised twice in supported_groups, so probably harmless, but worth a decision.

return false;
}
Comment on lines +2034 to +2036

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BoringSSL's equivalent (check_group_ids) pushes SSL_R_UNSUPPORTED_ELLIPTIC_CURVE when it hits an unknown ID.

Suggest matching upstream:

Suggested change
if (ssl_group_id_to_nid(group_id) == NID_undef) {
return false;
}
if (ssl_group_id_to_nid(group_id) == NID_undef) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_ELLIPTIC_CURVE);
return false;
}

}
return out_group_ids->CopyFrom(group_ids);
}

int SSL_CTX_set1_group_ids(SSL_CTX *ctx, const uint16_t *group_ids,
size_t num_group_ids) {
return ssl_check_group_ids(&ctx->supported_group_list,
MakeConstSpan(group_ids, num_group_ids));
}

int SSL_set1_group_ids(SSL *ssl, const uint16_t *group_ids,
size_t num_group_ids) {
if (!ssl->config) {
return 0;
}
return ssl_check_group_ids(&ssl->config->supported_group_list,
MakeConstSpan(group_ids, num_group_ids));
}

static bool ssl_str_to_group_ids(Array<uint16_t> *out_group_ids,
const char *str) {
// Count the number of groups in the list.
Expand Down
45 changes: 45 additions & 0 deletions ssl/ssl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -832,6 +832,51 @@ TEST(SSLTest, EarlyCallbackVersionSwitch) {
EXPECT_EQ(TLS1_2_VERSION, SSL_version(client.get()));
}

// Test that the handshake succeeds when the early callback leaves stale errors
// on the error queue
TEST(SSLTest, EarlyCallbackStaleErrorOnQueue) {
bssl::UniquePtr<SSL_CTX> server_ctx =
CreateContextWithTestCertificate(TLS_method());
bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
ASSERT_TRUE(server_ctx);
ASSERT_TRUE(client_ctx);

SSL_CTX_set_select_certificate_cb(
server_ctx.get(),
[](const SSL_CLIENT_HELLO *) -> ssl_select_cert_result_t {
// Simulate a PEM parsing loop leaving a stale error on the queue.
OPENSSL_PUT_ERROR(PEM, PEM_R_NO_START_LINE);
return ssl_select_cert_success;
});

bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
server_ctx.get()));
}

TEST(SSLTest, Set1GroupIds) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of small things:

  • OPENSSL_ARRAY_SIZE(valid_groups) instead of the literal 3/1 counts.
  • This never verifies the setting actually takes effect. Cheap addition: restrict the server to one group, handshake, and check SSL_get_group_id.
  • Might be worth pinning down num_group_ids == 0: BoringSSL explicitly resets to the default list, while we store an empty list that tls1_get_grouplist treats as "use defaults" -- functionally the same, but a test would keep it that way.

bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
ASSERT_TRUE(ctx);
bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get()));
ASSERT_TRUE(ssl);

// Valid group IDs should succeed.
const uint16_t valid_groups[] = {SSL_GROUP_X25519, SSL_GROUP_SECP256R1,
SSL_GROUP_SECP384R1};
EXPECT_TRUE(SSL_CTX_set1_group_ids(ctx.get(), valid_groups, 3));
EXPECT_TRUE(SSL_set1_group_ids(ssl.get(), valid_groups, 3));

// A single valid group ID should succeed.
const uint16_t one_group[] = {SSL_GROUP_X25519};
EXPECT_TRUE(SSL_CTX_set1_group_ids(ctx.get(), one_group, 1));
EXPECT_TRUE(SSL_set1_group_ids(ssl.get(), one_group, 1));

// Invalid group IDs should fail.
const uint16_t invalid_groups[] = {0xFFFF};
EXPECT_FALSE(SSL_CTX_set1_group_ids(ctx.get(), invalid_groups, 1));
EXPECT_FALSE(SSL_set1_group_ids(ssl.get(), invalid_groups, 1));
}

TEST(SSLTest, SetVersion) {
bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
ASSERT_TRUE(ctx);
Expand Down
Loading