Reduce the BER conversion recursion depth
2048 is too high, particularly in heavily instrumented fuzzer builds
with large stack frames. Use a much more conservative limit.
Change-Id: If0b49f2ca04520c41400dcbfd83463766fded3a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65508
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
index 522174b..b3ba771 100644
--- a/crypto/bytestring/ber.c
+++ b/crypto/bytestring/ber.c
@@ -18,13 +18,10 @@
#include <string.h>
#include "internal.h"
-#include "../internal.h"
-// kMaxDepth is a just a sanity limit. The code should be such that the length
-// of the input being processes always decreases. None the less, a very large
-// input could otherwise cause the stack to overflow.
-static const uint32_t kMaxDepth = 2048;
+// kMaxDepth limits the recursion depth to avoid overflowing the stack.
+static const uint32_t kMaxDepth = 128;
// is_string_type returns one if |tag| is a string type and zero otherwise. It
// ignores the constructed bit.
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index deae6bb..a40844b 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -730,6 +730,22 @@
ExpectBerConvert("kWrappedIndefBER", kWrappedIndefDER, kWrappedIndefBER);
ExpectBerConvert("kWrappedConstructedStringBER", kWrappedConstructedStringDER,
kWrappedConstructedStringBER);
+
+ // indef_overflow is 200 levels deep of an indefinite-length-encoded SEQUENCE.
+ // This will exceed our recursion limits and fail to be converted.
+ std::vector<uint8_t> indef_overflow;
+ for (int i = 0; i < 200; i++) {
+ indef_overflow.push_back(0x30);
+ indef_overflow.push_back(0x80);
+ }
+ for (int i = 0; i < 200; i++) {
+ indef_overflow.push_back(0x00);
+ indef_overflow.push_back(0x00);
+ }
+ CBS in, out;
+ CBS_init(&in, indef_overflow.data(), indef_overflow.size());
+ uint8_t *storage;
+ ASSERT_FALSE(CBS_asn1_ber_to_der(&in, &out, &storage));
}
struct BERTest {
diff --git a/crypto/pkcs8/pkcs12_test.cc b/crypto/pkcs8/pkcs12_test.cc
index 50cedff..0416ad7 100644
--- a/crypto/pkcs8/pkcs12_test.cc
+++ b/crypto/pkcs8/pkcs12_test.cc
@@ -158,6 +158,14 @@
nullptr);
TestImpl("EmptyPassword (BER, null password)", StringToBytes(data), nullptr,
nullptr);
+
+ // The constructed string with too much recursion.
+ data = GetTestData("crypto/pkcs8/test/empty_password_ber_nested.p12");
+ bssl::UniquePtr<STACK_OF(X509)> certs(sk_X509_new_null());
+ ASSERT_TRUE(certs);
+ EVP_PKEY *key = nullptr;
+ CBS pkcs12 = StringToBytes(data);
+ EXPECT_FALSE(PKCS12_get_key_and_certs(&key, certs.get(), &pkcs12, ""));
}
TEST(PKCS12Test, TestNullPassword) {
diff --git a/crypto/pkcs8/test/empty_password_ber_nested.p12 b/crypto/pkcs8/test/empty_password_ber_nested.p12
new file mode 100644
index 0000000..63671b0
--- /dev/null
+++ b/crypto/pkcs8/test/empty_password_ber_nested.p12
Binary files differ
diff --git a/sources.cmake b/sources.cmake
index b6206bf..3652458 100644
--- a/sources.cmake
+++ b/sources.cmake
@@ -148,6 +148,7 @@
crypto/kyber/kyber_tests.txt
crypto/pkcs8/test/empty_password.p12
crypto/pkcs8/test/empty_password_ber.p12
+ crypto/pkcs8/test/empty_password_ber_nested.p12
crypto/pkcs8/test/no_encryption.p12
crypto/pkcs8/test/nss.p12
crypto/pkcs8/test/null_password.p12
