Systematically test that parsers catch trailing data DER being self-describing means we can actually programmatically find all the places to insert trailing data, even without access to the schema. This does not handle DER structures that are embedded inside OCTET STRINGs, like X.509 extensions. Those we'll need to write something else on top of this. Change-Id: Ic22a857d62417bd961cdc92cb8072bbe896bffb6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81769 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com>
diff --git a/build.json b/build.json index 51ac71d..53551d8 100644 --- a/build.json +++ b/build.json
@@ -773,6 +773,7 @@ "test_support": { "srcs": [ "crypto/test/abi_test.cc", + "crypto/test/der_trailing_data.cc", "crypto/test/file_test.cc", "crypto/test/file_test_gtest.cc", "crypto/test/file_util.cc", @@ -782,6 +783,7 @@ ], "internal_hdrs": [ "crypto/test/abi_test.h", + "crypto/test/der_trailing_data.h", "crypto/test/file_test.h", "crypto/test/file_util.h", "crypto/test/gtest_main.h",
diff --git a/crypto/evp/evp_test.cc b/crypto/evp/evp_test.cc index 5c93cdd..d5952e1 100644 --- a/crypto/evp/evp_test.cc +++ b/crypto/evp/evp_test.cc
@@ -39,6 +39,7 @@ #include <openssl/obj.h> #include <openssl/rsa.h> +#include "../test/der_trailing_data.h" #include "../test/file_test.h" #include "../test/test_util.h" #include "../test/wycheproof_util.h" @@ -163,6 +164,21 @@ } keys.emplace_back(format_name + " - all algs", std::move(new_key)); + // Test that the parsers reject trailing data. + bool ok = TestDERTrailingData( + input, [&](bssl::Span<const uint8_t> rewritten, size_t n) { + // We currently intentionally ignore trailing data in the outermost + // PKCS#8 PrivateKeyInfo element because we don't parse the attributes. + if (n == 0 && key_role == KeyRole::kPrivate) { + return; + } + SCOPED_TRACE(n); + bssl::UniquePtr<EVP_PKEY> parsed(parse_func( + rewritten.data(), rewritten.size(), algs.data(), algs.size())); + EXPECT_FALSE(parsed); + }); + EXPECT_TRUE(ok); + // Parse with just the specific algorithm. std::string alg_name; if (!t->GetAttribute(&alg_name, "Algorithm")) {
diff --git a/crypto/test/der_trailing_data.cc b/crypto/test/der_trailing_data.cc new file mode 100644 index 0000000..f5dcf44 --- /dev/null +++ b/crypto/test/der_trailing_data.cc
@@ -0,0 +1,78 @@ +// Copyright 2025 The BoringSSL Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "der_trailing_data.h" + +#include <optional> + +#include <openssl/bytestring.h> + +static bool RewriteWithTrailingData(CBB *cbb, CBS *cbs, + std::optional<size_t> *rewrite_counter) { + CBS contents; + CBS_ASN1_TAG tag; + if (!CBS_get_any_asn1(cbs, &contents, &tag)) { + return false; + } + + if (!rewrite_counter->has_value() || (tag & CBS_ASN1_CONSTRUCTED) == 0) { + return CBB_add_asn1_element(cbb, tag, CBS_data(&contents), + CBS_len(&contents)); + } + + CBB child; + if (!CBB_add_asn1(cbb, &child, tag)) { + return false; + } + + if (rewrite_counter->value() == 0) { + *rewrite_counter = std::nullopt; + return CBB_add_bytes(&child, CBS_data(&contents), CBS_len(&contents)) && + // Add a BER EOC, which is always invalid in DER. + CBB_add_u8(&child, 0) && // + CBB_add_u8(&child, 0) && // + CBB_flush(cbb); + } + + *rewrite_counter = rewrite_counter->value() - 1; + while (CBS_len(&contents) != 0) { + if (!RewriteWithTrailingData(&child, &contents, rewrite_counter)) { + return false; + } + } + return CBB_flush(cbb); +} + +bool TestDERTrailingData( + bssl::Span<const uint8_t> in, + std::function<void(bssl::Span<const uint8_t>, size_t)> func) { + for (size_t elem_to_rewrite = 0; true; elem_to_rewrite++) { + std::optional<size_t> rewrite_counter = elem_to_rewrite; + CBS cbs = in; + bssl::ScopedCBB cbb; + if (!CBB_init(cbb.get(), in.size() + /* EOC */ 2 + + /* in case lengths get larger */ 8) || + !RewriteWithTrailingData(cbb.get(), &cbs, &rewrite_counter) || + CBS_len(&cbs) != 0) { + return false; + } + + // We have exhausted every constructed element. + if (rewrite_counter.has_value()) { + return true; + } + + func(bssl::Span(CBB_data(cbb.get()), CBB_len(cbb.get())), elem_to_rewrite); + } +}
diff --git a/crypto/test/der_trailing_data.h b/crypto/test/der_trailing_data.h new file mode 100644 index 0000000..2d5b369 --- /dev/null +++ b/crypto/test/der_trailing_data.h
@@ -0,0 +1,38 @@ +// Copyright 2025 The BoringSSL Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef OPENSSL_HEADER_CRYPTO_TEST_DER_TRAILING_DATA_H +#define OPENSSL_HEADER_CRYPTO_TEST_DER_TRAILING_DATA_H + +#include <functional> + +#include <openssl/span.h> + +// TestDERTrailingData decodes |in| as an arbitrary DER structure. It then calls +// |func| multiple times on different modified versions of |in|, each time with +// extra data appended to a different constructed element. The extra data will +// be a BER EOC, so this is guaranteed to make the structure invalid. +// +// |func| is expected to parse its argument and then assert with GTest that the +// parser failed. |n|, passed to |func|, is the number of the constructed +// element that was rewritten, following a pre-order numbering from zero. |func| +// should pass it to |SCOPED_TRACE| to aid debugging. +// +// TestDERTrailingData returns whether it successful rewrote |in| and called +// |func| for every constructed element. +bool TestDERTrailingData( + bssl::Span<const uint8_t> in, + std::function<void(bssl::Span<const uint8_t> rewritten, size_t n)> func); + +#endif // OPENSSL_HEADER_CRYPTO_TEST_DER_TRAILING_DATA_H
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 7b49cdc..cfb6823 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -38,6 +38,7 @@ #include <openssl/x509.h> #include "../internal.h" +#include "../test/der_trailing_data.h" #include "../test/file_util.h" #include "../test/test_data.h" #include "../test/test_util.h" @@ -8784,4 +8785,55 @@ } } +TEST(X509Test, TrailingDataX509) { + bssl::UniquePtr<X509> cert(CertFromPEM(kLeafPEM)); + uint8_t *der = nullptr; + int len = i2d_X509(cert.get(), &der); + ASSERT_GT(len, 0); + bssl::UniquePtr<uint8_t> free_der(der); + + bool ok = TestDERTrailingData( + bssl::Span(der, len), [](bssl::Span<const uint8_t> in, size_t n) { + SCOPED_TRACE(n); + const uint8_t *p = in.data(); + bssl::UniquePtr<X509> parsed(d2i_X509(nullptr, &p, in.size())); + EXPECT_FALSE(parsed); + }); + EXPECT_TRUE(ok); +} + +TEST(X509Test, TrailingDataCRL) { + bssl::UniquePtr<X509_CRL> crl(CRLFromPEM(kRevokedCRL)); + uint8_t *der = nullptr; + int len = i2d_X509_CRL(crl.get(), &der); + ASSERT_GT(len, 0); + bssl::UniquePtr<uint8_t> free_der(der); + + bool ok = TestDERTrailingData( + bssl::Span(der, len), [](bssl::Span<const uint8_t> in, size_t n) { + SCOPED_TRACE(n); + const uint8_t *p = in.data(); + bssl::UniquePtr<X509_CRL> parsed(d2i_X509_CRL(nullptr, &p, in.size())); + EXPECT_FALSE(parsed); + }); + EXPECT_TRUE(ok); +} + +TEST(X509Test, TrailingDataCSR) { + bssl::UniquePtr<X509_REQ> csr(CSRFromPEM(kTestCSR)); + uint8_t *der = nullptr; + int len = i2d_X509_REQ(csr.get(), &der); + ASSERT_GT(len, 0); + bssl::UniquePtr<uint8_t> free_der(der); + + bool ok = TestDERTrailingData( + bssl::Span(der, len), [](bssl::Span<const uint8_t> in, size_t n) { + SCOPED_TRACE(n); + const uint8_t *p = in.data(); + bssl::UniquePtr<X509_REQ> parsed(d2i_X509_REQ(nullptr, &p, in.size())); + EXPECT_FALSE(parsed); + }); + EXPECT_TRUE(ok); +} + } // namespace
diff --git a/gen/sources.bzl b/gen/sources.bzl index 6793c33..72d897b 100644 --- a/gen/sources.bzl +++ b/gen/sources.bzl
@@ -2844,6 +2844,7 @@ test_support_sources = [ "crypto/test/abi_test.cc", + "crypto/test/der_trailing_data.cc", "crypto/test/file_test.cc", "crypto/test/file_test_gtest.cc", "crypto/test/file_util.cc", @@ -2854,6 +2855,7 @@ test_support_internal_headers = [ "crypto/test/abi_test.h", + "crypto/test/der_trailing_data.h", "crypto/test/file_test.h", "crypto/test/file_util.h", "crypto/test/gtest_main.h",
diff --git a/gen/sources.cmake b/gen/sources.cmake index 4b73d17..be6608b 100644 --- a/gen/sources.cmake +++ b/gen/sources.cmake
@@ -2906,6 +2906,7 @@ TEST_SUPPORT_SOURCES crypto/test/abi_test.cc + crypto/test/der_trailing_data.cc crypto/test/file_test.cc crypto/test/file_test_gtest.cc crypto/test/file_util.cc @@ -2918,6 +2919,7 @@ TEST_SUPPORT_INTERNAL_HEADERS crypto/test/abi_test.h + crypto/test/der_trailing_data.h crypto/test/file_test.h crypto/test/file_util.h crypto/test/gtest_main.h
diff --git a/gen/sources.gni b/gen/sources.gni index b846755..83740e0 100644 --- a/gen/sources.gni +++ b/gen/sources.gni
@@ -2844,6 +2844,7 @@ test_support_sources = [ "crypto/test/abi_test.cc", + "crypto/test/der_trailing_data.cc", "crypto/test/file_test.cc", "crypto/test/file_test_gtest.cc", "crypto/test/file_util.cc", @@ -2854,6 +2855,7 @@ test_support_internal_headers = [ "crypto/test/abi_test.h", + "crypto/test/der_trailing_data.h", "crypto/test/file_test.h", "crypto/test/file_util.h", "crypto/test/gtest_main.h",
diff --git a/gen/sources.json b/gen/sources.json index 49a26a4..6334c44 100644 --- a/gen/sources.json +++ b/gen/sources.json
@@ -2828,6 +2828,7 @@ "test_support": { "srcs": [ "crypto/test/abi_test.cc", + "crypto/test/der_trailing_data.cc", "crypto/test/file_test.cc", "crypto/test/file_test_gtest.cc", "crypto/test/file_util.cc", @@ -2837,6 +2838,7 @@ ], "internal_hdrs": [ "crypto/test/abi_test.h", + "crypto/test/der_trailing_data.h", "crypto/test/file_test.h", "crypto/test/file_util.h", "crypto/test/gtest_main.h",
diff --git a/gen/sources.mk b/gen/sources.mk index 5c57c27..7de81ef 100644 --- a/gen/sources.mk +++ b/gen/sources.mk
@@ -2814,6 +2814,7 @@ boringssl_test_support_sources := \ crypto/test/abi_test.cc \ + crypto/test/der_trailing_data.cc \ crypto/test/file_test.cc \ crypto/test/file_test_gtest.cc \ crypto/test/file_util.cc \ @@ -2823,6 +2824,7 @@ boringssl_test_support_internal_headers := \ crypto/test/abi_test.h \ + crypto/test/der_trailing_data.h \ crypto/test/file_test.h \ crypto/test/file_util.h \ crypto/test/gtest_main.h \