From 28d501c2067e2783121fafa48f5eecb52558c79e Mon Sep 17 00:00:00 2001 From: Rudolf Polzer Date: Tue, 19 May 2026 06:45:51 -0700 Subject: [PATCH 1/8] RSA_generate_key_ex: reject invalid values of e. Previously, invalid values of e made the function loop forever. Change-Id: I3d03e4b096f988ebb01b378e52d7dab66a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95548 Reviewed-by: Adam Langley Commit-Queue: Rudolf Polzer --- crypto/fipsmodule/rsa/rsa_impl.cc.inc | 30 ++++++++++++++++++ crypto/rsa/rsa_test.cc | 45 +++++++++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/crypto/fipsmodule/rsa/rsa_impl.cc.inc b/crypto/fipsmodule/rsa/rsa_impl.cc.inc index f745eb28e7..3ed7347928 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.cc.inc +++ b/crypto/fipsmodule/rsa/rsa_impl.cc.inc @@ -752,6 +752,36 @@ static int rsa_generate_key_impl(RSAImpl *rsa, int bits, const BIGNUM *e_value, return 0; } + // The smallest reasonable RSA exponent is 3, and it definitely must be odd. + // Catching this here prevents endless loops or slow computation when trying + // to actually generate keys later. + if (BN_is_negative(e_value)) { + // You're kidding. + // + // Would fail in |bn_lcm_consttime| anyway, as it only allows positive + // integers. + // + // However, |RSA_R_BAD_E_VALUE| is a better error return than |ERR_LIB_BN|. + OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_E_VALUE); + return 0; + } + if (!BN_is_odd(e_value)) { + // The R in RSA doesn't stand for Rabin. + // + // Would fail in |generate_prime| anyway, as only one |rsa->p|-1 is coprime + // with an even |e_value| and that one is a little bit short. + // + // However, |RSA_R_BAD_E_VALUE| is a better error return than |ERR_LIB_BN|. + OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_E_VALUE); + return 0; + } + if (BN_is_one(e_value)) { + // Would loop endlessly because _somehow_ it'll always compute a |rsa->d| + // exponent of 1, which is too small. + OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_E_VALUE); + return 0; + } + UniquePtr ctx(BN_CTX_new()); if (ctx == nullptr) { OPENSSL_PUT_ERROR(RSA, ERR_LIB_BN); diff --git a/crypto/rsa/rsa_test.cc b/crypto/rsa/rsa_test.cc index ad6f644874..a7052b36e8 100644 --- a/crypto/rsa/rsa_test.cc +++ b/crypto/rsa/rsa_test.cc @@ -1019,6 +1019,51 @@ TEST(RSATest, CheckKey) { ERR_clear_error(); } +TEST(RSATest, KeygenBadExponent) { + UniquePtr rsa_opaque(RSA_new()); + RSAImpl *rsa = FromOpaque(rsa_opaque.get()); + ASSERT_TRUE(rsa); + + UniquePtr e(BN_new()); + ASSERT_TRUE(e); + + // 3 is the smallest allowed public key value. + ASSERT_TRUE(BN_set_word(e.get(), 3)); + ASSERT_TRUE(RSA_generate_key_ex(rsa, 2048, e.get(), nullptr)); + + // Maybe I prefer the Rabin scheme. But this is an RSA API! + ASSERT_TRUE(BN_set_word(e.get(), 2)); + EXPECT_FALSE(RSA_generate_key_ex(rsa, 2048, e.get(), nullptr)); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_RSA, RSA_R_BAD_E_VALUE)); + + // Rabin-Shamir-Adleman? Nope. + ASSERT_TRUE(BN_set_word(e.get(), 6)); + EXPECT_FALSE(RSA_generate_key_ex(rsa, 2048, e.get(), nullptr)); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_RSA, RSA_R_BAD_E_VALUE)); + + // RSA with exponent 1 is a joke. But we already use ROT26 for that purpose. + ASSERT_TRUE(BN_set_word(e.get(), 1)); + EXPECT_FALSE(RSA_generate_key_ex(rsa, 2048, e.get(), nullptr)); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_RSA, RSA_R_BAD_E_VALUE)); + + // RSA with exponent 0 is also known as /dev/null, and not supported here. + ASSERT_TRUE(BN_set_word(e.get(), 0)); + EXPECT_FALSE(RSA_generate_key_ex(rsa, 2048, e.get(), nullptr)); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_RSA, RSA_R_BAD_E_VALUE)); + + // Now this is just silly - perfectly fine RSA with e=3, except with an extra + // inversion that cryptographically does nothing at all except waste cycles. + // Throw it away. + ASSERT_TRUE(BN_set_word(e.get(), 3)); + BN_set_negative(e.get(), 1); + EXPECT_FALSE(RSA_generate_key_ex(rsa, 2048, e.get(), nullptr)); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_RSA, RSA_R_BAD_E_VALUE)); + + // To validate nothing got corrupted, try good old 65537. + ASSERT_TRUE(BN_set_word(e.get(), RSA_F4)); + EXPECT_TRUE(RSA_generate_key_ex(rsa, 2048, e.get(), nullptr)); +} + TEST(RSATest, KeygenFail) { UniquePtr rsa_opaque(RSA_new()); RSAImpl *rsa = FromOpaque(rsa_opaque.get()); From 3fff7111b0eca817466e121059cb4e8b67ade35b Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Tue, 19 May 2026 15:31:14 +0000 Subject: [PATCH 2/8] RSA_generate_key_ex: tighten up code a bit. Change-Id: I2301ea3ce75292bcd4f0e943c916973fb543fdc3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95567 Commit-Queue: Rudolf Polzer Reviewed-by: Rudolf Polzer Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com Auto-Submit: Adam Langley --- crypto/fipsmodule/rsa/rsa_impl.cc.inc | 41 +++++++++------------------ 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/crypto/fipsmodule/rsa/rsa_impl.cc.inc b/crypto/fipsmodule/rsa/rsa_impl.cc.inc index 3ed7347928..ba08ae772d 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.cc.inc +++ b/crypto/fipsmodule/rsa/rsa_impl.cc.inc @@ -753,31 +753,18 @@ static int rsa_generate_key_impl(RSAImpl *rsa, int bits, const BIGNUM *e_value, } // The smallest reasonable RSA exponent is 3, and it definitely must be odd. - // Catching this here prevents endless loops or slow computation when trying - // to actually generate keys later. - if (BN_is_negative(e_value)) { - // You're kidding. - // - // Would fail in |bn_lcm_consttime| anyway, as it only allows positive - // integers. - // - // However, |RSA_R_BAD_E_VALUE| is a better error return than |ERR_LIB_BN|. - OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_E_VALUE); - return 0; - } - if (!BN_is_odd(e_value)) { - // The R in RSA doesn't stand for Rabin. - // - // Would fail in |generate_prime| anyway, as only one |rsa->p|-1 is coprime - // with an even |e_value| and that one is a little bit short. - // - // However, |RSA_R_BAD_E_VALUE| is a better error return than |ERR_LIB_BN|. - OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_E_VALUE); - return 0; - } - if (BN_is_one(e_value)) { - // Would loop endlessly because _somehow_ it'll always compute a |rsa->d| - // exponent of 1, which is too small. + // Catching these here prevents endless loops or slow computation when trying + // to generate keys later, and results in a better error code. + if ( + // Would fail in |bn_lcm_consttime| as it only allows positive integers. + BN_is_negative(e_value) || + // Would fail in |generate_prime| as only one |rsa->p|-1 is coprime with + // an even |e_value| and that one is a little bit short. (The R in RSA + // doesn't stand for Rabin.) + !BN_is_odd(e_value) || + // Would loop endlessly because it'll always compute an |rsa->d| exponent + // of 1, which is too small. + BN_is_one(e_value)) { OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_E_VALUE); return 0; } @@ -830,8 +817,8 @@ static int rsa_generate_key_impl(RSAImpl *rsa, int bits, const BIGNUM *e_value, if (!generate_prime(rsa->p.get(), prime_bits, rsa->e.get(), nullptr, pow2_prime_bits_100, ctx.get(), cb) || !BN_GENCB_call(cb, 3, 0) || - !generate_prime(rsa->q.get(), prime_bits, rsa->e.get(), rsa->p.get(), pow2_prime_bits_100, - ctx.get(), cb) || + !generate_prime(rsa->q.get(), prime_bits, rsa->e.get(), rsa->p.get(), + pow2_prime_bits_100, ctx.get(), cb) || !BN_GENCB_call(cb, 3, 1)) { OPENSSL_PUT_ERROR(RSA, ERR_LIB_BN); return 0; From c2c9f2f3e5c5d57d0186b14ce2463f9c4a8f8cde Mon Sep 17 00:00:00 2001 From: Rudolf Polzer Date: Tue, 19 May 2026 04:01:27 -0700 Subject: [PATCH 3/8] X509_VERIFY_PARAM_inherit/_set1: refuse if either params are poisoned. Before, these functions would unconditionally copy the src's poison flag to the dst, potentially resulting in an unpoisoned context even if the reason for poisoning (e.g. a field with invalid value) remains. After this change, copying always fails if any of the two params is poisoned. The most relevant code path for this is when `X509_STORE_CTX_init` is called with a poisoned `X509_STORE`; it happily copied the poisoned context and then cleared the poison flag while inheriting from defaults. Yes, instead the calls in `X509_STORE_CTX_init` could be reversed to first `inherit` from the defaults and then to `set1` from the user provided context, which then would yield the correct copied poison flag; however it seems prudent to instead harden the public APIs, as there could be more issues of this kind. Not considering a vulnerability as the poison flag can only ever be set on a call to us if a caller used an API wrong by ignoring its return value. Of course, the whole purpose of the flag is to detect and fail such callers so no damage happens. Change-Id: Iaed97dfa21863c882a0d46b1b8439ec06a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95527 Commit-Queue: Rudolf Polzer Reviewed-by: Adam Langley Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com --- crypto/x509/x509_test.cc | 30 ++++++++++++++++++++++++++++++ crypto/x509/x509_vpm.cc | 18 +++++++++++------- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index d3ab671228..cc79b09016 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -8434,6 +8434,36 @@ TEST(X509Test, ParamInheritance) { // The new value is used. EXPECT_EQ(X509_VERIFY_PARAM_get_depth(dest.get()), 10); } + + // |X509_VERIFY_PARAM_inherit| and |X509_VERIFY_PARAM_set1| must fail if the + // source parameter is poisoned. + { + UniquePtr dest(X509_VERIFY_PARAM_new()); + ASSERT_TRUE(dest); + UniquePtr src(X509_VERIFY_PARAM_new()); + ASSERT_TRUE(src); + + // Poison the source parameter (using an embedded NUL in hostname). + ASSERT_FALSE(X509_VERIFY_PARAM_set1_host(src.get(), "a", 2)); + + EXPECT_FALSE(X509_VERIFY_PARAM_inherit(dest.get(), src.get())); + EXPECT_FALSE(X509_VERIFY_PARAM_set1(dest.get(), src.get())); + } + + // |X509_VERIFY_PARAM_inherit| and |X509_VERIFY_PARAM_set1| must fail if the + // destination parameter is poisoned. + { + UniquePtr dest(X509_VERIFY_PARAM_new()); + ASSERT_TRUE(dest); + UniquePtr src(X509_VERIFY_PARAM_new()); + ASSERT_TRUE(src); + + // Poison the destination parameter (using an embedded NUL in hostname). + ASSERT_FALSE(X509_VERIFY_PARAM_set1_host(dest.get(), "a", 2)); + + EXPECT_FALSE(X509_VERIFY_PARAM_inherit(dest.get(), src.get())); + EXPECT_FALSE(X509_VERIFY_PARAM_set1(dest.get(), src.get())); + } } TEST(X509Test, PublicKeyCache) { diff --git a/crypto/x509/x509_vpm.cc b/crypto/x509/x509_vpm.cc index a0f8a71723..7507981441 100644 --- a/crypto/x509/x509_vpm.cc +++ b/crypto/x509/x509_vpm.cc @@ -117,16 +117,21 @@ static void copy_int_param(T *dest, const T *src, T default_val, } } -// x509_verify_param_copy copies fields from |src| to |dest|. If both |src| and +// x509_verify_param_merge merges fields from |src| to |dest|. If both |src| and // |dest| have some field set, |prefer_src| determines whether |src| or |dest|'s // version is used. -static int x509_verify_param_copy(X509_VERIFY_PARAM *dest, - const X509_VERIFY_PARAM *src, - bool prefer_src) { +static int x509_verify_param_merge(X509_VERIFY_PARAM *dest, + const X509_VERIFY_PARAM *src, + bool prefer_src) { if (src == nullptr) { return 1; } + if (src->poison || dest->poison) { + OPENSSL_PUT_ERROR(X509, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + return 0; + } + copy_int_param(&dest->purpose, &src->purpose, /*default_val=*/0, prefer_src); copy_int_param(&dest->trust, &src->trust, /*default_val=*/0, prefer_src); copy_int_param(&dest->depth, &src->depth, /*default_val=*/-1, prefer_src); @@ -175,7 +180,6 @@ static int x509_verify_param_copy(X509_VERIFY_PARAM *dest, } } - dest->poison = src->poison; return 1; } @@ -183,14 +187,14 @@ int X509_VERIFY_PARAM_inherit(X509_VERIFY_PARAM *dest, const X509_VERIFY_PARAM *src) { // Prefer the destination. That is, this function only changes unset // parameters in |dest|. - return x509_verify_param_copy(dest, src, /*prefer_src=*/false); + return x509_verify_param_merge(dest, src, /*prefer_src=*/false); } int X509_VERIFY_PARAM_set1(X509_VERIFY_PARAM *to, const X509_VERIFY_PARAM *from) { // Prefer the source. That is, values in |to| are only preserved if they were // unset in |from|. - return x509_verify_param_copy(to, from, /*prefer_src=*/true); + return x509_verify_param_merge(to, from, /*prefer_src=*/true); } static int int_x509_param_set1(char **pdest, size_t *pdestlen, const char *src, From 2d8ef80d4d9a2f5a09bd03a17c66fdd706c3f89e Mon Sep 17 00:00:00 2001 From: Rudolf Polzer Date: Wed, 20 May 2026 03:21:01 -0700 Subject: [PATCH 4/8] RSA: handle gracefully when a SSL_PRIVATE_KEY_METHOD has NULL methods. Instead of UB, this now returns an error. This makes sense, given these can both sign and decrypt, but there are conceivable applications where only one is necessary. Change-Id: Idec6c00d17594aa604170b978370be396a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95607 Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com Commit-Queue: Xiangfei Ding Reviewed-by: Xiangfei Ding --- ssl/ssl_privkey.cc | 16 +++++++++++++++ ssl/ssl_test.cc | 51 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc index 29475296af..b48becdd10 100644 --- a/ssl/ssl_privkey.cc +++ b/ssl/ssl_privkey.cc @@ -261,8 +261,16 @@ enum ssl_private_key_result_t ssl_private_key_sign( assert(!hs->can_release_private_key); if (key_method != nullptr) { + if (key_method->sign == nullptr) { + OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); + return ssl_private_key_failure; + } enum ssl_private_key_result_t ret; if (hs->pending_private_key_op) { + if (key_method->complete == nullptr) { + OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); + return ssl_private_key_failure; + } ret = key_method->complete(ssl, out, out_len, max_out); } else { ret = key_method->sign(ssl, out, out_len, max_out, sigalg, in.data(), @@ -321,8 +329,16 @@ enum ssl_private_key_result_t ssl_private_key_decrypt(SSL_HANDSHAKE *hs, const SSLCredential *const cred = hs->credential.get(); assert(!hs->can_release_private_key); if (cred->key_method != nullptr) { + if (cred->key_method->decrypt == nullptr) { + OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); + return ssl_private_key_failure; + } enum ssl_private_key_result_t ret; if (hs->pending_private_key_op) { + if (cred->key_method->complete == nullptr) { + OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); + return ssl_private_key_failure; + } ret = cred->key_method->complete(ssl, out, out_len, max_out); } else { ret = cred->key_method->decrypt(ssl, out, out_len, max_out, in.data(), diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index a876718a78..a91a6268ca 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -5504,6 +5504,57 @@ TEST(SSLTest, OverrideKeyMethodWithKey) { ASSERT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get())); } +TEST(SSLTest, NullDecryptPrivateKeyMethod) { + bssl::UniquePtr key = GetTestKey(); + ASSERT_TRUE(key); + bssl::UniquePtr leaf = GetTestCertificate(); + ASSERT_TRUE(leaf); + + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(client_ctx); + bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(server_ctx); + + ASSERT_TRUE(SSL_CTX_use_certificate(server_ctx.get(), leaf.get())); + + // Configure an SSL_PRIVATE_KEY_METHOD on the server with sign and complete, + // but NULL decrypt. + static const SSL_PRIVATE_KEY_METHOD kNullDecryptMethod = { + [](SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, + uint16_t signature_algorithm, const uint8_t *in, + size_t in_len) { return ssl_private_key_failure; }, + nullptr, // decrypt + [](SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out) { + return ssl_private_key_failure; + }, + }; + + SSL_CTX_set_private_key_method(server_ctx.get(), &kNullDecryptMethod); + + // Negotiate RSA key exchange (SSL_kRSA). We do this by restricting both + // client and server to TLS 1.2 and configuring an RSA key exchange cipher + // suite. + ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), TLS1_2_VERSION)); + ASSERT_TRUE(SSL_CTX_set_max_proto_version(server_ctx.get(), TLS1_2_VERSION)); + + ASSERT_TRUE(SSL_CTX_set_cipher_list(client_ctx.get(), "AES128-GCM-SHA256")); + ASSERT_TRUE(SSL_CTX_set_cipher_list(server_ctx.get(), "AES128-GCM-SHA256")); + + // Use a custom verify on client to accept the self-signed test cert. + SSL_CTX_set_custom_verify(client_ctx.get(), SSL_VERIFY_PEER, + AcceptAnyCertificate); + + bssl::UniquePtr client, server; + // Since the decrypt hook is NULL, the server's decryption during the RSA key + // exchange should fail cleanly. + EXPECT_FALSE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + uint32_t err = ERR_get_error(); + EXPECT_TRUE(ErrorEquals(err, ERR_LIB_SSL, ERR_R_INTERNAL_ERROR)); + EXPECT_EQ(0u, ERR_get_error()); +} + + // Configuring a chain and then overwriting it with a different chain should // clear the old one. TEST(SSLTest, OverrideChain) { From e0d763c0a49764ce75df710741d6f90a23d584ca Mon Sep 17 00:00:00 2001 From: Rudolf Polzer Date: Wed, 20 May 2026 04:55:29 -0700 Subject: [PATCH 5/8] bio_read_all: bail out on every error. Otherwise, trying to read from an uninitialized BIO will underflow due to -2 being used as return value to indicate that. Note that the only way to trigger this function is using the public BIO_read_asn1 API; however, it does a bio_read_full first, which makes it hard to actually have this condition happen. The BIO would basically have to succeed at first, and suddenly return -2 later. None of the built-in BIOs in BoringSSL can do that. Change-Id: I38777688490ea6cc2d9af23def2beadd6a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95628 Reviewed-by: Adam Langley Commit-Queue: Rudolf Polzer Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com --- crypto/bio/bio.cc | 2 +- crypto/bio/bio_test.cc | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/crypto/bio/bio.cc b/crypto/bio/bio.cc index a3be772845..9250be0fbd 100644 --- a/crypto/bio/bio.cc +++ b/crypto/bio/bio.cc @@ -421,7 +421,7 @@ static int bio_read_all(Bio *bio, uint8_t **out, size_t *out_len, if (n == 0) { *out_len = done; return 1; - } else if (n == -1) { + } else if (n < 0) { OPENSSL_free(*out); return 0; } diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc index 5c9317486f..861eb2cfb5 100644 --- a/crypto/bio/bio_test.cc +++ b/crypto/bio/bio_test.cc @@ -513,6 +513,46 @@ TEST(BIOTest, ReadASN1) { } } +TEST(BIOTest, ReadASN1ErrorNegative) { + // A custom BIO whose bread callback returns a negative value other than -1. + BIO_METHOD *meth = BIO_meth_new(BIO_TYPE_SOURCE_SINK, "evil"); + ASSERT_TRUE(meth); + BIO_meth_set_read(meth, [](BIO *bio, char *buf, int len) -> int { + int *call_count = reinterpret_cast(BIO_get_data(bio)); + (*call_count)++; + if (*call_count == 1) { + if (len < 2) { + return -1; + } + buf[0] = '\x30'; + buf[1] = '\x80'; + return 2; + } + return -2; + }); + BIO_meth_set_ctrl( + meth, [](BIO *bio, int cmd, long larg, void *parg) -> long { return 1; }); + + UniquePtr bio(BIO_new(meth)); + ASSERT_TRUE(bio); + + int call_count = 0; + BIO_set_data(bio.get(), &call_count); + BIO_set_init(bio.get(), 1); + + uint8_t *out = nullptr; + size_t out_len = 0; + int ok = BIO_read_asn1(bio.get(), &out, &out_len, 1000); + EXPECT_EQ(ok, 0); + if (ok == 1) { + OPENSSL_free(out); + } + + bio.reset(); + BIO_meth_free(meth); +} + + TEST(BIOTest, MemReadOnly) { // A memory BIO created from |BIO_new_mem_buf| is a read-only buffer. static const char kData[] = "abcdefghijklmno"; From 5ee9407bc28dd9086507f02851886a185088f3a0 Mon Sep 17 00:00:00 2001 From: Rudolf Polzer Date: Wed, 20 May 2026 00:59:25 -0700 Subject: [PATCH 6/8] Fix off-by-one allowing an unauthenticated handshake abort. Not considering a vulnerability as any attacker who can do this can just as well inject ICMP Destination Unreachable or even just UDP flood the client. Having said that, definitely an interesting bug. Change-Id: I7608b38a26abcb41ffccaab453990c066a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95587 Commit-Queue: Rudolf Polzer Reviewed-by: Adam Langley Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com --- ssl/d1_both.cc | 2 +- ssl/test/runner/dtls_tests.go | 72 +++++++++++++++++++++++++++++++++++ ssl/test/runner/runner.go | 1 + 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc index c98dae49e9..7b7a1184fb 100644 --- a/ssl/d1_both.cc +++ b/ssl/d1_both.cc @@ -318,7 +318,7 @@ bool dtls1_process_handshake_fragments(SSL *ssl, uint8_t *out_alert, implicit_ack = true; } - if (msg_hdr.seq - ssl->d1->handshake_read_seq > SSL_MAX_HANDSHAKE_FLIGHT) { + if (msg_hdr.seq - ssl->d1->handshake_read_seq >= SSL_MAX_HANDSHAKE_FLIGHT) { // Ignore fragments too far in the future. skipped_fragments = true; continue; diff --git a/ssl/test/runner/dtls_tests.go b/ssl/test/runner/dtls_tests.go index 45bbf6742c..215c8b5be2 100644 --- a/ssl/test/runner/dtls_tests.go +++ b/ssl/test/runner/dtls_tests.go @@ -1610,3 +1610,75 @@ func addDTLSReorderTests() { }) } } + +func addDTLSFragmentWindowTests() { + for _, vers := range allVersions(dtls) { + // A handshake fragment with a sequence number exactly at the upper bound + // of the sliding window (handshake_read_seq + SSL_MAX_HANDSHAKE_FLIGHT, + // which is +7) should be silently discarded rather than triggering a + // fatal internal error alert. + testCases = append(testCases, testCase{ + protocol: dtls, + testType: serverTest, + name: "DTLS-FragmentWindow-UpperLimit-Discard-Server-" + vers.name, + config: Config{ + MaxVersion: vers.version, + Bugs: ProtocolBugs{ + WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { + for _, msg := range next { + if msg.Sequence == 0 && !msg.IsChangeCipherSpec { + // Injected future fragment with offset +7. + // Since SSL_MAX_HANDSHAKE_FLIGHT is 7, sequence + 7 is at the upper limit. + // This must be silently discarded by the shim. + shouldDiscard := DTLSFragment{ + Epoch: msg.Epoch, + Type: msg.Type, + Sequence: msg.Sequence + 7, + Offset: 0, + TotalLength: len(msg.Data), + Data: msg.Data, + ShouldDiscard: true, + } + c.WriteFragments([]DTLSFragment{shouldDiscard, msg.Fragment(0, len(msg.Data))}) + } else { + c.WriteFragments([]DTLSFragment{msg.Fragment(0, len(msg.Data))}) + } + } + }, + }, + }, + }) + + testCases = append(testCases, testCase{ + protocol: dtls, + testType: clientTest, + name: "DTLS-FragmentWindow-UpperLimit-Discard-Client-" + vers.name, + config: Config{ + MaxVersion: vers.version, + Bugs: ProtocolBugs{ + PackHandshakeFragments: 4096, + WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { + for _, msg := range next { + if msg.Sequence == 0 && !msg.IsChangeCipherSpec { + // Injected future fragment with offset +7. + // This must be silently discarded by the shim. + shouldDiscard := DTLSFragment{ + Epoch: msg.Epoch, + Type: msg.Type, + Sequence: msg.Sequence + 7, + Offset: 0, + TotalLength: len(msg.Data), + Data: msg.Data, + ShouldDiscard: true, + } + c.WriteFragments([]DTLSFragment{shouldDiscard, msg.Fragment(0, len(msg.Data))}) + } else { + c.WriteFragments([]DTLSFragment{msg.Fragment(0, len(msg.Data))}) + } + } + }, + }, + }, + }) + } +} diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 14c6c8d461..f92e3b1b46 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -2363,6 +2363,7 @@ func main() { addSignatureAlgorithmTests() addDTLSRetransmitTests() addDTLSReorderTests() + addDTLSFragmentWindowTests() addExportKeyingMaterialTests() addExportTrafficSecretsTests() addTLSUniqueTests() From e2da59b2d9e85f20a87d4b9116bc4a3de3525603 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 May 2026 12:48:14 -0400 Subject: [PATCH 7/8] Bump BORINGSSL_API_VERSION Change-Id: I5f0b162ca9c2bd4a718b4e933499f15509e420e2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95727 Auto-Submit: David Benjamin Commit-Queue: Emily Stark Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- include/openssl/base.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/openssl/base.h b/include/openssl/base.h index 961e542a04..1ce0e1bb22 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h @@ -73,7 +73,7 @@ extern "C" { // A consumer may use this symbol in the preprocessor to temporarily build // against multiple revisions of BoringSSL at the same time. It is not // recommended to do so for longer than is necessary. -#define BORINGSSL_API_VERSION 40 +#define BORINGSSL_API_VERSION 41 #if defined(BORINGSSL_SHARED_LIBRARY) From 956bac6e4db9b33789dcedb5ea3f28e51030cead Mon Sep 17 00:00:00 2001 From: Matt Mueller Date: Fri, 22 May 2026 14:44:38 -0700 Subject: [PATCH 8/8] add CBS_get_u48 The latest MTC draft uses uint48 values. Change-Id: Ide5e405de82e1bdd6b6229d8f959973348bbc360 Bug: 452983502 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95747 Reviewed-by: David Benjamin Commit-Queue: Matt Mueller --- crypto/bytestring/bytestring_test.cc | 11 +++++++---- crypto/bytestring/cbs.cc | 2 ++ include/openssl/bytestring.h | 4 ++++ include/openssl/prefix_symbols.h | 2 ++ 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc index 0e36a2e3c4..e0c200d776 100644 --- a/crypto/bytestring/bytestring_test.cc +++ b/crypto/bytestring/bytestring_test.cc @@ -59,7 +59,8 @@ TEST(CBSTest, Skip) { TEST(CBSTest, GetUint) { static const uint8_t kData[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, - 11, 12, 13, 14, 15, 16, 17, 18, 19, 20}; + 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, + 21, 22, 23, 24, 25, 26}; uint8_t u8; uint16_t u16; uint32_t u32; @@ -75,12 +76,14 @@ TEST(CBSTest, GetUint) { EXPECT_EQ(0x40506u, u32); ASSERT_TRUE(CBS_get_u32(&data, &u32)); EXPECT_EQ(0x708090au, u32); + ASSERT_TRUE(CBS_get_u48(&data, &u64)); + EXPECT_EQ(0xb0c0d0e0f10u, u64); ASSERT_TRUE(CBS_get_u64(&data, &u64)); - EXPECT_EQ(0xb0c0d0e0f101112u, u64); + EXPECT_EQ(0x1112131415161718u, u64); ASSERT_TRUE(CBS_get_last_u8(&data, &u8)); - EXPECT_EQ(0x14u, u8); + EXPECT_EQ(0x1au, u8); ASSERT_TRUE(CBS_get_last_u8(&data, &u8)); - EXPECT_EQ(0x13u, u8); + EXPECT_EQ(0x19u, u8); EXPECT_FALSE(CBS_get_u8(&data, &u8)); EXPECT_FALSE(CBS_get_last_u8(&data, &u8)); diff --git a/crypto/bytestring/cbs.cc b/crypto/bytestring/cbs.cc index dbaa3e8af5..f9011e3ae0 100644 --- a/crypto/bytestring/cbs.cc +++ b/crypto/bytestring/cbs.cc @@ -146,6 +146,8 @@ int CBS_get_u32le(CBS *cbs, uint32_t *out) { return 1; } +int CBS_get_u48(CBS *cbs, uint64_t *out) { return cbs_get_u(cbs, out, 6); } + int CBS_get_u64(CBS *cbs, uint64_t *out) { return cbs_get_u(cbs, out, 8); } int CBS_get_u64le(CBS *cbs, uint64_t *out) { diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h index 3e1cfe5d27..50263cb3d9 100644 --- a/include/openssl/bytestring.h +++ b/include/openssl/bytestring.h @@ -121,6 +121,10 @@ OPENSSL_EXPORT int CBS_get_u32(CBS *cbs, uint32_t *out); // |cbs| and advances |cbs|. It returns one on success and zero on error. OPENSSL_EXPORT int CBS_get_u32le(CBS *cbs, uint32_t *out); +// CBS_get_u48 sets |*out| to the next, big-endian 48-bit value from |cbs| and +// advances |cbs|. It returns one on success and zero on error. +OPENSSL_EXPORT int CBS_get_u48(CBS *cbs, uint64_t *out); + // CBS_get_u64 sets |*out| to the next, big-endian uint64_t value from |cbs| // and advances |cbs|. It returns one on success and zero on error. OPENSSL_EXPORT int CBS_get_u64(CBS *cbs, uint64_t *out); diff --git a/include/openssl/prefix_symbols.h b/include/openssl/prefix_symbols.h index c25242da86..4d970a3f2a 100644 --- a/include/openssl/prefix_symbols.h +++ b/include/openssl/prefix_symbols.h @@ -568,6 +568,7 @@ #pragma redefine_extname CBS_get_u24_length_prefixed BORINGSSL_ADD_USER_LABEL_AND_PREFIX(CBS_get_u24_length_prefixed) #pragma redefine_extname CBS_get_u32 BORINGSSL_ADD_USER_LABEL_AND_PREFIX(CBS_get_u32) #pragma redefine_extname CBS_get_u32le BORINGSSL_ADD_USER_LABEL_AND_PREFIX(CBS_get_u32le) +#pragma redefine_extname CBS_get_u48 BORINGSSL_ADD_USER_LABEL_AND_PREFIX(CBS_get_u48) #pragma redefine_extname CBS_get_u64 BORINGSSL_ADD_USER_LABEL_AND_PREFIX(CBS_get_u64) #pragma redefine_extname CBS_get_u64_decimal BORINGSSL_ADD_USER_LABEL_AND_PREFIX(CBS_get_u64_decimal) #pragma redefine_extname CBS_get_u64le BORINGSSL_ADD_USER_LABEL_AND_PREFIX(CBS_get_u64le) @@ -3682,6 +3683,7 @@ #define CBS_get_u24_length_prefixed BORINGSSL_ADD_PREFIX(CBS_get_u24_length_prefixed) #define CBS_get_u32 BORINGSSL_ADD_PREFIX(CBS_get_u32) #define CBS_get_u32le BORINGSSL_ADD_PREFIX(CBS_get_u32le) +#define CBS_get_u48 BORINGSSL_ADD_PREFIX(CBS_get_u48) #define CBS_get_u64 BORINGSSL_ADD_PREFIX(CBS_get_u64) #define CBS_get_u64_decimal BORINGSSL_ADD_PREFIX(CBS_get_u64_decimal) #define CBS_get_u64le BORINGSSL_ADD_PREFIX(CBS_get_u64le)