Make CBS_get_any_asn1_element() more compliant with DER encoding.

CBS_get_any_asn1_element violates DER encoding by allowing indefinite
form.  All callers except bs_ber.c expect DER encoding.  The callers
must check to see if it was indefinite or not.

Rather than exposing all callers to this behavior,
cbs_get_any_asn1_element_internal() allows specifying whether you want to
allow the normally forbidden indefinite form.  This is used by
CBS_get_any_asn1_element() for strict DER encoding and by a new static
function in bs_ber.c for the relaxed version.

While I was here, I added comments to differentiate between ASN.1
restrictions and CBS limitations.

ok miod@

6 files changed, 172 insertions, 56 deletions

diff --git a/src/lib/libssl/bs_ber.c b/src/lib/libssl/bs_ber.c
index 2ec91fc800..3d39def111 100644
--- a/src/lib/libssl/bs_ber.c
+++ b/src/lib/libssl/bs_ber.c
@@ -1,4 +1,4 @@
-/*      $OpenBSD: bs_ber.c,v 1.4 2015/04/29 02:11:09 doug Exp $ */
+/*      $OpenBSD: bs_ber.c,v 1.5 2015/06/15 07:35:49 doug Exp $ */
 /*
  * Copyright (c) 2014, Google Inc.
  *
@@ -27,6 +27,15 @@
  */
 static const unsigned kMaxDepth = 2048;
 
