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>
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_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 @@
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, 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)
