Skip to content

Commit

Permalink
Align on using the "group" over "curve" for ECDH in TLS
Browse files Browse the repository at this point in the history 
We're this awkward mix of "group" and "curve" right now. On the spec
side, this is because they used to be "curves", but then RFC 7919
renamed to "group" in an attempt to generalize FFDH and ECDH. The
negotiated FFDH stuff never really went anywhere (the way it used cipher
suite values in TLS 1.2 made it unusable), but the name change stuck.

In our implementation and API, we originally called it "curve". In
preparation for TLS 1.3, we renamed the internals to "group" to match
the spec in
https://boringssl-review.googlesource.com/c/boringssl/+/7955, but the
public API was still "curve".

Then we exported a few more things in
https://boringssl-review.googlesource.com/c/boringssl/+/8565, but I left
it at "curve" to keep the public API self-consistent.

Then we added OpenSSL's new "group" APIs in
https://boringssl-review.googlesource.com/c/boringssl/+/54306, but
didn't go as far to deprecate the old ones yet.

Now I'd like to add new APIs to clear up the weird mix of TLS codepoints
and NIDs that appear in our APIs. But our naming is a mess, so either
choice of "group" or "curve" for the new API looks weird.

In hindsight, we probably should have left it at "curve". Both terms are
equally useless for the future post-quantum KEMs, but at least "curve"
is more unique of a name than "group". But at this point, I think we're
too far through the "group" rename to really backtrack:

- Chromium says "group" in its internals
- QUICHE says "group" in its internals and public API
- Our internals say "group"
- OpenSSL has switched to "group" and deprecated "curve", so new APIs
  will be based on "group"

So align with all this and say "group". This CL handles set1_curves and
set1_curves_list APIs, which already have "group" replacements from
OpenSSL. A follow-up CL will handle our APIs. This is a soft deprecation
because I don't think updating things is particularly worth the churn,
but get the old names out of the way, so new code can have a simpler API
to target.

Also rewrite the documentation for that section accordingly. I don't
think we need to talk about how it's always enabled now. That's a
reference to some very, very old OpenSSL behavior where ECDH negotiation
needed to be separately enabled.

Change-Id: I7a356793d36419fc668364c912ca7b4f5c6c23a2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60206
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
  • Loading branch information
