Tighten the limit in ASN1_STRING_set further

The actual maximum allowed for this function is INT_MAX - 1. However,
elsewhere we bound parsers to INT_MAX / 2, to guard against overflows
elsewhere in crypto/asn1.

As parsing code is rewritten with CBS, it will naturally handle size_t.
Rather than manually pepper bounds checks in all the functions, we can
catch it at ASN1_STRING_set time. (Whether we'll still need this hack by
the end, I'm not sure, but let's keep it for now.)

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

2 files changed, 28 insertions, 3 deletions

diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
index dd56c98..0158622 100644
--- a/crypto/asn1/asn1_lib.c
+++ b/crypto/asn1/asn1_lib.c
@@ -102,6 +102,15 @@ OPENSSL_DECLARE_ERROR_REASON(ASN1, UNKNOWN_FORMAT)
 OPENSSL_DECLARE_ERROR_REASON(ASN1, UNKNOWN_TAG)
 OPENSSL_DECLARE_ERROR_REASON(ASN1, UNSUPPORTED_TYPE)
 
+// Limit |ASN1_STRING|s to 64 MiB of data. Most of this module, as well as
+// downstream code, does not correctly handle overflow. We cap string fields
+// more tightly than strictly necessary to fit in |int|. This is not expected to
+// impact real world uses of this field.
+//
+// In particular, this limit is small enough that the bit count of a BIT STRING
+// comfortably fits in an |int|, with room for arithmetic.
+#define ASN1_STRING_MAX (64 * 1024 * 1024)
+
 static void asn1_put_length(unsigned char **pp, int length);
 
 int ASN1_get_object(const unsigned char **inp, long *out_len, int *out_tag,
@@ -273,9 +282,8 @@ int ASN1_STRING_set(ASN1_STRING *str, const void *_data, ossl_ssize_t len_s) {
     len = (size_t)len_s;
   }
 
-  // |ASN1_STRING| cannot represent strings that exceed |int|, and we must
-  // reserve space for a trailing NUL below.
-  if (len > INT_MAX || len + 1 < len) {
+  static_assert(ASN1_STRING_MAX < INT_MAX, "len will not overflow int");
+  if (len > ASN1_STRING_MAX) {
     OPENSSL_PUT_ERROR(ASN1, ERR_R_OVERFLOW);
     return 0;
   }
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 94f2272..07628c8 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -2459,6 +2459,23 @@ TEST(ASN1Test, POSIXTime) {
   }
 }
 
+TEST(ASN1Test, LargeString) {
+  bssl::UniquePtr<ASN1_STRING> str(ASN1_STRING_type_new(V_ASN1_OCTET_STRING));
+  ASSERT_TRUE(str);
+  // Very large strings should be rejected by |ASN1_STRING_set|. Strictly
+  // speaking, this is an invalid call because the buffer does not have that
+  // much size available. |ASN1_STRING_set| should cleanly fail before it
+  // crashes, and actually allocating 512 MiB in a test is likely to break.
+  char b = 0;
+  EXPECT_FALSE(ASN1_STRING_set(str.get(), &b, INT_MAX / 4));
+
+#if defined(OPENSSL_64_BIT)
+  // |ASN1_STRING_set| should tolerate lengths that exceed |int| without
+  // overflow.
+  EXPECT_FALSE(ASN1_STRING_set(str.get(), &b, 1 + (ossl_ssize_t{1} << 48)));
+#endif
+}
+
 // The ASN.1 macros do not work on Windows shared library builds, where usage of
 // |OPENSSL_EXPORT| is a bit stricter.
 #if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY)