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(