+/* Non-strict version that allows a relaxed DER with indefinite form. */
+static int
+cbs_nonstrict_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
+    size_t *out_header_len)
+{
+        return cbs_get_any_asn1_element_internal(cbs, out,
+            out_tag, out_header_len, 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
@@ -49,10 +58,11 @@ cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth)
                 unsigned tag;
                 size_t header_len;
 
-                if (!CBS_get_any_asn1_element(&in, &contents, &tag,
+                if (!cbs_nonstrict_get_any_asn1_element(&in, &contents, &tag,
                     &header_len))
                         return 0;
 
+                /* Indefinite form not allowed by DER. */
                 if (CBS_len(&contents) == header_len && header_len > 0 &&
                     CBS_data(&contents)[header_len - 1] == 0x80) {
                         *ber_found = 1;
@@ -84,7 +94,8 @@ is_primitive_type(unsigned tag)
 
 /*
  * is_eoc returns true if |header_len| and |contents|, as returned by
- * |CBS_get_any_asn1_element|, indicate an "end of contents" (EOC) value.
+ * |cbs_nonstrict_get_any_asn1_element|, indicate an "end of contents" (EOC)
+ * value.
  */
 static char
 is_eoc(size_t header_len, CBS *contents)
@@ -113,7 +124,8 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc,
                 size_t header_len;
                 CBB *out_contents, out_contents_storage;
 
-                if (!CBS_get_any_asn1_element(in, &contents, &tag, &header_len))
+                if (!cbs_nonstrict_get_any_asn1_element(in, &contents, &tag,
+                    &header_len))
                         return 0;
 
                 out_contents = out;
@@ -156,9 +168,9 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc,
 
                                         CBS_init(&in_copy, CBS_data(in),
                                             CBS_len(in));
-                                        if (!CBS_get_any_asn1_element(&in_copy,
-                                            &inner_contents, &inner_tag,
-                                            &inner_header_len))
+                                        if (!cbs_nonstrict_get_any_asn1_element(
+                                            &in_copy, &inner_contents,
+                                            &inner_tag, &inner_header_len))
                                                 return 0;
 
                                         if (CBS_len(&inner_contents) >
diff --git a/src/lib/libssl/bs_cbs.c b/src/lib/libssl/bs_cbs.c
index c37f81dd60..ba38303c18 100644
--- a/src/lib/libssl/bs_cbs.c
+++ b/src/lib/libssl/bs_cbs.c
@@ -1,4 +1,4 @@
-/*      $OpenBSD: bs_cbs.c,v 1.8 2015/06/13 08:46:00 doug Exp $ */
+/*      $OpenBSD: bs_cbs.c,v 1.9 2015/06/15 07:35:49 doug Exp $ */
 /*
  * Copyright (c) 2014, Google Inc.
  *
@@ -205,24 +205,45 @@ int
 CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
     size_t *out_header_len)
 {
+        return cbs_get_any_asn1_element_internal(cbs, out, out_tag,
+            out_header_len, 1);
+}
+
+/*
+ * Review X.690 for details on ASN.1 DER encoding.
+ *
+ * If non-strict mode is enabled, then DER rules are relaxed
+ * for indefinite constructs (violates DER but a little closer to BER).
+ * Non-strict mode should only be used by bs_ber.c
+ *
+ * Sections 8, 10 and 11 for DER encoding
+ */
+int
+cbs_get_any_asn1_element_internal(CBS *cbs, CBS *out, unsigned *out_tag,
+    size_t *out_header_len, int strict)
+{
         uint8_t tag, length_byte;
         CBS header = *cbs;
         CBS throwaway;
+        size_t len;
 
         if (out == NULL)
                 out = &throwaway;
 
+        /*
+         * Get identifier octet and length octet.  Only 1 octet for each
+         * is a CBS limitation.
+         */
         if (!CBS_get_u8(&header, &tag) || !CBS_get_u8(&header, &length_byte))
                 return 0;
 
+        /* CBS limitation: long form tags are not supported. */
         if ((tag & 0x1f) == 0x1f)
-                /* Long form tags are not supported. */
                 return 0;
 
         if (out_tag != NULL)
                 *out_tag = tag;
 
-        size_t len;
         if ((length_byte & 0x80) == 0) {
                 /* Short form length. */
                 len = ((size_t) length_byte) + 2;
@@ -234,21 +255,40 @@ CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
                 const size_t num_bytes = length_byte & 0x7f;
                 uint32_t len32;
 
-                if ((tag & CBS_ASN1_CONSTRUCTED) != 0 && num_bytes == 0) {
-                        /* indefinite length */
-                        if (out_header_len != NULL)
-                                *out_header_len = 2;
-                        return CBS_get_bytes(cbs, out, 2);
+                /* ASN.1 reserved value for future extensions */
+                if (num_bytes == 0x7f)
+                        return 0;
+
+                /* Handle indefinite form length */
+                if (num_bytes == 0) {
+                        /* DER encoding doesn't allow for indefinite form. */
+                        if (strict) {
+                                return 0;
+
+                        } else {
+                                if ((tag & CBS_ASN1_CONSTRUCTED) != 0 &&
+                                    num_bytes == 0) {
+                                        /* indefinite length */
+                                        if (out_header_len != NULL)
+                                                *out_header_len = 2;
+                                        return CBS_get_bytes(cbs, out, 2);
+                                } else {
+                                        /* Primitive cannot use indefinite. */
+                                        return 0;
+                                }
+                        }
                 }
 
-                if (num_bytes == 0 || num_bytes > 4)
+                /* CBS limitation. */
+                if (num_bytes > 4)
                         return 0;
 
                 if (!cbs_get_u(&header, &len32, num_bytes))
                         return 0;
 
+                /* DER has a minimum length octet requirements. */
                 if (len32 < 128)
-                        /* Length should have used short-form encoding. */
+                        /* Should have used short form instead */
                         return 0;
 
                 if ((len32 >> ((num_bytes - 1) * 8)) == 0)
@@ -279,13 +319,7 @@ cbs_get_asn1(CBS *cbs, CBS *out, unsigned tag_value, int skip_header)
                 out = &throwaway;
 
         if (!CBS_get_any_asn1_element(cbs, out, &tag, &header_len) ||
-            tag != tag_value || (header_len > 0 &&
-            /*
-             * This ensures that the tag is either zero length or
-             * indefinite-length.
-             */
-            CBS_len(out) == header_len &&
-            CBS_data(out)[header_len - 1] == 0x80))
+            tag != tag_value)
                 return 0;
 
         if (skip_header && !CBS_skip(out, header_len)) {
diff --git a/src/lib/libssl/bytestring.h b/src/lib/libssl/bytestring.h
index b98c930da5..d66ab65b91 100644
--- a/src/lib/libssl/bytestring.h
+++ b/src/lib/libssl/bytestring.h
@@ -1,4 +1,4 @@
-/*      $OpenBSD: bytestring.h,v 1.6 2015/06/13 09:02:45 doug Exp $     */
+/*      $OpenBSD: bytestring.h,v 1.7 2015/06/15 07:35:49 doug Exp $     */
 /*
  * Copyright (c) 2014, Google Inc.
  *
@@ -223,9 +223,8 @@ int CBS_peek_asn1_tag(const CBS *cbs, unsigned tag_value);
 /*
  * CBS_get_any_asn1_element sets |*out| to contain the next ASN.1 element from
  * |*cbs| (including header bytes) and advances |*cbs|. It sets |*out_tag| to
- * the tag number and |*out_header_len| to the length of the ASN.1 header. If
- * the element has indefinite length then |*out| will only contain the
- * header. Each of |out|, |out_tag|, and |out_header_len| may be NULL to ignore
+ * the tag number and |*out_header_len| to the length of the ASN.1 header.
+ * Each of |out|, |out_tag|, and |out_header_len| may be NULL to ignore
  * the value.
  *
  * Tag numbers greater than 30 are not supported (i.e. short form only).
@@ -452,6 +451,19 @@ int CBB_add_asn1_uint64(CBB *cbb, uint64_t value);
 
 #ifdef LIBRESSL_INTERNAL
 /*
+ * CBS_get_any_asn1_element sets |*out| to contain the next ASN.1 element from
+ * |*cbs| (including header bytes) and advances |*cbs|. It sets |*out_tag| to
+ * the tag number and |*out_header_len| to the length of the ASN.1 header. If
+ * strict mode is disabled and the element has indefinite length then |*out|
+ * will only contain the header. Each of |out|, |out_tag|, and
+ * |out_header_len| may be NULL to ignore the value.
+ *
+ * Tag numbers greater than 30 are not supported (i.e. short form only).
+ */
+int cbs_get_any_asn1_element_internal(CBS *cbs, CBS *out, unsigned *out_tag,
+    size_t *out_header_len, int strict);
+
+/*
  * 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
diff --git a/src/lib/libssl/src/ssl/bs_ber.c b/src/lib/libssl/src/ssl/bs_ber.c
index 2ec91fc800..3d39def111 100644
--- a/src/lib/libssl/src/ssl/bs_ber.c
+++ b/src/lib/libssl/src/ssl/bs_ber.c
@@ -1,4 +1,4 @@
-/*      $OpenBSD: bs_ber.c,v 1.4 2015/04/29 02:11:09 doug Exp $ */
+/*      $OpenBSD: bs_ber.c,v 1.5 2015/06/15 07:35:49 doug Exp $ */
 /*
  * Copyright (c) 2014, Google Inc.
  *
@@ -27,6 +27,15 @@
  */
 static const unsigned kMaxDepth = 2048;
 
+/* Non-strict version that allows a relaxed DER with indefinite form. */
+static int
+cbs_nonstrict_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
+    size_t *out_header_len)
+{
+        return cbs_get_any_asn1_element_internal(cbs, out,
+            out_tag, out_header_len, 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
@@ -49,10 +58,11 @@ cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth)
                 unsigned tag;
                 size_t header_len;
 
-                if (!CBS_get_any_asn1_element(&in, &contents, &tag,
+                if (!cbs_nonstrict_get_any_asn1_element(&in, &contents, &tag,
                     &header_len))
                         return 0;
 
+                /* Indefinite form not allowed by DER. */
                 if (CBS_len(&contents) == header_len && header_len > 0 &&
                     CBS_data(&contents)[header_len - 1] == 0x80) {
                         *ber_found = 1;
@@ -84,7 +94,8 @@ is_primitive_type(unsigned tag)
 
 /*
  * is_eoc returns true if |header_len| and |contents|, as returned by
- * |CBS_get_any_asn1_element|, indicate an "end of contents" (EOC) value.
+ * |cbs_nonstrict_get_any_asn1_element|, indicate an "end of contents" (EOC)
+ * value.
  */
 static char
 is_eoc(size_t header_len, CBS *contents)
@@ -113,7 +124,8 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc,
                 size_t header_len;
                 CBB *out_contents, out_contents_storage;
 
-                if (!CBS_get_any_asn1_element(in, &contents, &tag, &header_len))
+                if (!cbs_nonstrict_get_any_asn1_element(in, &contents, &tag,
+                    &header_len))
                         return 0;
 
                 out_contents = out;
@@ -156,9 +168,9 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc,
 
                                         CBS_init(&in_copy, CBS_data(in),
                                             CBS_len(in));
-                                        if (!CBS_get_any_asn1_element(&in_copy,
-                                            &inner_contents, &inner_tag,
-                                            &inner_header_len))
+                                        if (!cbs_nonstrict_get_any_asn1_element(
+                                            &in_copy, &inner_contents,
+                                            &inner_tag, &inner_header_len))
                                                 return 0;
 
                                         if (CBS_len(&inner_contents) >
diff --git a/src/lib/libssl/src/ssl/bs_cbs.c b/src/lib/libssl/src/ssl/bs_cbs.c
index c37f81dd60..ba38303c18 100644
--- a/src/lib/libssl/src/ssl/bs_cbs.c
+++ b/src/lib/libssl/src/ssl/bs_cbs.c
@@ -1,4 +1,4 @@
-/*      $OpenBSD: bs_cbs.c,v 1.8 2015/06/13 08:46:00 doug Exp $ */
+/*      $OpenBSD: bs_cbs.c,v 1.9 2015/06/15 07:35:49 doug Exp $ */
 /*
  * Copyright (c) 2014, Google Inc.
  *
@@ -205,24 +205,45 @@ int
 CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
     size_t *out_header_len)
 {
+        return cbs_get_any_asn1_element_internal(cbs, out, out_tag,
+            out_header_len, 1);
+}
+
+/*
+ * Review X.690 for details on ASN.1 DER encoding.
+ *
+ * If non-strict mode is enabled, then DER rules are relaxed
+ * for indefinite constructs (violates DER but a little closer to BER).
+ * Non-strict mode should only be used by bs_ber.c
+ *
+ * Sections 8, 10 and 11 for DER encoding
+ */
+int
+cbs_get_any_asn1_element_internal(CBS *cbs, CBS *out, unsigned *out_tag,
+    size_t *out_header_len, int strict)
+{
         uint8_t tag, length_byte;
         CBS header = *cbs;
         CBS throwaway;
+        size_t len;
 
         if (out == NULL)
                 out = &throwaway;
 
+        /*
+         * Get identifier octet and length octet.  Only 1 octet for each
+         * is a CBS limitation.
+         */
         if (!CBS_get_u8(&header, &tag) || !CBS_get_u8(&header, &length_byte))
                 return 0;
 
+        /* CBS limitation: long form tags are not supported. */
         if ((tag & 0x1f) == 0x1f)
-                /* Long form tags are not supported. */
                 return 0;
 
         if (out_tag != NULL)
                 *out_tag = tag;
 
-        size_t len;
         if ((length_byte & 0x80) == 0) {
                 /* Short form length. */
                 len = ((size_t) length_byte) + 2;
@@ -234,21 +255,40 @@ CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
                 const size_t num_bytes = length_byte & 0x7f;
                 uint32_t len32;
 
-                if ((tag & CBS_ASN1_CONSTRUCTED) != 0 && num_bytes == 0) {
-                        /* indefinite length */
-                        if (out_header_len != NULL)
-                                *out_header_len = 2;
-                        return CBS_get_bytes(cbs, out, 2);
+                /* ASN.1 reserved value for future extensions */
+                if (num_bytes == 0x7f)
+                        return 0;
+
+                /* Handle indefinite form length */
+                if (num_bytes == 0) {
+                        /* DER encoding doesn't allow for indefinite form. */
+                        if (strict) {
+                                return 0;
+
+                        } else {
+                                if ((tag & CBS_ASN1_CONSTRUCTED) != 0 &&
+                                    num_bytes == 0) {
+                                        /* indefinite length */
+                                        if (out_header_len != NULL)
+                                                *out_header_len = 2;
+                                        return CBS_get_bytes(cbs, out, 2);
+                                } else {
+                                        /* Primitive cannot use indefinite. */
+                                        return 0;
+                                }
+                        }
                 }
 
-                if (num_bytes == 0 || num_bytes > 4)
+                /* CBS limitation. */
+                if (num_bytes > 4)
                         return 0;
 
                 if (!cbs_get_u(&header, &len32, num_bytes))
                         return 0;
 
+                /* DER has a minimum length octet requirements. */
                 if (len32 < 128)
-                        /* Length should have used short-form encoding. */
+                        /* Should have used short form instead */
                         return 0;
 
                 if ((len32 >> ((num_bytes - 1) * 8)) == 0)
@@ -279,13 +319,7 @@ cbs_get_asn1(CBS *cbs, CBS *out, unsigned tag_value, int skip_header)
                 out = &throwaway;
 
         if (!CBS_get_any_asn1_element(cbs, out, &tag, &header_len) ||
-            tag != tag_value || (header_len > 0 &&
-            /*
-             * This ensures that the tag is either zero length or
-             * indefinite-length.
-             */
-            CBS_len(out) == header_len &&
-            CBS_data(out)[header_len - 1] == 0x80))
+            tag != tag_value)
                 return 0;
 
         if (skip_header && !CBS_skip(out, header_len)) {
diff --git a/src/lib/libssl/src/ssl/bytestring.h b/src/lib/libssl/src/ssl/bytestring.h
index b98c930da5..d66ab65b91 100644
--- a/src/lib/libssl/src/ssl/bytestring.h
+++ b/src/lib/libssl/src/ssl/bytestring.h
@@ -1,4 +1,4 @@
-/*      $OpenBSD: bytestring.h,v 1.6 2015/06/13 09:02:45 doug Exp $     */
+/*      $OpenBSD: bytestring.h,v 1.7 2015/06/15 07:35:49 doug Exp $     */
 /*
  * Copyright (c) 2014, Google Inc.
  *
@@ -223,9 +223,8 @@ int CBS_peek_asn1_tag(const CBS *cbs, unsigned tag_value);
 /*
  * CBS_get_any_asn1_element sets |*out| to contain the next ASN.1 element from
  * |*cbs| (including header bytes) and advances |*cbs|. It sets |*out_tag| to
- * the tag number and |*out_header_len| to the length of the ASN.1 header. If
- * the element has indefinite length then |*out| will only contain the
- * header. Each of |out|, |out_tag|, and |out_header_len| may be NULL to ignore
+ * the tag number and |*out_header_len| to the length of the ASN.1 header.
+ * Each of |out|, |out_tag|, and |out_header_len| may be NULL to ignore
  * the value.
  *
  * Tag numbers greater than 30 are not supported (i.e. short form only).
@@ -452,6 +451,19 @@ int CBB_add_asn1_uint64(CBB *cbb, uint64_t value);
 
 #ifdef LIBRESSL_INTERNAL
 /*
+ * CBS_get_any_asn1_element sets |*out| to contain the next ASN.1 element from
+ * |*cbs| (including header bytes) and advances |*cbs|. It sets |*out_tag| to
+ * the tag number and |*out_header_len| to the length of the ASN.1 header. If
+ * strict mode is disabled and the element has indefinite length then |*out|
+ * will only contain the header. Each of |out|, |out_tag|, and
+ * |out_header_len| may be NULL to ignore the value.
+ *
+ * Tag numbers greater than 30 are not supported (i.e. short form only).
+ */
+int cbs_get_any_asn1_element_internal(CBS *cbs, CBS *out, unsigned *out_tag,
+    size_t *out_header_len, int strict);
+
+/*
  * 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