Fix a bug detecting BER deeply nested inside DER

When BER was nested deep inside otherwise valid DER, cbs_find_ber's
internally book-keeping would clobber *out_ber_found and lose track that
there was some BER to convert.

This is unlikely to come up in real world inputs, as the only reason to
use any of these awful BER features is to stream the output. I.e., there
is no reason to have indefinite-length inside definite-length, and there
is no reason to have definite-length constructed string. That means
cbs_find_ber, realistically, can just check for indefinite-length
encoding at the front and do nothing else.

Nonetheless, these inputs are nominally allowed, so fix the book-keeping
bug.

Change-Id: Idaf3c178fa511ce24af83d6ae27fa9f197f1789e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65507
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

crypto/bytestring/ber.c

diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
index 5d43d0b..522174b 100644
--- a/crypto/bytestring/ber.c
+++ b/crypto/bytestring/ber.c
@@ -56,13 +56,11 @@
 // found. The value of |orig_in| is not changed. It returns one on success (i.e.
 // |*ber_found| was set) and zero on error.
 static int cbs_find_ber(const CBS *orig_in, int *ber_found, uint32_t depth) {
-  CBS in;
-
   if (depth > kMaxDepth) {
     return 0;
   }
 
-  CBS_init(&in, CBS_data(orig_in), CBS_len(orig_in));
+  CBS in = *orig_in;
   *ber_found = 0;
 
   while (CBS_len(&in) > 0) {
@@ -87,6 +85,10 @@
           !cbs_find_ber(&contents, ber_found, depth + 1)) {
         return 0;
       }
+      if (*ber_found) {
+        // We already found BER. No need to continue parsing.
+        return 1;
+      }
     }
   }
 

crypto/bytestring/bytestring_test.cc

diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 08cfb87..deae6bb 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -679,6 +679,38 @@
       0xa0, 0x08, 0x04, 0x02, 0x00, 0x01, 0x04, 0x02, 0x02, 0x03,
   };
 
+  // kWrappedIndefBER contains indefinite-length SEQUENCE, wrapped
+  // and followed by valid DER. This tests that we correctly identify BER nested
+  // inside DER.
+  //
+  //  SEQUENCE {
+  //    SEQUENCE {
+  //      SEQUENCE indefinite {}
+  //    }
+  //    SEQUENCE {}
+  //  }
+  static const uint8_t kWrappedIndefBER[] = {0x30, 0x08, 0x30, 0x04, 0x30,
+                                             0x80, 0x00, 0x00, 0x30, 0x00};
+  static const uint8_t kWrappedIndefDER[] = {0x30, 0x06, 0x30, 0x02,
+                                             0x30, 0x00, 0x30, 0x00};
+
+  // kWrappedConstructedStringBER contains a constructed OCTET STRING, wrapped
+  // and followed by valid DER. This tests that we correctly identify BER nested
+  // inside DER.
+  //
+  //  SEQUENCE {
+  //    SEQUENCE {
+  //      [OCTET_STRING CONSTRUCTED] {
+  //        OCTET_STRING {}
+  //      }
+  //    }
+  //    SEQUENCE {}
+  //  }
+  static const uint8_t kWrappedConstructedStringBER[] = {
+      0x30, 0x08, 0x30, 0x04, 0x24, 0x02, 0x04, 0x00, 0x30, 0x00};
+  static const uint8_t kWrappedConstructedStringDER[] = {
+      0x30, 0x06, 0x30, 0x02, 0x04, 0x00, 0x30, 0x00};
+
   // kConstructedBitString contains a BER constructed BIT STRING. These are not
   // supported and thus are left unchanged.
   static const uint8_t kConstructedBitStringBER[] = {
@@ -695,6 +727,9 @@
                    kConstructedStringBER);
   ExpectBerConvert("kConstructedBitStringBER", kConstructedBitStringBER,
                    kConstructedBitStringBER);
+  ExpectBerConvert("kWrappedIndefBER", kWrappedIndefDER, kWrappedIndefBER);
+  ExpectBerConvert("kWrappedConstructedStringBER", kWrappedConstructedStringDER,
+                   kWrappedConstructedStringBER);
 }
 
 struct BERTest {

crypto/pkcs8/pkcs12_test.cc

diff --git a/crypto/pkcs8/pkcs12_test.cc b/crypto/pkcs8/pkcs12_test.cc
index 79ebae3..50cedff 100644
--- a/crypto/pkcs8/pkcs12_test.cc
+++ b/crypto/pkcs8/pkcs12_test.cc
@@ -151,6 +151,13 @@
   TestImpl("EmptyPassword (empty password)", StringToBytes(data), "", nullptr);
   TestImpl("EmptyPassword (null password)", StringToBytes(data), nullptr,
            nullptr);
+
+  // The above input, modified to have a constructed string.
+  data = GetTestData("crypto/pkcs8/test/empty_password_ber.p12");
+  TestImpl("EmptyPassword (BER, empty password)", StringToBytes(data), "",
+           nullptr);
+  TestImpl("EmptyPassword (BER, null password)", StringToBytes(data), nullptr,
+           nullptr);
 }
 
 TEST(PKCS12Test, TestNullPassword) {

crypto/pkcs8/test/empty_password_ber.p12

diff --git a/crypto/pkcs8/test/empty_password_ber.p12 b/crypto/pkcs8/test/empty_password_ber.p12
new file mode 100644
index 0000000..3a87ebb
--- /dev/null
+++ b/crypto/pkcs8/test/empty_password_ber.p12
Binary files differ

sources.cmake

diff --git a/sources.cmake b/sources.cmake
index 5cf324b..b6206bf 100644
--- a/sources.cmake
+++ b/sources.cmake
@@ -147,6 +147,7 @@
   crypto/keccak/keccak_tests.txt
   crypto/kyber/kyber_tests.txt
   crypto/pkcs8/test/empty_password.p12
+  crypto/pkcs8/test/empty_password_ber.p12
   crypto/pkcs8/test/no_encryption.p12
   crypto/pkcs8/test/nss.p12
   crypto/pkcs8/test/null_password.p12