commit 414a0f86e53329a8b27b2d73f2975048c86a9736
author David Benjamin <davidben@google.com> Fri Oct 29 09:05:33 2021 -0400
committer Adam Langley <agl@google.com> Mon Nov 01 18:08:51 2021 +0000
tree f6920e4a57437a5a373f0db94239858f76fca8c3
parent 13c67c99d8b2e4efef4fb6f6b67e3abfd6031920

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

diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
index fd35e97..e7f67dd 100644
--- a/crypto/bytestring/ber.c
+++ b/crypto/bytestring/ber.c
@@ -29,11 +29,10 @@
 // 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:

crypto/bytestring/bytestring_test.cc

diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 0877a2e..985e38c 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 @@
   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 @@
       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 {