Serialize SSL curve list in handoff and check it on application.
A split SSL handshake may involve 2 binaries, potentially built at
different versions: call them the "handoff/handback" binary and the
"handshake" binary. We would like to guarantee that the
handoff/handback binary does not make any promises that the handshake
binary cannot keep.
d2ed382 serialized |kCiphers|; this commit extends the same approach
to |kNamedGroups|.
Change-Id: Idb13e54e9b189236309f6054a36872c5a4d96985
Reviewed-on: https://boringssl-review.googlesource.com/c/32825
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index a49d289..4cca981 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -37,6 +37,15 @@
return false;
}
}
+ CBB curves;
+ if (!CBB_add_asn1(out, &curves, CBS_ASN1_OCTETSTRING)) {
+ return false;
+ }
+ for (const NamedGroup& g : NamedGroups()) {
+ if (!CBB_add_u16(&curves, g.group_id)) {
+ return false;
+ }
+ }
return CBB_flush(out);
}
@@ -120,7 +129,52 @@
for (const SSL_CIPHER *unsupported_cipher : unsupported.get()) {
ssl->config->cipher_list->Remove(unsupported_cipher);
}
- return sk_SSL_CIPHER_num(SSL_get_ciphers(ssl)) > 0;
+ if (sk_SSL_CIPHER_num(SSL_get_ciphers(ssl)) == 0) {
+ return false;
+ }
+
+ CBS curves;
+ if (!CBS_get_asn1(in, &curves, CBS_ASN1_OCTETSTRING)) {
+ return false;
+ }
+ Array<uint16_t> supported_curves;
+ if (!supported_curves.Init(CBS_len(&curves) / 2)) {
+ return false;
+ }
+ size_t idx = 0;
+ while (CBS_len(&curves)) {
+ uint16_t curve;
+ if (!CBS_get_u16(&curves, &curve)) {
+ return false;
+ }
+ supported_curves[idx++] = curve;
+ }
+ Span<const uint16_t> configured_curves =
+ tls1_get_grouplist(ssl->s3->hs.get());
+ Array<uint16_t> new_configured_curves;
+ if (!new_configured_curves.Init(configured_curves.size())) {
+ return false;
+ }
+ idx = 0;
+ for (uint16_t configured_curve : configured_curves) {
+ bool ok = false;
+ for (uint16_t supported_curve : supported_curves) {
+ if (supported_curve == configured_curve) {
+ ok = true;
+ break;
+ }
+ }
+ if (ok) {
+ new_configured_curves[idx++] = configured_curve;
+ }
+ }
+ if (idx == 0) {
+ return false;
+ }
+ new_configured_curves.Shrink(idx);
+ ssl->config->supported_group_list = std::move(new_configured_curves);
+
+ return true;
}
bool SSL_apply_handoff(SSL *ssl, Span<const uint8_t> handoff) {
diff --git a/ssl/internal.h b/ssl/internal.h
index 2e05ee0..9d78b1f 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1000,6 +1000,15 @@
virtual bool Deserialize(CBS *in) { return false; }
};
+struct NamedGroup {
+ int nid;
+ uint16_t group_id;
+ const char name[8], alias[11];
+};
+
+// NamedGroups returns all supported groups.
+Span<const NamedGroup> NamedGroups();
+
// 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.
diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc
index 8466eab..80b7d0a 100644
--- a/ssl/ssl_key_share.cc
+++ b/ssl/ssl_key_share.cc
@@ -211,11 +211,7 @@
uint8_t private_key_[32];
};
-CONSTEXPR_ARRAY struct {
- int nid;
- uint16_t group_id;
- const char name[8], alias[11];
-} kNamedGroups[] = {
+CONSTEXPR_ARRAY NamedGroup kNamedGroups[] = {
{NID_secp224r1, SSL_CURVE_SECP224R1, "P-224", "secp224r1"},
{NID_X9_62_prime256v1, SSL_CURVE_SECP256R1, "P-256", "prime256v1"},
{NID_secp384r1, SSL_CURVE_SECP384R1, "P-384", "secp384r1"},
@@ -225,6 +221,10 @@
} // namespace
+Span<const NamedGroup> NamedGroups() {
+ return MakeConstSpan(kNamedGroups, OPENSSL_ARRAY_SIZE(kNamedGroups));
+}
+
UniquePtr<SSLKeyShare> SSLKeyShare::Create(uint16_t group_id) {
switch (group_id) {
case SSL_CURVE_SECP224R1:
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index f7b299a..14b85d4 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -4285,20 +4285,29 @@
// handoff is a handoff message that has been artificially modified to pretend
// that only cipher 0x0A is supported. When it is applied to |server|, all
// ciphers but that one should be removed.
+ //
+ // To make a new one of these, try sticking this in the |Handoff| test above:
+ //
+ // hexdump(stderr, "", handoff.data(), handoff.size());
+ // sed -e 's/\(..\)/0x\1, /g'
+ //
+ // and modify serialize_features() to emit only cipher 0x0A.
+
uint8_t handoff[] = {
- 0x30, 0x81, 0x8e, 0x02, 0x01, 0x00, 0x04, 0x00, 0x04, 0x81, 0x82, 0x01,
- 0x00, 0x00, 0x7e, 0x03, 0x03, 0x77, 0x62, 0x00, 0x9a, 0x13, 0x48, 0x23,
- 0x46, 0x11, 0x6c, 0x0b, 0x1c, 0x91, 0x4e, 0xbc, 0x1c, 0xff, 0x54, 0xb9,
- 0xe6, 0x3f, 0xa8, 0x8d, 0x49, 0x37, 0x7a, 0x9e, 0xbf, 0x36, 0xd5, 0x08,
- 0x24, 0x00, 0x00, 0x1e, 0xc0, 0x2b, 0xc0, 0x2f, 0xc0, 0x2c, 0xc0, 0x30,
+ 0x30, 0x81, 0x9a, 0x02, 0x01, 0x00, 0x04, 0x00, 0x04, 0x81, 0x82, 0x01,
+ 0x00, 0x00, 0x7e, 0x03, 0x03, 0x30, 0x8e, 0x8f, 0x79, 0xd2, 0x87, 0x39,
+ 0xc2, 0x23, 0x23, 0x13, 0xca, 0x3c, 0x80, 0x44, 0xfd, 0x80, 0x83, 0x62,
+ 0x3c, 0xcc, 0xf8, 0x76, 0xd3, 0x62, 0xbb, 0x54, 0xe3, 0xc4, 0x39, 0x24,
+ 0xa5, 0x00, 0x00, 0x1e, 0xc0, 0x2b, 0xc0, 0x2f, 0xc0, 0x2c, 0xc0, 0x30,
0xcc, 0xa9, 0xcc, 0xa8, 0xc0, 0x09, 0xc0, 0x13, 0xc0, 0x0a, 0xc0, 0x14,
0x00, 0x9c, 0x00, 0x9d, 0x00, 0x2f, 0x00, 0x35, 0x00, 0x0a, 0x01, 0x00,
- 0x00, 0x37, 0xff, 0x01, 0x00, 0x01, 0x00, 0x00, 0x17, 0x00, 0x00, 0x00,
- 0x23, 0x00, 0x00, 0x00, 0x0d, 0x00, 0x14, 0x00, 0x12, 0x04, 0x03, 0x08,
- 0x04, 0x04, 0x01, 0x05, 0x03, 0x08, 0x05, 0x05, 0x01, 0x08, 0x06, 0x06,
- 0x01, 0x02, 0x01, 0x00, 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x0a, 0x00,
- 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18, 0x04, 0x02, 0x00,
- 0x0a,
+ 0x00, 0x37, 0x00, 0x17, 0x00, 0x00, 0xff, 0x01, 0x00, 0x01, 0x00, 0x00,
+ 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18, 0x00,
+ 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x23, 0x00, 0x00, 0x00, 0x0d, 0x00,
+ 0x14, 0x00, 0x12, 0x04, 0x03, 0x08, 0x04, 0x04, 0x01, 0x05, 0x03, 0x08,
+ 0x05, 0x05, 0x01, 0x08, 0x06, 0x06, 0x01, 0x02, 0x01, 0x04, 0x02, 0x00,
+ 0x0a, 0x04, 0x0a, 0x00, 0x15, 0x00, 0x17, 0x00, 0x18, 0x00, 0x19, 0x00,
+ 0x1d,
};
EXPECT_EQ(20u, sk_SSL_CIPHER_num(SSL_get_ciphers(server.get())));
@@ -4307,6 +4316,43 @@
EXPECT_EQ(1u, sk_SSL_CIPHER_num(SSL_get_ciphers(server.get())));
}
+TEST(SSLTest, ApplyHandoffRemovesUnsupportedCurves) {
+ bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(TLS_method()));
+ bssl::UniquePtr<SSL> server(SSL_new(server_ctx.get()));
+
+ // handoff is a handoff message that has been artificially modified to pretend
+ // that only one curve is supported. When it is applied to |server|, all
+ // curves but that one should be removed.
+ //
+ // See |ApplyHandoffRemovesUnsupportedCiphers| for how to make a new one of
+ // these.
+ uint8_t handoff[] = {
+ 0x30, 0x81, 0xc0, 0x02, 0x01, 0x00, 0x04, 0x00, 0x04, 0x81, 0x82, 0x01,
+ 0x00, 0x00, 0x7e, 0x03, 0x03, 0x98, 0x30, 0xce, 0xd9, 0xb0, 0xdf, 0x5f,
+ 0x82, 0x05, 0x4a, 0x43, 0x67, 0x7e, 0xdb, 0x6a, 0x4f, 0x21, 0x18, 0x4e,
+ 0x0d, 0x94, 0x63, 0x18, 0x8b, 0x54, 0x89, 0xdb, 0x8b, 0x1d, 0x84, 0xbc,
+ 0x09, 0x00, 0x00, 0x1e, 0xc0, 0x2b, 0xc0, 0x2f, 0xc0, 0x2c, 0xc0, 0x30,
+ 0xcc, 0xa9, 0xcc, 0xa8, 0xc0, 0x09, 0xc0, 0x13, 0xc0, 0x0a, 0xc0, 0x14,
+ 0x00, 0x9c, 0x00, 0x9d, 0x00, 0x2f, 0x00, 0x35, 0x00, 0x0a, 0x01, 0x00,
+ 0x00, 0x37, 0x00, 0x17, 0x00, 0x00, 0xff, 0x01, 0x00, 0x01, 0x00, 0x00,
+ 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18, 0x00,
+ 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x23, 0x00, 0x00, 0x00, 0x0d, 0x00,
+ 0x14, 0x00, 0x12, 0x04, 0x03, 0x08, 0x04, 0x04, 0x01, 0x05, 0x03, 0x08,
+ 0x05, 0x05, 0x01, 0x08, 0x06, 0x06, 0x01, 0x02, 0x01, 0x04, 0x30, 0x00,
+ 0x02, 0x00, 0x0a, 0x00, 0x2f, 0x00, 0x35, 0x00, 0x8c, 0x00, 0x8d, 0x00,
+ 0x9c, 0x00, 0x9d, 0x13, 0x01, 0x13, 0x02, 0x13, 0x03, 0xc0, 0x09, 0xc0,
+ 0x0a, 0xc0, 0x13, 0xc0, 0x14, 0xc0, 0x2b, 0xc0, 0x2c, 0xc0, 0x2f, 0xc0,
+ 0x30, 0xc0, 0x35, 0xc0, 0x36, 0xcc, 0xa8, 0xcc, 0xa9, 0xcc, 0xac, 0x04,
+ 0x02, 0x00, 0x17,
+ };
+
+ // The zero length means that the default list of groups is used.
+ EXPECT_EQ(0u, server->config->supported_group_list.size());
+ ASSERT_TRUE(
+ SSL_apply_handoff(server.get(), {handoff, OPENSSL_ARRAY_SIZE(handoff)}));
+ EXPECT_EQ(1u, server->config->supported_group_list.size());
+}
+
TEST_P(SSLVersionTest, VerifyBeforeCertRequest) {
// Configure the server to request client certificates.
SSL_CTX_set_custom_verify(
