Parse BER for PKCS#12 more accurately.

CBS_asn1_ber_to_der currently uses heuristics because implicitly-tagged
constructed strings in BER are ambiguous with implicitly-tagged sequences. It's
not possible to convert BER to DER without knowing the schema.

Fortunately, implicitly tagged strings don't appear often so instead split the
job up: CBS_asn1_ber_to_der fixes indefinite-length elements and constructed
strings it can see. Implicitly-tagged strings it leaves uncoverted, but they
will only nest one level down (because BER kindly allows one to nest
constructed strings arbitrarily!).

CBS_get_asn1_implicit_string then performs the final concatenation at parse
time. This isn't much more complex and lets us parse BER more accurately and
also reject a number of mis-encoded values (e.g. constructed INTEGERs are not a
thing) we'd previously let through. The downside is the post-conversion parsing
code must be aware of this limitation of CBS_asn1_ber_to_der. Fortunately,
there's only one implicitly-tagged string in our PKCS#12 code.

(In the category of things that really really don't matter, but I had spare
cycles and the old BER converter is weird.)

Change-Id: Iebdd13b08559fa158b308ef83a5bb07bfdf80ae8
Reviewed-on: https://boringssl-review.googlesource.com/7052
Reviewed-by: Adam Langley <agl@google.com>

4 files changed, 224 insertions, 96 deletions

diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
index 6f7d1077..2a968e16 100644
--- a/crypto/bytestring/ber.c
+++ b/crypto/bytestring/ber.c
@@ -14,6 +14,7 @@
 
 #include <openssl/bytestring.h>
 
+#include <assert.h>
 #include <string.h>
 
 #include "internal.h"
@@ -24,11 +25,36 @@
  * input could otherwise cause the stack to overflow. */
 static const unsigned kMaxDepth = 2048;
 
