From 414a0f86e53329a8b27b2d73f2975048c86a9736 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Fri, 29 Oct 2021 09:05:33 -0400
Subject: [PATCH] Don't parse constructed BIT STRINGs in crypto/bytestring

Update-Note: PKCS#7 and PKCS#12 parsers will now reject BER constructed
BIT STRINGs. We were previously misparsing them, as was OpenSSL. Given
how long the incorrect parse has been out there, without anyone noticing
(other parsers handle it correctly), it is unlikely these exist.

Change-Id: I61d317461cc59480dc9f772f88edc7758206d20d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50289
Reviewed-by: Adam Langley <agl@google.com>
---
 crypto/bytestring/ber.c              |  7 ++---
 crypto/bytestring/bytestring_test.cc | 43 ++++++++++++++--------------
 2 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
index fd35e9707..e7f67ddfa 100644
--- a/crypto/bytestring/ber.c
+++ b/crypto/bytestring/ber.c
@@ -29,11 +29,10 @@ static const unsigned kMaxDepth = 2048;
 // is_string_type returns one if |tag| is a string type and zero otherwise. It
 // ignores the constructed bit.
 static int is_string_type(unsigned tag) {
+  // While BER supports constructed BIT STRINGS, OpenSSL misparses them. To
+  // avoid acting on an ambiguous input, we do not support constructed BIT
+  // STRINGS. See https://github.com/openssl/openssl/issues/12810.
   switch (tag & ~CBS_ASN1_CONSTRUCTED) {
-    // TODO(davidben): Reject constructed BIT STRINGs. The current handling
-    // matches OpenSSL but is incorrect. See
-    // https://github.com/openssl/openssl/issues/12810.
-    case CBS_ASN1_BITSTRING:
     case CBS_ASN1_OCTETSTRING:
     case CBS_ASN1_UTF8STRING:
     case CBS_ASN1_NUMERICSTRING:
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 0877a2e72..985e38ccc 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -22,6 +22,7 @@
 
 #include <openssl/bytestring.h>
 #include <openssl/crypto.h>
+#include <openssl/span.h>
 
 #include "internal.h"
 #include "../internal.h"
@@ -594,22 +595,22 @@ TEST(CBBTest, ASN1) {
   EXPECT_EQ(Bytes(test_data.data(), test_data.size()), Bytes(buf + 10, 100000));
 }
 
-static void ExpectBerConvert(const char *name, const uint8_t *der_expected,
-                             size_t der_len, const uint8_t *ber,
-                             size_t ber_len) {
+static void ExpectBerConvert(const char *name,
+                             bssl::Span<const uint8_t> der_expected,
+                             bssl::Span<const uint8_t> ber) {
   SCOPED_TRACE(name);
   CBS in, out;
   uint8_t *storage;
 
-  CBS_init(&in, ber, ber_len);
+  CBS_init(&in, ber.data(), ber.size());
   ASSERT_TRUE(CBS_asn1_ber_to_der(&in, &out, &storage));
   bssl::UniquePtr<uint8_t> scoper(storage);
 
-  EXPECT_EQ(Bytes(der_expected, der_len), Bytes(CBS_data(&out), CBS_len(&out)));
+  EXPECT_EQ(Bytes(der_expected), Bytes(CBS_data(&out), CBS_len(&out)));
   if (storage != nullptr) {
-    EXPECT_NE(Bytes(der_expected, der_len), Bytes(ber, ber_len));
+    EXPECT_NE(Bytes(der_expected), Bytes(ber));
   } else {
-    EXPECT_EQ(Bytes(der_expected, der_len), Bytes(ber, ber_len));
+    EXPECT_EQ(Bytes(der_expected), Bytes(ber));
   }
 }
 
@@ -670,22 +671,22 @@ TEST(CBSTest, BerConvert) {
       0xa0, 0x08, 0x04, 0x02, 0x00, 0x01, 0x04, 0x02, 0x02, 0x03,
   };
 
-  ExpectBerConvert("kSimpleBER", kSimpleBER, sizeof(kSimpleBER), kSimpleBER,
-                   sizeof(kSimpleBER));
+  // kConstructedBitString contains a BER constructed BIT STRING. These are not
+  // supported and thus are left unchanged.
+  static const uint8_t kConstructedBitStringBER[] = {
+      0x23, 0x0a, 0x03, 0x03, 0x00, 0x12, 0x34, 0x03, 0x03, 0x00, 0x56, 0x78};
+
+  ExpectBerConvert("kSimpleBER", kSimpleBER, kSimpleBER);
   ExpectBerConvert("kNonMinimalLengthBER", kNonMinimalLengthDER,
-                   sizeof(kNonMinimalLengthDER), kNonMinimalLengthBER,
-                   sizeof(kNonMinimalLengthBER));
-  ExpectBerConvert("kIndefBER", kIndefDER, sizeof(kIndefDER), kIndefBER,
-                   sizeof(kIndefBER));
-  ExpectBerConvert("kIndefBER2", kIndefDER2, sizeof(kIndefDER2), kIndefBER2,
-                   sizeof(kIndefBER2));
-  ExpectBerConvert("kOctetStringBER", kOctetStringDER, sizeof(kOctetStringDER),
-                   kOctetStringBER, sizeof(kOctetStringBER));
-  ExpectBerConvert("kNSSBER", kNSSDER, sizeof(kNSSDER), kNSSBER,
-                   sizeof(kNSSBER));
+                   kNonMinimalLengthBER);
+  ExpectBerConvert("kIndefBER", kIndefDER, kIndefBER);
+  ExpectBerConvert("kIndefBER2", kIndefDER2, kIndefBER2);
+  ExpectBerConvert("kOctetStringBER", kOctetStringDER, kOctetStringBER);
+  ExpectBerConvert("kNSSBER", kNSSDER, kNSSBER);
   ExpectBerConvert("kConstructedStringBER", kConstructedStringDER,
-                   sizeof(kConstructedStringDER), kConstructedStringBER,
-                   sizeof(kConstructedStringBER));
+                   kConstructedStringBER);
+  ExpectBerConvert("kConstructedBitStringBER", kConstructedBitStringBER,
+                   kConstructedBitStringBER);
 }
 
 struct BERTest {