@davidben
davidben authored and Boringssl LUCI CQ committed May 31, 2023
1 parent 4631ccc commit 2da5ba9
Show file tree
Hide file tree
Showing 7 changed files with 137 additions and 160 deletions.
12 changes: 6 additions & 6 deletions fuzz/ssl_ctx_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,18 +410,18 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *buf, size_t len) {
SSL_CTX_set_tlsext_ticket_keys(ctx, keys.data(), keys.size());
},
[](SSL_CTX *ctx, CBS *cbs) {
std::vector<int> curves;
if (!GetVector(&curves, cbs)) {
std::vector<int> groups;
if (!GetVector(&groups, cbs)) {
return;
}
SSL_CTX_set1_curves(ctx, curves.data(), curves.size());
SSL_CTX_set1_groups(ctx, groups.data(), groups.size());
},
[](SSL_CTX *ctx, CBS *cbs) {
std::string curves;
if (!GetString(&curves, cbs)) {
std::string groups;
if (!GetString(&groups, cbs)) {
return;
}
SSL_CTX_set1_curves_list(ctx, curves.c_str());
SSL_CTX_set1_groups_list(ctx, groups.c_str());
},
[](SSL_CTX *ctx, CBS *cbs) {
SSL_CTX_enable_signed_cert_timestamps(ctx);
Expand Down
102 changes: 52 additions & 50 deletions include/openssl/ssl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2331,45 +2331,51 @@ OPENSSL_EXPORT int SSL_CTX_set_num_tickets(SSL_CTX *ctx, size_t num_tickets);
OPENSSL_EXPORT size_t SSL_CTX_get_num_tickets(const SSL_CTX *ctx);


// Elliptic curve Diffie-Hellman.
//
// Cipher suites using an ECDHE key exchange perform Diffie-Hellman over an
// elliptic curve negotiated by both endpoints. See RFC 4492. Only named curves
// are supported. ECDHE is always enabled, but the curve preferences may be
// configured with these functions.
//
// Note that TLS 1.3 renames these from curves to groups. For consistency, we
// currently use the TLS 1.2 name in the API.

// SSL_CTX_set1_curves sets the preferred curves for |ctx| to be |curves|. Each
// element of |curves| should be a curve nid. It returns one on success and
// zero on failure.
// Diffie-Hellman groups and ephemeral key exchanges.
//
// Most TLS handshakes (ECDHE cipher suites in TLS 1.2, and all supported TLS
// 1.3 modes) incorporate an ephemeral key exchange, most commonly using
// Elliptic Curve Diffie-Hellman (ECDH), as described in RFC 8422. The key
// exchange algorithm is negotiated separately from the cipher suite, using
// NamedGroup values, which define Diffie-Hellman groups.
//
// Historically, these values were known as "curves", in reference to ECDH, and
// some APIs refer to the original name. RFC 7919 renamed them to "groups" in
// reference to Diffie-Hellman in general. These values are also used to select
// experimental post-quantum KEMs. Though not Diffie-Hellman groups, KEMs can
// fill a similar role in TLS, so they use the same codepoints.
//
// In TLS 1.2, the ECDH values also negotiate elliptic curves used in ECDSA. In
// TLS 1.3 and later, ECDSA curves are part of the signature algorithm. See
// |SSL_SIGN_*|.

// SSL_CTX_set1_groups sets the preferred groups for |ctx| to be |groups|. Each
// element of |groups| should be a |NID_*| constant from nid.h. It returns one
// on success and zero on failure.
//
// Note that this API uses nid values from nid.h and not the |SSL_CURVE_*|
// values defined below.
OPENSSL_EXPORT int SSL_CTX_set1_curves(SSL_CTX *ctx, const int *curves,
size_t curves_len);
// Note that this API does not use the |SSL_CURVE_*| values defined below.
OPENSSL_EXPORT int SSL_CTX_set1_groups(SSL_CTX *ctx, const int *groups,
size_t num_groups);

// SSL_set1_curves sets the preferred curves for |ssl| to be |curves|. Each
// element of |curves| should be a curve nid. It returns one on success and
// zero on failure.
// SSL_set1_groups sets the preferred groups for |ssl| to be |groups|. Each
// element of |groups| should be a |NID_*| constant from nid.h. It returns one
// on success and zero on failure.
//
// Note that this API uses nid values from nid.h and not the |SSL_CURVE_*|
// values defined below.
OPENSSL_EXPORT int SSL_set1_curves(SSL *ssl, const int *curves,
size_t curves_len);
// Note that this API does not use the |SSL_CURVE_*| values defined below.
OPENSSL_EXPORT int SSL_set1_groups(SSL *ssl, const int *groups,
size_t num_groups);

// SSL_CTX_set1_curves_list sets the preferred curves for |ctx| to be the
// colon-separated list |curves|. Each element of |curves| should be a curve
// 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
// failure.
OPENSSL_EXPORT int SSL_CTX_set1_curves_list(SSL_CTX *ctx, const char *curves);
OPENSSL_EXPORT int SSL_CTX_set1_groups_list(SSL_CTX *ctx, const char *groups);

// SSL_set1_curves_list sets the preferred curves for |ssl| to be the
// colon-separated list |curves|. Each element of |curves| should be a curve
// SSL_set1_groups_list sets the preferred groups for |ssl| 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
// failure.
OPENSSL_EXPORT int SSL_set1_curves_list(SSL *ssl, const char *curves);
OPENSSL_EXPORT int SSL_set1_groups_list(SSL *ssl, const char *groups);

// SSL_CURVE_* define TLS curve IDs.
#define SSL_CURVE_SECP224R1 21
Expand Down Expand Up @@ -2404,20 +2410,6 @@ OPENSSL_EXPORT const char *SSL_get_curve_name(uint16_t curve_id);
// list, so this does not apply if, say, sending strings across services.
OPENSSL_EXPORT size_t SSL_get_all_curve_names(const char **out, size_t max_out);

// SSL_CTX_set1_groups calls |SSL_CTX_set1_curves|.
OPENSSL_EXPORT int SSL_CTX_set1_groups(SSL_CTX *ctx, const int *groups,
size_t groups_len);

// SSL_set1_groups calls |SSL_set1_curves|.
OPENSSL_EXPORT int SSL_set1_groups(SSL *ssl, const int *groups,
size_t groups_len);

// SSL_CTX_set1_groups_list calls |SSL_CTX_set1_curves_list|.
OPENSSL_EXPORT int SSL_CTX_set1_groups_list(SSL_CTX *ctx, const char *groups);

// SSL_set1_groups_list calls |SSL_set1_curves_list|.
OPENSSL_EXPORT int SSL_set1_groups_list(SSL *ssl, const char *groups);


// Certificate verification.
//
Expand Down Expand Up @@ -5090,12 +5082,12 @@ OPENSSL_EXPORT int SSL_state(const SSL *ssl);
// Use |SSL_CTX_set_quiet_shutdown| instead.
OPENSSL_EXPORT void SSL_set_shutdown(SSL *ssl, int mode);

// SSL_CTX_set_tmp_ecdh calls |SSL_CTX_set1_curves| with a one-element list
// containing |ec_key|'s curve.
// SSL_CTX_set_tmp_ecdh calls |SSL_CTX_set1_groups| with a one-element list
// containing |ec_key|'s curve. The remainder of |ec_key| is ignored.
OPENSSL_EXPORT int SSL_CTX_set_tmp_ecdh(SSL_CTX *ctx, const EC_KEY *ec_key);

// SSL_set_tmp_ecdh calls |SSL_set1_curves| with a one-element list containing
// |ec_key|'s curve.
// SSL_set_tmp_ecdh calls |SSL_set1_groups| with a one-element list containing
// |ec_key|'s curve. The remainder of |ec_key| is ignored.
OPENSSL_EXPORT int SSL_set_tmp_ecdh(SSL *ssl, const EC_KEY *ec_key);

// SSL_add_dir_cert_subjects_to_stack lists files in directory |dir|. It calls
Expand Down Expand Up @@ -5244,6 +5236,14 @@ OPENSSL_EXPORT int SSL_CTX_set_tlsext_status_arg(SSL_CTX *ctx, void *arg);
SSL_R_TLSV1_ALERT_BAD_CERTIFICATE_HASH_VALUE
#define SSL_R_TLSV1_CERTIFICATE_REQUIRED SSL_R_TLSV1_ALERT_CERTIFICATE_REQUIRED

// The following symbols are compatibility aliases for equivalent functions that
// use the newer "group" terminology. New code should use the new functions for
// consistency, but we do not plan to remove these aliases.
#define SSL_CTX_set1_curves SSL_CTX_set1_groups
#define SSL_set1_curves SSL_set1_groups
#define SSL_CTX_set1_curves_list SSL_CTX_set1_groups_list
#define SSL_set1_curves_list SSL_set1_groups_list


// Compliance policy configurations
//
Expand Down Expand Up @@ -5359,6 +5359,8 @@ OPENSSL_EXPORT int SSL_set_compliance_policy(
#define SSL_CTRL_SESS_NUMBER doesnt_exist
#define SSL_CTRL_SET_CURVES doesnt_exist
#define SSL_CTRL_SET_CURVES_LIST doesnt_exist
#define SSL_CTRL_SET_GROUPS doesnt_exist
#define SSL_CTRL_SET_GROUPS_LIST doesnt_exist
#define SSL_CTRL_SET_ECDH_AUTO doesnt_exist
#define SSL_CTRL_SET_MAX_CERT_LIST doesnt_exist
#define SSL_CTRL_SET_MAX_SEND_FRAGMENT doesnt_exist
Expand Down Expand Up @@ -5407,7 +5409,7 @@ OPENSSL_EXPORT int SSL_set_compliance_policy(
#define SSL_CTX_sess_set_cache_size SSL_CTX_sess_set_cache_size
#define SSL_CTX_set0_chain SSL_CTX_set0_chain
#define SSL_CTX_set1_chain SSL_CTX_set1_chain
#define SSL_CTX_set1_curves SSL_CTX_set1_curves
#define SSL_CTX_set1_groups SSL_CTX_set1_groups
#define SSL_CTX_set_max_cert_list SSL_CTX_set_max_cert_list
#define SSL_CTX_set_max_send_fragment SSL_CTX_set_max_send_fragment
#define SSL_CTX_set_mode SSL_CTX_set_mode
Expand Down Expand Up @@ -5440,7 +5442,7 @@ OPENSSL_EXPORT int SSL_set_compliance_policy(
#define SSL_session_reused SSL_session_reused
#define SSL_set0_chain SSL_set0_chain
#define SSL_set1_chain SSL_set1_chain
#define SSL_set1_curves SSL_set1_curves
#define SSL_set1_groups SSL_set1_groups
#define SSL_set_max_cert_list SSL_set_max_cert_list
#define SSL_set_max_send_fragment SSL_set_max_send_fragment
#define SSL_set_mode SSL_set_mode
Expand Down
51 changes: 0 additions & 51 deletions ssl/extensions.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,57 +358,6 @@ bool tls1_get_shared_group(SSL_HANDSHAKE *hs, uint16_t *out_group_id) {
return false;
}

bool tls1_set_curves(Array<uint16_t> *out_group_ids, Span<const int> curves) {
Array<uint16_t> group_ids;
if (!group_ids.Init(curves.size())) {
return false;
}

for (size_t i = 0; i < curves.size(); i++) {
if (!ssl_nid_to_group_id(&group_ids[i], curves[i])) {
return false;
}
}

*out_group_ids = std::move(group_ids);
return true;
}

bool tls1_set_curves_list(Array<uint16_t> *out_group_ids, const char *curves) {
// Count the number of curves in the list.
size_t count = 0;
const char *ptr = curves, *col;
do {
col = strchr(ptr, ':');
count++;
if (col) {
ptr = col + 1;
}
} while (col);

Array<uint16_t> group_ids;
if (!group_ids.Init(count)) {
return false;
}

size_t i = 0;
ptr = curves;
do {
col = strchr(ptr, ':');
if (!ssl_name_to_group_id(&group_ids[i++], ptr,
col ? (size_t)(col - ptr) : strlen(ptr))) {
return false;
}
if (col) {
ptr = col + 1;
}
} while (col);

assert(i == count);
*out_group_ids = std::move(group_ids);
return true;
}

bool tls1_check_group_id(const SSL_HANDSHAKE *hs, uint16_t group_id) {
if (is_post_quantum_group(group_id) &&
ssl_protocol_version(hs->ssl) < TLS1_3_VERSION) {
Expand Down
11 changes: 0 additions & 11 deletions ssl/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3353,17 +3353,6 @@ bool tls1_check_group_id(const SSL_HANDSHAKE *ssl, uint16_t group_id);
// found, it returns false.
bool tls1_get_shared_group(SSL_HANDSHAKE *hs, uint16_t *out_group_id);

// tls1_set_curves converts the array of NIDs in |curves| into a newly allocated
// array of TLS group IDs. On success, the function returns true and writes the
// array to |*out_group_ids|. Otherwise, it returns false.
bool tls1_set_curves(Array<uint16_t> *out_group_ids, Span<const int> curves);

// tls1_set_curves_list converts the string of curves pointed to by |curves|
// into a newly allocated array of TLS group IDs. On success, the function
// returns true and writes the array to |*out_group_ids|. Otherwise, it returns
// false.
bool tls1_set_curves_list(Array<uint16_t> *out_group_ids, const char *curves);

// ssl_add_clienthello_tlsext writes ClientHello extensions to |out| for |type|.
// It returns true on success and false on failure. The |header_len| argument is
// the length of the ClientHello written so far and is used to compute the
Expand Down

0 comments on commit 2da5ba9

Please sign in to comment.