+/* is_string_type returns one if |tag| is a string type and zero otherwise. It
+ * ignores the constructed bit. */
+static int is_string_type(unsigned tag) {
+  if ((tag & 0xc0) != 0) {
+    return 0;
+  }
+  switch (tag & 0x1f) {
+    case CBS_ASN1_BITSTRING:
+    case CBS_ASN1_OCTETSTRING:
+    case CBS_ASN1_NUMERICSTRING:
+    case CBS_ASN1_PRINTABLESTRING:
+    case CBS_ASN1_T16STRING:
+    case CBS_ASN1_VIDEOTEXSTRING:
+    case CBS_ASN1_IA5STRING:
+    case CBS_ASN1_GRAPHICSTRING:
+    case CBS_ASN1_VISIBLESTRING:
+    case CBS_ASN1_GENERALSTRING:
+    case CBS_ASN1_UNIVERSALSTRING:
+    case CBS_ASN1_BMPSTRING:
+      return 1;
+    default:
+      return 0;
+  }
+}
+
 /* cbs_find_ber walks an ASN.1 structure in |orig_in| and sets |*ber_found|
- * depending on whether an indefinite length element was found. The value of
- * |in| is not changed. It returns one on success (i.e. |*ber_found| was set)
- * and zero on error. */
-static int cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) {
+ * depending on whether an indefinite length element or constructed string was
+ * found. The value of |orig_in| is not changed. It returns one on success (i.e.
+ * |*ber_found| was set) and zero on error. */
+static int cbs_find_ber(const CBS *orig_in, char *ber_found, unsigned depth) {
   CBS in;
 
   if (depth > kMaxDepth) {
@@ -49,10 +75,16 @@ static int cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) {
     if (CBS_len(&contents) == header_len &&
         header_len > 0 &&
         CBS_data(&contents)[header_len-1] == 0x80) {
+      /* Found an indefinite-length element. */
       *ber_found = 1;
       return 1;
     }
     if (tag & CBS_ASN1_CONSTRUCTED) {
+      if (is_string_type(tag)) {
+        /* Constructed strings are only legal in BER and require conversion. */
+        *ber_found = 1;
+        return 1;
+      }
       if (!CBS_skip(&contents, header_len) ||
           !cbs_find_ber(&contents, ber_found, depth + 1)) {
         return 0;
@@ -63,16 +95,6 @@ static int cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) {
   return 1;
 }
 
-/* is_primitive_type returns true if |tag| likely a primitive type. Normally
- * one can just test the "constructed" bit in the tag but, in BER, even
- * primitive tags can have the constructed bit if they have indefinite
- * length. */
-static char is_primitive_type(unsigned tag) {
-  return (tag & 0xc0) == 0 &&
-         (tag & 0x1f) != (CBS_ASN1_SEQUENCE & 0x1f) &&
-         (tag & 0x1f) != (CBS_ASN1_SET & 0x1f);
-}
-
 /* 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) {
@@ -81,92 +103,65 @@ static char is_eoc(size_t header_len, CBS *contents) {
 }
 
 /* cbs_convert_ber reads BER data from |in| and writes DER data to |out|. If
- * |squash_header| is set then the top-level of elements from |in| will not
- * have their headers written. This is used when concatenating the fragments of
- * an indefinite length, primitive value. If |looking_for_eoc| is set then any
- * EOC elements found will cause the function to return after consuming it.
- * It returns one on success and zero on error. */
-static int cbs_convert_ber(CBS *in, CBB *out, char squash_header,
+ * |string_tag| is non-zero, then all elements must match |string_tag| up to the
+ * constructed bit and primitive element bodies are written to |out| without
+ * element headers. This is used when concatenating the fragments of a
+ * constructed string. If |looking_for_eoc| is set then any EOC elements found
+ * will cause the function to return after consuming it. It returns one on
+ * success and zero on error. */
+static int cbs_convert_ber(CBS *in, CBB *out, unsigned string_tag,
                            char looking_for_eoc, unsigned depth) {
+  assert(!(string_tag & CBS_ASN1_CONSTRUCTED));
+
   if (depth > kMaxDepth) {
     return 0;
   }
 
   while (CBS_len(in) > 0) {
     CBS contents;
-    unsigned tag;
+    unsigned tag, child_string_tag = string_tag;
     size_t header_len;
     CBB *out_contents, out_contents_storage;
 
     if (!CBS_get_any_ber_asn1_element(in, &contents, &tag, &header_len)) {
       return 0;
     }
-    out_contents = out;
 
-    if (CBS_len(&contents) == header_len) {
-      if (is_eoc(header_len, &contents)) {
-        return looking_for_eoc;
-      }
+    if (is_eoc(header_len, &contents)) {
+      return looking_for_eoc;
+    }
 
-      if (header_len > 0 && CBS_data(&contents)[header_len - 1] == 0x80) {
-        /* This is an indefinite length element. If it's a SEQUENCE or SET then
-         * we just need to write the out the contents as normal, but with a
-         * concrete length prefix.
-         *
-         * If it's a something else then the contents will be a series of BER
-         * elements of the same type which need to be concatenated. */
-        const char context_specific = (tag & 0xc0) == 0x80;
-        char squash_child_headers = is_primitive_type(tag);
-
-        /* This is a hack, but it sufficies to handle NSS's output. If we find
-         * an indefinite length, context-specific tag with a definite, primitive
-         * tag inside it, then we assume that the context-specific tag is
-         * implicit and the tags within are fragments of a primitive type that
-         * need to be concatenated. */
-        if (context_specific && (tag & CBS_ASN1_CONSTRUCTED)) {
-          CBS in_copy, inner_contents;
-          unsigned inner_tag;
-          size_t inner_header_len;
-
-          CBS_init(&in_copy, CBS_data(in), CBS_len(in));
-          if (!CBS_get_any_ber_asn1_element(&in_copy, &inner_contents,
-                                            &inner_tag, &inner_header_len)) {
-            return 0;
-          }
-          if (CBS_len(&inner_contents) > inner_header_len &&
-              is_primitive_type(inner_tag)) {
-            squash_child_headers = 1;
-          }
-        }
-
-        if (!squash_header) {
-          unsigned out_tag = tag;
-          if (squash_child_headers) {
-            out_tag &= ~CBS_ASN1_CONSTRUCTED;
-          }
-          if (!CBB_add_asn1(out, &out_contents_storage, out_tag)) {
-            return 0;
-          }
-          out_contents = &out_contents_storage;
-        }
-
-        if (!cbs_convert_ber(in, out_contents,
-                             squash_child_headers,
-                             1 /* looking for eoc */, depth + 1)) {
-          return 0;
-        }
-        if (out_contents != out && !CBB_flush(out)) {
-          return 0;
-        }
-        continue;
+    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|
+       * without a child element. */
+      if ((tag & ~CBS_ASN1_CONSTRUCTED) != string_tag) {
+        return 0;
+      }
+      out_contents = out;
+    } else {
+      unsigned out_tag = tag;
+      if ((tag & CBS_ASN1_CONSTRUCTED) && is_string_type(tag)) {
+        /* If a constructed string, clear the constructed bit and inform
+         * children to concatenate bodies. */
+        out_tag &= ~CBS_ASN1_CONSTRUCTED;
+        child_string_tag = out_tag;
       }
+      if (!CBB_add_asn1(out, &out_contents_storage, out_tag)) {
+        return 0;
+      }
+      out_contents = &out_contents_storage;
     }
 
-    if (!squash_header) {
-      if (!CBB_add_asn1(out, &out_contents_storage, tag)) {
+    if (CBS_len(&contents) == header_len && header_len > 0 &&
+        CBS_data(&contents)[header_len - 1] == 0x80) {
+      /* This is an indefinite length element. */
+      if (!cbs_convert_ber(in, out_contents, child_string_tag,
+                           1 /* looking for eoc */, depth + 1) ||
+          !CBB_flush(out)) {
         return 0;
       }
-      out_contents = &out_contents_storage;
+      continue;
     }
 
     if (!CBS_skip(&contents, header_len)) {
@@ -174,18 +169,20 @@ static int cbs_convert_ber(CBS *in, CBB *out, char squash_header,
     }
 
     if (tag & CBS_ASN1_CONSTRUCTED) {
-      if (!cbs_convert_ber(&contents, out_contents, 0 /* don't squash header */,
+      /* Recurse into children. */
+      if (!cbs_convert_ber(&contents, out_contents, child_string_tag,
                            0 /* not looking for eoc */, depth + 1)) {
         return 0;
       }
     } else {
+      /* Copy primitive contents as-is. */
       if (!CBB_add_bytes(out_contents, CBS_data(&contents),
                          CBS_len(&contents))) {
         return 0;
       }
     }
 
-    if (out_contents != out && !CBB_flush(out)) {
+    if (!CBB_flush(out)) {
       return 0;
     }
   }
@@ -218,3 +215,48 @@ int CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len) {
 
   return 1;
 }
+
+int CBS_get_asn1_implicit_string(CBS *in, CBS *out, uint8_t **out_storage,
+                                 unsigned outer_tag, unsigned inner_tag) {
+  assert(!(outer_tag & CBS_ASN1_CONSTRUCTED));
+  assert(!(inner_tag & CBS_ASN1_CONSTRUCTED));
+  assert(is_string_type(inner_tag));
+
+  if (CBS_peek_asn1_tag(in, outer_tag)) {
+    /* Normal implicitly-tagged string. */
+    *out_storage = NULL;
+    return CBS_get_asn1(in, out, outer_tag);
+  }
+
+  /* Otherwise, try to parse an implicitly-tagged constructed string.
+   * |CBS_asn1_ber_to_der| is assumed to have run, so only allow one level deep
+   * of nesting. */
+  CBB result;
+  CBS child;
+  if (!CBB_init(&result, CBS_len(in)) ||
+      !CBS_get_asn1(in, &child, outer_tag | CBS_ASN1_CONSTRUCTED)) {
+    goto err;
+  }
+
+  while (CBS_len(&child) > 0) {
+    CBS chunk;
+    if (!CBS_get_asn1(&child, &chunk, inner_tag) ||
+        !CBB_add_bytes(&result, CBS_data(&chunk), CBS_len(&chunk))) {
+      goto err;
+    }
+  }
+
+  uint8_t *data;
+  size_t len;
+  if (!CBB_finish(&result, &data, &len)) {
+    goto err;
+  }
+
+  CBS_init(out, data, len);
+  *out_storage = data;
+  return 1;
+
+err:
+  CBB_cleanup(&result);
+  return 0;
+}
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 188c63d5..84ecffcd 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -579,7 +579,7 @@ static bool TestBerConvert() {
   static const uint8_t kIndefBER[] = {0x30, 0x80, 0x01, 0x01, 0x02, 0x00, 0x00};
   static const uint8_t kIndefDER[] = {0x30, 0x03, 0x01, 0x01, 0x02};
 
-  // kOctetStringBER contains an indefinite length OCTETSTRING with two parts.
+  // kOctetStringBER contains an indefinite length OCTET STRING with two parts.
   // These parts need to be concatenated in DER form.
   static const uint8_t kOctetStringBER[] = {0x24, 0x80, 0x04, 0x02, 0,    1,
                                             0x04, 0x02, 2,    3,    0x00, 0x00};
@@ -609,6 +609,16 @@ static bool TestBerConvert() {
       0x6e, 0x10, 0x9b, 0xb8, 0x02, 0x02, 0x07, 0xd0,
   };
 
+  // kConstructedStringBER contains a deeply-nested constructed OCTET STRING.
+  // The BER conversion collapses this to one level deep, but not completely.
+  static const uint8_t kConstructedStringBER[] = {
+      0xa0, 0x10, 0x24, 0x06, 0x04, 0x01, 0x00, 0x04, 0x01,
+      0x01, 0x24, 0x06, 0x04, 0x01, 0x02, 0x04, 0x01, 0x03,
+  };
+  static const uint8_t kConstructedStringDER[] = {
+      0xa0, 0x08, 0x04, 0x02, 0x00, 0x01, 0x04, 0x02, 0x02, 0x03,
+  };
+
   return DoBerConvert("kSimpleBER", kSimpleBER, sizeof(kSimpleBER),
                       kSimpleBER, sizeof(kSimpleBER)) &&
          DoBerConvert("kIndefBER", kIndefDER, sizeof(kIndefDER), kIndefBER,
@@ -617,7 +627,59 @@ static bool TestBerConvert() {
                       sizeof(kOctetStringDER), kOctetStringBER,
                       sizeof(kOctetStringBER)) &&
          DoBerConvert("kNSSBER", kNSSDER, sizeof(kNSSDER), kNSSBER,
-                      sizeof(kNSSBER));
+                      sizeof(kNSSBER)) &&
+         DoBerConvert("kConstructedStringBER", kConstructedStringDER,
+                      sizeof(kConstructedStringDER), kConstructedStringBER,
+                      sizeof(kConstructedStringBER));
+}
+
+struct ImplicitStringTest {
+  const char *in;
+  size_t in_len;
+  bool ok;
+  const char *out;
+  size_t out_len;
+};
+
+static const ImplicitStringTest kImplicitStringTests[] = {
+    // A properly-encoded string.
+    {"\x80\x03\x61\x61\x61", 5, true, "aaa", 3},
+    // An implicit-tagged string.
+    {"\xa0\x09\x04\x01\x61\x04\x01\x61\x04\x01\x61", 11, true, "aaa", 3},
+    // |CBS_get_asn1_implicit_string| only accepts one level deep of nesting.
+    {"\xa0\x0b\x24\x06\x04\x01\x61\x04\x01\x61\x04\x01\x61", 13, false, nullptr,
+     0},
+    // The outer tag must match.
+    {"\x81\x03\x61\x61\x61", 5, false, nullptr, 0},
+    {"\xa1\x09\x04\x01\x61\x04\x01\x61\x04\x01\x61", 11, false, nullptr, 0},
+    // The inner tag must match.
+    {"\xa1\x09\x0c\x01\x61\x0c\x01\x61\x0c\x01\x61", 11, false, nullptr, 0},
+};
+
+static bool TestImplicitString() {
+  for (const auto &test : kImplicitStringTests) {
+    uint8_t *storage = nullptr;
+    CBS in, out;
+    CBS_init(&in, reinterpret_cast<const uint8_t *>(test.in), test.in_len);
+    int ok = CBS_get_asn1_implicit_string(&in, &out, &storage,
+                                          CBS_ASN1_CONTEXT_SPECIFIC | 0,
+                                          CBS_ASN1_OCTETSTRING);
+    ScopedOpenSSLBytes scoper(storage);
+
+    if (static_cast<bool>(ok) != test.ok) {
+      fprintf(stderr, "CBS_get_asn1_implicit_string unexpectedly %s\n",
+              ok ? "succeeded" : "failed");
+      return false;
+    }
+
+    if (ok && (CBS_len(&out) != test.out_len ||
+               memcmp(CBS_data(&out), test.out, test.out_len) != 0)) {
+      fprintf(stderr, "CBS_get_asn1_implicit_string gave the wrong output\n");
+      return false;
+    }
+  }
+
+  return true;
 }
 
 struct ASN1Uint64Test {
@@ -747,6 +809,7 @@ int main(void) {
       !TestCBBDiscardChild() ||
       !TestCBBASN1() ||
       !TestBerConvert() ||
+      !TestImplicitString() ||
       !TestASN1Uint64() ||
       !TestGetOptionalASN1Bool() ||
       !TestZero() ||
diff --git a/crypto/bytestring/internal.h b/crypto/bytestring/internal.h
index b4ea7e51..f4f4b9cd 100644
--- a/crypto/bytestring/internal.h
+++ b/crypto/bytestring/internal.h
@@ -22,22 +22,42 @@ extern "C" {
 #endif
 
 
-/* CBS_asn1_ber_to_der reads an ASN.1 structure from |in|. If it finds
- * indefinite-length elements then it attempts to convert the BER data to DER
- * and sets |*out| and |*out_length| to describe a malloced buffer containing
- * the DER data. Additionally, |*in| will be advanced over the ASN.1 data.
+/* CBS_asn1_ber_to_der reads a BER element from |in|. If it finds
+ * indefinite-length elements or constructed strings then it converts the BER
+ * data to DER and sets |*out| and |*out_length| to describe a malloced buffer
+ * containing the DER data. Additionally, |*in| will be advanced over the BER
+ * element.
  *
- * If it doesn't find any indefinite-length elements then it sets |*out| to
- * NULL and |*in| is unmodified.
+ * If it doesn't find any indefinite-length elements or constructed strings then
+ * it sets |*out| to NULL and |*in| is unmodified.
  *
- * A sufficiently complex ASN.1 structure will break this function because it's
- * not possible to generically convert BER to DER without knowledge of the
- * structure itself. However, this sufficies to handle the PKCS#7 and #12 output
- * from NSS.
+ * This function should successfully process any valid BER input, however it
+ * will not convert all of BER's deviations from DER. BER is ambiguous between
+ * implicitly-tagged SEQUENCEs of strings and implicitly-tagged constructed
+ * strings. Implicitly-tagged strings must be parsed with
+ * |CBS_get_ber_implicitly_tagged_string| instead of |CBS_get_asn1|. The caller
+ * must also account for BER variations in the contents of a primitive.
  *
  * It returns one on success and zero otherwise. */
 OPENSSL_EXPORT int CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len);
 
+/* CBS_get_asn1_implicit_string parses a BER string of primitive type
+ * |inner_tag| implicitly-tagged with |outer_tag|. It sets |out| to the
+ * contents. If concatenation was needed, it sets |*out_storage| to a buffer
+ * which the caller must release with |OPENSSL_free|. Otherwise, it sets
+ * |*out_storage| to NULL.
+ *
+ * This function does not parse all of BER. It requires the string be
+ * definite-length. Constructed strings are allowed, but all children of the
+ * outermost element must be primitive. The caller should use
+ * |CBS_asn1_ber_to_der| before running this function.
+ *
+ * It returns one on success and zero otherwise. */
+OPENSSL_EXPORT int CBS_get_asn1_implicit_string(CBS *in, CBS *out,
+                                                uint8_t **out_storage,
+                                                unsigned outer_tag,
+                                                unsigned inner_tag);
+
 
 #if defined(__cplusplus)
 }  /* extern C */
diff --git a/crypto/pkcs8/pkcs8.c b/crypto/pkcs8/pkcs8.c
index ac13faf7..fdce544a 100644
--- a/crypto/pkcs8/pkcs8.c
+++ b/crypto/pkcs8/pkcs8.c
@@ -737,6 +737,7 @@ static int PKCS12_handle_content_info(CBS *content_info, unsigned depth,
                                       struct pkcs12_context *ctx) {
   CBS content_type, wrapped_contents, contents, content_infos;
   int nid, ret = 0;
+  uint8_t *storage = NULL;
 
   if (!CBS_get_asn1(content_info, &content_type, CBS_ASN1_OBJECT) ||
       !CBS_get_asn1(content_info, &wrapped_contents,
@@ -767,8 +768,9 @@ static int PKCS12_handle_content_info(CBS *content_info, unsigned depth,
         /* AlgorithmIdentifier, see
          * https://tools.ietf.org/html/rfc5280#section-4.1.1.2 */
         !CBS_get_asn1_element(&eci, &ai, CBS_ASN1_SEQUENCE) ||
-        !CBS_get_asn1(&eci, &encrypted_contents,
-                      CBS_ASN1_CONTEXT_SPECIFIC | 0)) {
+        !CBS_get_asn1_implicit_string(
+            &eci, &encrypted_contents, &storage,
+            CBS_ASN1_CONTEXT_SPECIFIC | 0, CBS_ASN1_OCTETSTRING)) {
       OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA);
       goto err;
     }
@@ -895,6 +897,7 @@ static int PKCS12_handle_content_info(CBS *content_info, unsigned depth,
   }
 
 err:
+  OPENSSL_free(storage);
   return ret;
 }