Reject [UNIVERSAL 0] in DER/BER element parsers.
[UNIVERSAL 0] is reserved by X.680 for the encoding to use. BER uses
this to encode indefinite-length EOCs, but it is possible to encode it
in a definite-length element or in a non-EOC form (non-zero length, or
constructed).
Whether we accept such encodings is normally moot: parsers will reject
the tag as unsuitable for the type. However, the ANY type matches all
tags. Previously, we would allow this, but crypto/asn1 has some ad-hoc
checks for unexpected EOCs, in some contexts, but not others.
Generalize this check to simply rejecting [UNIVERSAL 0] in all forms.
This avoids a weird hole in the abstraction where tags are sometimes
representable in BER and sometimes not. It also means we'll preserve
this check when migrating parsers from crypto/asn1.
Update-Note: There are two kinds of impacts I might expect from this
change. The first is BER parsers might be relying on the CBS DER/BER
element parser to pick up EOCs, as our ber.c does. This should be caught
by the most basic unit test and can be fixed by detecting EOCs
externally.
The second is code might be trying to parse "actual" elements with tag
[UNIVERSAL 0]. No actual types use this tag, so any non-ANY field is
already rejecting such inputs. However, it is possible some input has
this tag in a field with type ANY. This CL will cause us to reject that
input. Note, however, that crypto/asn1 already rejects unexpected EOCs
inside sequences, so many cases were already rejected anyway. Such
inputs are also invalid as the ANY should match some actual, unknown
ASN.1 type, and that type cannot use the reserved tag.
Fixed: 455
Change-Id: If42cacc01840439059baa0e67179d0f198234fc4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52245
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 7d69889..6e8a1a9 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -2075,6 +2075,47 @@
sizeof(kIndefinite)));
}
+template <typename T>
+void ExpectNoParse(T *(*d2i)(T **, const uint8_t **, long),
+ const std::vector<uint8_t> &in) {
+ SCOPED_TRACE(Bytes(in));
+ const uint8_t *ptr = in.data();
+ bssl::UniquePtr<T> obj(d2i(nullptr, &ptr, in.size()));
+ EXPECT_FALSE(obj);
+}
+
+// The zero tag, constructed or primitive, is reserved and should rejected by
+// the parser.
+TEST(ASN1Test, ZeroTag) {
+ ExpectNoParse(d2i_ASN1_TYPE, {0x00, 0x00});
+ ExpectNoParse(d2i_ASN1_TYPE, {0x00, 0x10, 0x00});
+ ExpectNoParse(d2i_ASN1_TYPE, {0x20, 0x00});
+ ExpectNoParse(d2i_ASN1_TYPE, {0x20, 0x00});
+ ExpectNoParse(d2i_ASN1_SEQUENCE_ANY, {0x30, 0x02, 0x00, 0x00});
+ ExpectNoParse(d2i_ASN1_SET_ANY, {0x31, 0x02, 0x00, 0x00});
+ // SEQUENCE {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.1 }
+ // [UNIVERSAL 0 PRIMITIVE] {}
+ // }
+ ExpectNoParse(d2i_X509_ALGOR,
+ {0x30, 0x10, 0x06, 0x0c, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12,
+ 0x04, 0x01, 0x84, 0xb7, 0x09, 0x01, 0x00, 0x00});
+ // SEQUENCE {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.1 }
+ // [UNIVERSAL 0 CONSTRUCTED] {}
+ // }
+ ExpectNoParse(d2i_X509_ALGOR,
+ {0x30, 0x10, 0x06, 0x0c, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12,
+ 0x04, 0x01, 0x84, 0xb7, 0x09, 0x01, 0x20, 0x00});
+ // SEQUENCE {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.1 }
+ // [UNIVERSAL 0 PRIMITIVE] { "a" }
+ // }
+ ExpectNoParse(d2i_X509_ALGOR,
+ {0x30, 0x11, 0x06, 0x0c, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12,
+ 0x04, 0x01, 0x84, 0xb7, 0x09, 0x01, 0x00, 0x01, 0x61});
+}
+
// 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)
diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c
index beb9a0b..c2b52eb 100644
--- a/crypto/asn1/tasn_dec.c
+++ b/crypto/asn1/tasn_dec.c
@@ -74,8 +74,6 @@
*/
#define ASN1_MAX_CONSTRUCTED_NEST 30
-static int asn1_check_eoc(const unsigned char **in, long len);
-
static int asn1_check_tlen(long *olen, int *otag, unsigned char *oclass,
char *cst, const unsigned char **in, long len,
int exptag, int expclass, char opt, ASN1_TLC *ctx);
@@ -373,13 +371,6 @@
if (!len)
break;
q = p;
- /* TODO(https://crbug.com/boringssl/455): Although we've removed
- * indefinite-length support, this check is not quite a no-op.
- * Reject [UNIVERSAL 0] in the tag parsers themselves. */
- if (asn1_check_eoc(&p, len)) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_UNEXPECTED_EOC);
- goto err;
- }
/*
* This determines the OPTIONAL flag value. The field cannot be
* omitted if it is the last of a SEQUENCE and there is still
@@ -592,13 +583,6 @@
while (len > 0) {
ASN1_VALUE *skfield;
const unsigned char *q = p;
- /* TODO(https://crbug.com/boringssl/455): Although we've removed
- * indefinite-length support, this check is not quite a no-op.
- * Reject [UNIVERSAL 0] in the tag parsers themselves. */
- if (asn1_check_eoc(&p, len)) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_UNEXPECTED_EOC);
- goto err;
- }
skfield = NULL;
if (!asn1_item_ex_d2i(&skfield, &p, len, ASN1_ITEM_ptr(tt->item),
-1, 0, 0, ctx, depth)) {
@@ -868,21 +852,6 @@
return ret;
}
-/* Check for ASN1 EOC and swallow it if found */
-
-static int asn1_check_eoc(const unsigned char **in, long len)
-{
- const unsigned char *p;
- if (len < 2)
- return 0;
- p = *in;
- if (!p[0] && !p[1]) {
- *in += 2;
- return 1;
- }
- return 0;
-}
-
/*
* Check an ASN1 tag and length: a bit like ASN1_get_object but it handles
* the ASN1_TLC cache and checks the expected tag.
diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
index d9b780f..dc707b9 100644
--- a/crypto/bytestring/ber.c
+++ b/crypto/bytestring/ber.c
@@ -93,11 +93,14 @@
return 1;
}
-// is_eoc returns true if |header_len| and |contents|, as returned by
-// |CBS_get_any_ber_asn1_element|, indicate an "end of contents" (EOC) value.
-static char is_eoc(size_t header_len, CBS *contents) {
- return header_len == 2 && CBS_len(contents) == 2 &&
- OPENSSL_memcmp(CBS_data(contents), "\x00\x00", 2) == 0;
+// cbs_get_eoc returns one if |cbs| begins with an "end of contents" (EOC) value
+// and zero otherwise. If an EOC was found, it advances |cbs| past it.
+static int cbs_get_eoc(CBS *cbs) {
+ if (CBS_len(cbs) >= 2 &&
+ CBS_data(cbs)[0] == 0 && CBS_data(cbs)[1] == 0) {
+ return CBS_skip(cbs, 2);
+ }
+ return 0;
}
// cbs_convert_ber reads BER data from |in| and writes DER data to |out|. If
@@ -116,21 +119,20 @@
}
while (CBS_len(in) > 0) {
+ if (looking_for_eoc && cbs_get_eoc(in)) {
+ return 1;
+ }
+
CBS contents;
unsigned tag, child_string_tag = string_tag;
size_t header_len;
int indefinite;
CBB *out_contents, out_contents_storage;
-
if (!CBS_get_any_ber_asn1_element(in, &contents, &tag, &header_len,
/*out_ber_found=*/NULL, &indefinite)) {
return 0;
}
- if (is_eoc(header_len, &contents)) {
- return looking_for_eoc;
- }
-
if (string_tag != 0) {
// This is part of a constructed string. All elements must match
// |string_tag| up to the constructed bit and get appended to |out|
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 77261a3..a8c1913 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -699,7 +699,6 @@
static const BERTest kBERTests[] = {
// Trivial cases, also valid DER.
- {"0000", true, false, false, 0},
{"0100", true, false, false, 1},
{"020101", true, false, false, 2},
@@ -725,6 +724,12 @@
{"1f4000", true, false, false, 0x40},
// Non-minimal tags are invalid, even in BER.
{"1f804000", false, false, false, 0},
+
+ // EOCs and other forms of tag [UNIVERSAL 0] are rejected as elements.
+ {"0000", false, false, false, 0},
+ {"000100", false, false, false, 0},
+ {"00800000", false, false, false, 0},
+ {"2000", false, false, false, 0},
};
TEST(CBSTest, BERElementTest) {
diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c
index 293e66c..010897b 100644
--- a/crypto/bytestring/cbs.c
+++ b/crypto/bytestring/cbs.c
@@ -279,6 +279,13 @@
tag |= tag_number;
+ // Tag [UNIVERSAL 0] is reserved for use by the encoding. Reject it here to
+ // avoid some ambiguity around ANY values and BER indefinite-length EOCs. See
+ // https://crbug.com/boringssl/455.
+ if ((tag & ~CBS_ASN1_CONSTRUCTED) == 0) {
+ return 0;
+ }
+
*out = tag;
return 1;
}
diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h
index 199d89c..68c1ba4 100644
--- a/include/openssl/bytestring.h
+++ b/include/openssl/bytestring.h
@@ -265,6 +265,10 @@
// also true for empty elements so |*out_indefinite| should be checked). If
// |out_ber_found| is not NULL then it is set to one if any case of invalid DER
// but valid BER is found, and to zero otherwise.
+//
+// This function will not successfully parse an end-of-contents (EOC) as an
+// element. Callers parsing indefinite-length encoding must check for EOC
+// separately.
OPENSSL_EXPORT int CBS_get_any_ber_asn1_element(CBS *cbs, CBS *out,
unsigned *out_tag,
size_t *out_header_len,
