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 <[email protected]>
Commit-Queue: David Benjamin <[email protected]>

Author: davidben

crypto/asn1/asn1_test.cc

@@ -2075,6 +2075,47 @@ TEST(ASN1Test, GetObject) {
                                   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)

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 @@ static int asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in,
             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 @@ static int asn1_template_noexp_d2i(ASN1_VALUE **val,
         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 @@ static int asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len,
     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.

crypto/bytestring/ber.c

@@ -93,11 +93,14 @@ static int cbs_find_ber(const CBS *orig_in, int *ber_found, unsigned depth) {
   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 @@ static int cbs_convert_ber(CBS *in, CBB *out, unsigned string_tag,
   }
 
   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|

crypto/bytestring/bytestring_test.cc

@@ -699,7 +699,6 @@ struct BERTest {
 
 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 @@ static const BERTest kBERTests[] = {
     {"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) {

crypto/bytestring/cbs.c

@@ -279,6 +279,13 @@ static int parse_asn1_tag(CBS *cbs, unsigned *out) {
 
   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;
 }

include/openssl/bytestring.h

@@ -265,6 +265,10 @@ OPENSSL_EXPORT int CBS_get_any_asn1_element(CBS *cbs, CBS *out,
 // 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,