commit 50de086abd0f23b58320d6aa310bacdd48e80e53
author David Benjamin <davidben@google.com> Thu Feb 02 11:25:15 2023 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Feb 02 16:44:57 2023 +0000
tree 7b9db8460625f796fd55348bf38d609dfec6a482
parent 31f1466a613774369053a94eabbae38fb9cbb7f9

Cap bit indices in the unsafe string-based X.509 extensions API

Without a limit, a short input can translate into a very large allocation,
which is upsetting the fuzzers. Set a limit of 256, which allows up to a
32-byte allocation. (The highest bit index of any type in RFC 5280 is
8, so this is plenty of buffer.)

We do not consider this function to be safe with untrusted inputs (even
without bugs, it is prone to string injection vulnerabilities), so DoS
is not truly a concern, but the limit is necessary to keep fuzzing
effective.

Update-Note: If anyone is using FORMAT:BITLIST to create very large BIT
STRINGs, this will break. This is unlikely and should be caught by
unit tests; if a project hits this outside of tests, that means they are
passing untrusted input into this function, which is a security
vulnerability in itself, and means they especially need this change to
avoid a DoS.

Bug: oss-fuzz:55603
Change-Id: Ie9ec0d35c7d67a568371dfa961867bf1404f7e2f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56785
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

crypto/x509/asn1_gen.c

diff --git a/crypto/x509/asn1_gen.c b/crypto/x509/asn1_gen.c
index 6dcf129..3704808 100644
--- a/crypto/x509/asn1_gen.c
+++ b/crypto/x509/asn1_gen.c
@@ -541,7 +541,16 @@
   CBS_init(&cbs, (const uint8_t *)elem, len);
   uint64_t bitnum;
   if (!CBS_get_u64_decimal(&cbs, &bitnum) || CBS_len(&cbs) != 0 ||
-      bitnum > INT_MAX) {
+      // Cap the highest allowed bit so this mechanism cannot be used to create
+      // extremely large allocations with short inputs. The highest named bit in
+      // RFC 5280 is 8, so 256 should give comfortable margin but still only
+      // allow a 32-byte allocation.
+      //
+      // We do not consider this function to be safe with untrusted inputs (even
+      // without bugs, it is prone to string injection vulnerabilities), so DoS
+      // is not truly a concern, but the limit is necessary to keep fuzzing
+      // effective.
+      bitnum > 256) {
     OPENSSL_PUT_ERROR(ASN1, ASN1_R_INVALID_NUMBER);
     return 0;
   }

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 9abe953..cd231c8 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -6074,8 +6074,18 @@
         0x01, 0x84, 0xb7, 0x09, 0x02, 0x04, 0x04, 0x03, 0x02, 0x02, 0x44}},
 
       {kTestOID, "ASN1:FORMAT:BITLIST,BITSTR:1,invalid,5", nullptr, {}},
-      // Overflow.
-      {kTestOID, "ASN1:FORMAT:BITLIST,BITSTR:4294967296", nullptr, {}},
+      // Negative bit inidices are not allowed.
+      {kTestOID, "ASN1:FORMAT:BITLIST,BITSTR:-1", nullptr, {}},
+      // We cap bit indices at 256.
+      {kTestOID, "ASN1:FORMAT:BITLIST,BITSTR:257", nullptr, {}},
+      {kTestOID,
+       "ASN1:FORMAT:BITLIST,BITSTR:256",
+       nullptr,
+       {0x30, 0x34, 0x06, 0x0c, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04,
+        0x01, 0x84, 0xb7, 0x09, 0x02, 0x04, 0x24, 0x03, 0x22, 0x07, 0x00,
+        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80}},
 
       // Unsupported formats for string types.
       {kTestOID, "ASN1:FORMAT:BITLIST,IA5:abcd", nullptr, {}},