Bound the overall output size of ASN1_generate_v3
The output of ASN1_generate_v3 is *mostly* linear with the input, except
SEQ and SET reference config sections. Sections can be referenced
multiple times, and so the structure grows exponentially.
Cap the total output size to mitigate this. As before, we don't consider
these functions safe to use with untrusted inputs, but unbounded growth
will frustrate fuzzing. This CL sets the limit to 64K, which should be
enough for anyone. (This is the size of a single X.509 extension,
whereas certificates themselves should not get that large.)
While not strictly necessary, this also rearranges the
ASN1_mbstring_copy call to pass in a maximum output. This portion does
scale linearly with the output, so it's fine, but the fuzzer discovered
an input with a 700K-byte input, which, with fuzzer instrumentation and
sanitizers, seems to be a bit slow. This change should help the fuzzer
get past those cases faster.
Update-Note: The stringly-typed API for constructing X.509 extensions
now has a maximum output size. If anyone was constructing an extension
larger than 64K, 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:55725
Change-Id: Ibb65854293f44bf48ed5855016ef7cd46d2fae77
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57125
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/a_mbstr.c b/crypto/asn1/a_mbstr.c
index 81916c2..e1a3df2 100644
--- a/crypto/asn1/a_mbstr.c
+++ b/crypto/asn1/a_mbstr.c
@@ -162,20 +162,16 @@
nchar++;
utf8_len += cbb_get_utf8_len(c);
+ if (maxsize > 0 && nchar > (size_t)maxsize) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_STRING_TOO_LONG);
+ ERR_add_error_dataf("maxsize=%ld", maxsize);
+ return -1;
+ }
}
- char strbuf[32];
if (minsize > 0 && nchar < (size_t)minsize) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_STRING_TOO_SHORT);
- BIO_snprintf(strbuf, sizeof strbuf, "%ld", minsize);
- ERR_add_error_data(2, "minsize=", strbuf);
- return -1;
- }
-
- if (maxsize > 0 && nchar > (size_t)maxsize) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_STRING_TOO_LONG);
- BIO_snprintf(strbuf, sizeof strbuf, "%ld", maxsize);
- ERR_add_error_data(2, "maxsize=", strbuf);
+ ERR_add_error_dataf("minsize=%ld", minsize);
return -1;
}
diff --git a/crypto/x509/asn1_gen.c b/crypto/x509/asn1_gen.c
index 3704808..989deee 100644
--- a/crypto/x509/asn1_gen.c
+++ b/crypto/x509/asn1_gen.c
@@ -79,6 +79,11 @@
// ASN1_GEN_MAX_DEPTH is the maximum number of nested TLVs allowed.
#define ASN1_GEN_MAX_DEPTH 50
+// ASN1_GEN_MAX_OUTPUT is the maximum output, in bytes, allowed. This limit is
+// necessary because the SEQUENCE and SET section reference mechanism allows the
+// output length to grow super-linearly with the input length.
+#define ASN1_GEN_MAX_OUTPUT (64 * 1024)
+
// ASN1_GEN_FORMAT_* are the values for the format modifiers.
#define ASN1_GEN_FORMAT_ASCII 1
#define ASN1_GEN_FORMAT_UTF8 2
@@ -105,6 +110,15 @@
return NULL;
}
+ // While not strictly necessary to avoid a DoS (we rely on any super-linear
+ // checks being performed internally), cap the overall output to
+ // |ASN1_GEN_MAX_OUTPUT| so the externally-visible behavior is consistent.
+ if (CBB_len(&cbb) > ASN1_GEN_MAX_OUTPUT) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_TOO_LONG);
+ CBB_cleanup(&cbb);
+ return NULL;
+ }
+
const uint8_t *der = CBB_data(&cbb);
ASN1_TYPE *ret = d2i_ASN1_TYPE(NULL, &der, CBB_len(&cbb));
CBB_cleanup(&cbb);
@@ -446,9 +460,14 @@
return 0;
}
+ // |maxsize| is measured in code points, rather than bytes, but pass it in
+ // as a loose cap so fuzzers can exit from excessively long inputs
+ // earlier. This limit is not load-bearing because |ASN1_mbstring_ncopy|'s
+ // output is already linear in the input.
ASN1_STRING *obj = NULL;
- if (ASN1_mbstring_copy(&obj, (const uint8_t *)value, -1, encoding,
- ASN1_tag2bit(type)) <= 0) {
+ if (ASN1_mbstring_ncopy(&obj, (const uint8_t *)value, -1, encoding,
+ ASN1_tag2bit(type), /*minsize=*/0,
+ /*maxsize=*/ASN1_GEN_MAX_OUTPUT) <= 0) {
return 0;
}
int ok = CBB_add_bytes(&child, obj->data, obj->length) && CBB_flush(cbb);
@@ -522,6 +541,13 @@
ASN1_GEN_FORMAT_ASCII, depth + 1)) {
return 0;
}
+ // This recursive call, by referencing |section|, is the one place
+ // where |generate_v3|'s output can be super-linear in the input.
+ // Check bounds here.
+ if (CBB_len(&child) > ASN1_GEN_MAX_OUTPUT) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_TOO_LONG);
+ return 0;
+ }
}
}
if (type == CBS_ASN1_SET) {
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 96c80b3..e496d82 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -6293,6 +6293,65 @@
0x20, 0x9d, 0x61, 0xb1, 0x9d, 0xef, 0xfd, 0x5a, 0x60, 0xba, 0x84,
0x4a, 0xf4, 0x92, 0xec, 0x2c, 0xc4, 0x44, 0x49, 0xc5, 0x69, 0x7b,
0x32, 0x69, 0x19, 0x70, 0x3b, 0xac, 0x03, 0x1c, 0xae, 0x7f, 0x60}},
+
+ // Sections can be referenced multiple times.
+ {kTestOID,
+ "ASN1:SEQUENCE:seq1",
+ R"(
+[seq1]
+val1 = SEQUENCE:seq2
+val2 = SEQUENCE:seq2
+[seq2]
+val1 = INT:1
+val2 = INT:2
+)",
+ {0x30, 0x22, 0x06, 0x0c, 0x2a, 0x86, 0x48, 0x86, 0xf7,
+ 0x12, 0x04, 0x01, 0x84, 0xb7, 0x09, 0x02, 0x04, 0x12,
+ 0x30, 0x10, 0x30, 0x06, 0x02, 0x01, 0x01, 0x02, 0x01,
+ 0x02, 0x30, 0x06, 0x02, 0x01, 0x01, 0x02, 0x01, 0x02}},
+
+ // But we cap this before it blows up exponentially.
+ {kTestOID,
+ "ASN1:SEQ:seq1",
+ R"(
+[seq1]
+val1 = SEQ:seq2
+val2 = SEQ:seq2
+[seq2]
+val1 = SEQ:seq3
+val2 = SEQ:seq3
+[seq3]
+val1 = SEQ:seq4
+val2 = SEQ:seq4
+[seq4]
+val1 = SEQ:seq5
+val2 = SEQ:seq5
+[seq5]
+val1 = SEQ:seq6
+val2 = SEQ:seq6
+[seq6]
+val1 = SEQ:seq7
+val2 = SEQ:seq7
+[seq7]
+val1 = SEQ:seq8
+val2 = SEQ:seq8
+[seq8]
+val1 = SEQ:seq9
+val2 = SEQ:seq9
+[seq9]
+val1 = SEQ:seq10
+val2 = SEQ:seq10
+[seq10]
+val1 = SEQ:seq11
+val2 = SEQ:seq11
+[seq11]
+val1 = SEQ:seq12
+val2 = SEQ:seq12
+[seq12]
+val1 = IA5:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+val2 = IA5:BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB
+)",
+ {}},
};
for (const auto &t : kTests) {
SCOPED_TRACE(t.name);