From 7c1433eb1959ef7579cb460aab464bc0441467e3 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Wed, 17 Jan 2024 16:40:23 -0500
Subject: [PATCH] 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>
---
 crypto/bytestring/ber.c                         |   7 ++-----
 crypto/bytestring/bytestring_test.cc            |  16 ++++++++++++++++
 crypto/pkcs8/pkcs12_test.cc                     |   8 ++++++++
 crypto/pkcs8/test/empty_password_ber_nested.p12 | Bin 0 -> 1619 bytes
 sources.cmake                                   |   1 +
 5 files changed, 27 insertions(+), 5 deletions(-)
 create mode 100644 crypto/pkcs8/test/empty_password_ber_nested.p12

diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
index 522174bc9..b3ba7711b 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 deae6bb52..a40844bac 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -730,6 +730,22 @@ TEST(CBSTest, BerConvert) {
   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 50cedff0f..0416ad7b1 100644
--- a/crypto/pkcs8/pkcs12_test.cc
+++ b/crypto/pkcs8/pkcs12_test.cc
@@ -158,6 +158,14 @@ TEST(PKCS12Test, TestEmptyPassword) {
            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 0000000000000000000000000000000000000000..63671b0a65d5cf50c0bcfc174eaf22bce297603d
GIT binary patch
literal 1619
zcmY+?c`(*_9KdnU&(rgG@Q?=4$T~hMLcd26tt2Y<edX$~kCsG^4z;N7FOs8(bUkVw
zN8{L@*&bK>R4R!`Y;>_rQ_`bLMT2Uj_S4Mnw)@9xJ~N-sU!R#b&2W4umLkn?v^YGo
z{jU2z@L61znBj1M;ZVR(+ffpVN3kdxZA6hM6a^ta<b~YOYP1q9M|Nll5+Dm?icF9p
znu~Oi7Scd`#6vR>3mDck`h+IX7<z+VA~_mFkI(?Rhk8*r>O^g*1vMiXYD7{bK{e<M
zDnqBx2~>g(qk||1Wg&vn(Jr(dC82l}i=xp+6p2Dn5b{G_$PKMVE75Xfhn64#vOuQD
z1R0{aNEc}#4a7%0Gy}1Kd;u~A<Rg#?An$>^12O{SC6HktLqMJYc?9H7AP<1t1=0(o
z2S^u?4j^qnt^v6M<Ps1WkR~AYK+XY?0I3C11*8&48IaRJL_m%MDFIRhqyR`hkQ^ZU
zf$Rga7cw9Xc0(%cfNhWriI4zWU^B$PCJ;gtY=CtT0b#Hff*}z6!56&26WqZS*1#%o
z0!MHFdsqgxU<1}*1vFTKIher$m<IqxU;z4{2XjCNv_TVQ!AwvCRZs>c-~byawgQ_`
z%%T}ZLNnCP?_F}l3>AYbA;nU_5&8YXqV+h+f9I>p;?QbrrKk0Fea&I{Y80iUu$Q6e
zR3lctcDKg<vKtB4ca|M&$ZD;leoD-nIu}u(H==$cy-8Vdy@7(2h2<>ounn$TZ@<jf
z3XO;w2G8jq=rW@`b@@^zPM3cx%d1ByVobldKhx+NX7xM_6vlC58+E6~k~8hS4+^Ji
z7T3-Vonzmv|8|)}Zt0jIy!P+iV-*r3K5KF0g=c7m^OVjm&&$^`mnYOO>PWt1I>O0r
zJ?ZMGHf-JE{=qEaiE3%NRnUEB^HbYAMk8t$#Whqk%^Wn4l%E#_>J<e^T|~jpa|#_R
zvy*H~Mx5t)gi7K^uYK4mK6%mDch!{wx22sv_lz?pBd7Yg!Y9=g%N%YLgyhzRcYdC4
zV$pp|6tRk%J}z;ZW#XzCY_v%}@?wY8kdE~1LiN5P=ILE0K3`COC}8&WT;)$&?Rt4Z
zY&lQs`3keqoUhXAq1=z6O6%fRYv%o7oWf+M4fToK0>V$!+|8~Wz0EkDQ^~JOzG*HE
z35=dRaFJhpwkxHu`WNk>l#Gm_!M5wFHtQB2b}LQNn79*e+dSN;o}6#puCt|6?pS)c
z-@(i2&e7wVex?`Rc>3^sUB)ssqq$j(B-uJJTy0h4@8(HA2N|Ro$bxsruDJ;=&I2yb
zQ#0B>hsp=GKaaNTuSi(?=xE$H-DmIAV^+GSD)#Y2YRf)-!!U{2X63wMvC7f-f!dIo
zK7paXhMz~4oV!Tm`?adLxN7pWUB`5QSG;iHqxOs!U)a?KAM4A*#VYf2O%h9JhDH4+
zFfkctlQEjSrO9iK%0H<ovnVlXpo#Q<{V9Iy@5eLs3EMp0sebE^O$u!SUAU8#>!k}G
zbQKLYJJ*bsbB~$wN8DfaYQ8)(v&(;;rOwBWl<~KR52d!)K1dTq%7kv~jpLq;4JIea
zB##9rf4wc4Ewk`Aboq(&$o;EJ-`YlBZS&^sre^p1?DHwNjA}aOVAHIn<mlUymmzB3
zHK_mFKmfC`&Oi5kaAIq6cTMBy=Bpk#^0Z=^eoVu%R)Gm^{O@}_ErB*$bA`ZmL^ig1
z(4jDi-?HF>_f~P~8(Kgc(mc+Lg=&h_oEZvi%{_O+Gp)~m%07_hzk3&TbnUH&r{1%b
Pq9)P{GUY;(Z_oY(x&CEK

literal 0
HcmV?d00001

diff --git a/sources.cmake b/sources.cmake
index b6206bf60..365245867 100644
--- a/sources.cmake
+++ b/sources.cmake
@@ -148,6 +148,7 @@ set(
   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