Allow ASN1_get_object to parse indefinite and universal (#1994)

Loosen the restrictions on ASN1 parsing on calls made to
`ASN1_get_object`. Other places that parse ASN1 should be unaffected.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

crypto/asn1/asn1_lib.c

@@ -65,6 +65,7 @@
 
 #include "../internal.h"
 #include "internal.h"
+#include "../bytestring/internal.h"
 
 
 // Cross-module errors from crypto/x509/i2d_pr.c.
@@ -113,8 +114,9 @@ OPENSSL_DECLARE_ERROR_REASON(ASN1, UNSUPPORTED_TYPE)
 
 static void asn1_put_length(unsigned char **pp, int length);
 
-int ASN1_get_object(const unsigned char **inp, long *out_len, int *out_tag,
-                    int *out_class, long in_len) {
+int asn1_get_object_maybe_indefinite(const unsigned char **inp, long *out_len,
+                                     int *out_tag, int *out_class, long in_len,
+                                     int indefinite_ok) {
   if (in_len < 0) {
     OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG);
     return 0x80;
@@ -131,9 +133,12 @@ int ASN1_get_object(const unsigned char **inp, long *out_len, int *out_tag,
   int indefinite;
   CBS cbs, body;
   CBS_init(&cbs, *inp, (size_t)in_len);
-  if (!CBS_get_any_ber_asn1_element(&cbs, &body, &tag, &header_len,
-                                    /*out_ber_found=*/NULL, &indefinite) ||
-      indefinite || !CBS_skip(&body, header_len) ||
+
+  int ber_found_temp;
+  if (!cbs_get_any_asn1_element(&cbs, &body, &tag, &header_len, &ber_found_temp,
+                                &indefinite, /*ber_ok=*/1,
+                                /*universal_tag_ok=*/indefinite_ok) ||
+      (indefinite && !indefinite_ok) || !CBS_skip(&body, header_len) ||
       // Bound the length to comfortably fit in an int. Lengths in this
       // module often switch between int and long without overflow checks.
       CBS_len(&body) > INT_MAX / 2) {
@@ -143,22 +148,32 @@ int ASN1_get_object(const unsigned char **inp, long *out_len, int *out_tag,
 
   // Convert between tag representations.
   int tag_class = (tag & CBS_ASN1_CLASS_MASK) >> CBS_ASN1_TAG_SHIFT;
-  int constructed = (tag & CBS_ASN1_CONSTRUCTED) >> CBS_ASN1_TAG_SHIFT;
+  int constructed = (indefinite ? 1 : 0) |
+                    ((tag & CBS_ASN1_CONSTRUCTED) >> CBS_ASN1_TAG_SHIFT);
   int tag_number = tag & CBS_ASN1_TAG_NUMBER_MASK;
 
   // To avoid ambiguity with V_ASN1_NEG, impose a limit on universal tags.
   if (tag_class == V_ASN1_UNIVERSAL && tag_number > V_ASN1_MAX_UNIVERSAL) {
+    // Ruby's OpenSSL::test_basic_asn1data has an assertion that violates this
     OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG);
     return 0x80;
   }
 
   *inp = CBS_data(&body);
+  // If |indefinite|, |header_len| and |CBS_len(&body)| were equal prior to the
+  // |CBS_skip(&body, header_len)| call above and |CBS_len(&body)| is now zero.
   *out_len = CBS_len(&body);
   *out_tag = tag_number;
   *out_class = tag_class;
   return constructed;
 }
 
+int ASN1_get_object(const unsigned char **inp, long *out_len, int *out_tag,
+                    int *out_class, long in_len) {
+  return asn1_get_object_maybe_indefinite(inp, out_len, out_tag, out_class, in_len,
+    /*indefinite_ok=*/1);
+}
+
 // class 0 is constructed constructed == 2 for indefinite length constructed
 void ASN1_put_object(unsigned char **pp, int constructed, int length, int tag,
                      int xclass) {

crypto/asn1/asn1_test.cc

@@ -2291,20 +2291,69 @@ TEST(ASN1Test, PrintASN1Object) {
             std::string(reinterpret_cast<const char *>(bio_data), bio_len));
 }
 
-TEST(ASN1Test, GetObject) {
-  // The header is valid, but there are not enough bytes for the length.
-  static const uint8_t kTruncated[] = {0x30, 0x01};
-  const uint8_t *ptr = kTruncated;
+const struct GetObjectTestData {
+  std::vector<uint8_t> in;
+  const int expected_return;
+  const int expected_tag;
+  const int expected_class;
+  const int expected_length;
+} kGetObjectTests[] = {
+    // OpenSSL succeeds for all test cases below.
+    // The header is valid, but there are not enough bytes for the length.
+    {{0x30, 0x01}, 0x80, 0, 0, 0},
+    {{0x30, 0x80, 0x00, 0x00}, 0x21, 0x10, 0, 0},
+    {{0x00, 0x00}, 0x00, 0x00, 0x00, 0},
+    {{0x01, 0x00}, 0x00, 0x01, 0x00, 0},
+    {{0x41, 0x00}, 0x00, 0x01, 0x40, 0},
+    {{0x81, 0x00}, 0x00, 0x01, 0x80, 0},
+    {{0xC1, 0x00}, 0x00, 0x01, 0xC0, 0},
+    {{0x1F, 0x20, 0x00}, 0x00, 0x20, 0x00, 0},
+      // Rejected to avoid ambiguity with V_ASN1_NEG. Ruby has a test case expecting this to succeed.
+    {{0x1F, 0xC0, 0x20, 0x00}, 0x80, 0x00, 0x00, 0},
+    {{0x41, 0x02, 0xAB, 0xCD}, 0x00, 0x01, 0x40, 2},
+    {{0x61, 0x00}, 0x20, 0x01, 0x40, 0},
+    {{0x61, 0x80, 0xC2, 0x02, 0xAB, 0xCD, 0x00, 0x00}, 0x21, 0x01, 0x40, 0},
+};
+
+static void verifyGetObject(GetObjectTestData t) {
   long length;
   int tag;
   int tag_class;
-  EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class,
-                                  sizeof(kTruncated)));
 
-  static const uint8_t kIndefinite[] = {0x30, 0x80, 0x00, 0x00};
-  ptr = kIndefinite;
-  EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class,
-                                  sizeof(kIndefinite)));
+  SCOPED_TRACE(Bytes(t.in));
+  const uint8_t *ptr = t.in.data();
+  EXPECT_EQ(t.expected_return,
+            ASN1_get_object(&ptr, &length, &tag, &tag_class, t.in.size()));
+  if (!(t.expected_return & 0x80)) {
+    EXPECT_EQ(t.expected_length, length);
+    EXPECT_EQ(t.expected_tag, tag);
+    EXPECT_EQ(t.expected_class, tag_class);
+  }
+}
+
+TEST(ASN1Test, GetObject) {
+  for (const auto &t : kGetObjectTests) {
+    verifyGetObject(t);
+  }
+
+  {
+    GetObjectTestData test_case{ {0x41, 0x81, 0x80}, 0x00, 0x01, 0x40, 128 };
+    for(int i = 0; i < 64; i++) {
+      test_case.in.push_back(0xAB);
+      test_case.in.push_back(0xCD);
+    }
+    verifyGetObject(test_case);
+  }
+
+  {
+    GetObjectTestData test_case{ {0x41, 0x82, 0x01, 0x00}, 0x00, 0x01, 0x40, 256 };
+    for(int i = 0; i < 128; i++) {
+      test_case.in.push_back(0xAB);
+      test_case.in.push_back(0xCD);
+    }
+    verifyGetObject(test_case);
+  }
+
 }
 
 template <typename T>

crypto/asn1/internal.h

@@ -217,6 +217,21 @@ void asn1_type_cleanup(ASN1_TYPE *a);
 // ASN.1 PrintableString, and zero otherwise.
 int asn1_is_printable(uint32_t value);
 
+// asn1_get_object_maybe_indefinite parses an ASN.1 header, including tag, class,
+// and length information. The tag number is written to |*out_tag|. The class is
+// written to |*out_class|. If the tag is not indefinite, the content length is
+// written to |*out_len|. |inp| is advanced past the header in the input buffer.
+//
+// If |indefinite_ok| is non-zero, indefinite-length encoding and universal tags
+// are allowed, otherwise these will produce errors.
+//
+// The return value may have the following bits set:
+//   * 0x80: error occurred while parsing.
+//   * 0x20: the encoding is constructed, not primitive.
+//   * 0x01: indefinite-length constructed encoding.
+int asn1_get_object_maybe_indefinite(const unsigned char **inp, long *out_len, int *out_tag,
+                        int *out_class, long in_len, int indefinite_ok);
+
 // asn1_bit_string_length returns the number of bytes in |str| and sets
 // |*out_padding_bits| to the number of padding bits.
 //

crypto/asn1/tasn_dec.c

@@ -867,7 +867,7 @@ static int asn1_check_tlen(long *olen, int *otag, unsigned char *oclass,
   const unsigned char *p;
   p = *in;
 
-  i = ASN1_get_object(&p, &plen, &ptag, &pclass, len);
+  i = asn1_get_object_maybe_indefinite(&p, &plen, &ptag, &pclass, len, /*indefinite_ok=*/0);
   if (i & 0x80) {
     OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_OBJECT_HEADER);
     return 0;

crypto/bytestring/cbs.c

@@ -277,7 +277,7 @@ static int parse_base128_integer(CBS *cbs, uint64_t *out) {
   return 1;
 }
 
-static int parse_asn1_tag(CBS *cbs, CBS_ASN1_TAG *out) {
+static int parse_asn1_tag(CBS *cbs, CBS_ASN1_TAG *out, int universal_tag_ok) {
   uint8_t tag_byte;
   if (!CBS_get_u8(cbs, &tag_byte)) {
     return 0;
@@ -308,17 +308,17 @@ static int parse_asn1_tag(CBS *cbs, CBS_ASN1_TAG *out) {
   // 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) {
+  if (!universal_tag_ok && (tag & ~CBS_ASN1_CONSTRUCTED) == 0) {
     return 0;
   }
 
   *out = tag;
   return 1;
 }
 
-static int cbs_get_any_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
+int cbs_get_any_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
                                     size_t *out_header_len, int *out_ber_found,
-                                    int *out_indefinite, int ber_ok) {
+                                    int *out_indefinite, int ber_ok, int universal_tag_ok) {
   CBS header = *cbs;
   CBS throwaway;
 
@@ -334,7 +334,7 @@ static int cbs_get_any_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
   }
 
   CBS_ASN1_TAG tag;
-  if (!parse_asn1_tag(&header, &tag)) {
+  if (!parse_asn1_tag(&header, &tag, universal_tag_ok)) {
     return 0;
   }
   if (out_tag != NULL) {
@@ -434,7 +434,7 @@ int CBS_get_any_asn1(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag) {
 int CBS_get_any_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
                              size_t *out_header_len) {
   return cbs_get_any_asn1_element(cbs, out, out_tag, out_header_len, NULL, NULL,
-                                  /*ber_ok=*/0);
+                                  /*ber_ok=*/0, /*universal_tag_ok=*/0);
 }
 
 int CBS_get_any_ber_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
@@ -444,7 +444,7 @@ int CBS_get_any_ber_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
   return cbs_get_any_asn1_element(
       cbs, out, out_tag, out_header_len,
       out_ber_found ? out_ber_found : &ber_found_temp, out_indefinite,
-      /*ber_ok=*/1);
+      /*ber_ok=*/1, /*universal_tag_ok=*/0);
 }
 
 static int cbs_get_asn1(CBS *cbs, CBS *out, CBS_ASN1_TAG tag_value,
@@ -481,7 +481,7 @@ int CBS_get_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG tag_value) {
 int CBS_peek_asn1_tag(const CBS *cbs, CBS_ASN1_TAG tag_value) {
   CBS copy = *cbs;
   CBS_ASN1_TAG actual_tag;
-  return parse_asn1_tag(&copy, &actual_tag) && tag_value == actual_tag;
+  return parse_asn1_tag(&copy, &actual_tag, /*universal_tag_ok=*/0) && tag_value == actual_tag;
 }
 
 int CBS_get_asn1_uint64(CBS *cbs, uint64_t *out) {

crypto/bytestring/internal.h

@@ -88,6 +88,26 @@ OPENSSL_EXPORT int cbb_add_latin1(CBB *cbb, uint32_t u);
 OPENSSL_EXPORT int cbb_add_ucs2_be(CBB *cbb, uint32_t u);
 OPENSSL_EXPORT int cbb_add_utf32_be(CBB *cbb, uint32_t u);
 
+// cbs_get_any_asn1_element parses an ASN.1 element from |cbs|. |*out_indefinite|
+// is set to one if the length was indefinite and zero otherwise. On success,
+// if the length is indefinite |out| will only contain the ASN.1 header,
+// otherwise is will contain both the header and the content. If |out_tag| is
+// not NULL, |*out_tag| is set to the element's tag number. If |out_header_len|
+// is not NULL, |*out_header_len| is set to the length of the header.
+//
+// If |ber_ok| is one, BER encoding is permitted. In this case, if
+// |out_ber_found| is not NULL and BER-specific encoding was found,
+// |*out_ber_found| is set to one. If |out_indefinite| is not NULL and the
+// element has indefinite-length, |*out_indefinite| is set to one.
+// If |ber_ok| is zero, both |out_ber_found| and |out_indefinite| must be NULL.
+//
+// If |universal_tag_ok| is 1, universal tags are permitted. Otherwise, only
+// context-specific tags are accepted.
+//
+// It returns one on success and zero on failure.
+int cbs_get_any_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
+                                      size_t *out_header_len, int *out_ber_found,
+                                      int *out_indefinite, int ber_ok, int universal_tag_ok);
 
 #if defined(__cplusplus)
 }  // extern C