Reject missing required fields in i2d functions.

See also 006906cddda37e24a66443199444ef4476697477 from OpenSSL, though
this CL uses a different strategy from upstream. Upstream makes
ASN1_item_ex_i2d continue to allow optionals and checks afterwards at
every non-optional call site. This CL pushes down an optional parameter
and says functions cannot omit items unless explicitly allowed.

I think this is a better default, though it is a larger change. Fields
are only optional when they come from an ASN1_TEMPLATE with the OPTIONAL
flag. Upstream's strategy misses top-level calls.

This CL additionally adds checks for optional ASN1_TEMPLATEs in contexts
where it doesn't make sense. Only fields of SEQUENCEs and SETs may be
OPTIONAL, but the ASN1_ITEM/ASN1_TEMPLATE split doesn't quite match
ASN.1 itself. ASN1_TEMPLATE is additionally responsible for
explicit/implicit tagging, and SEQUENCE/SET OF. That means CHOICE arms
and the occasional top-level type (ASN1_ITEM_TEMPLATE) use ASN1_TEMPLATE
but will get confused if marked optional.

As part of this, i2d_FOO(NULL) now returns -1 rather than "successfully"
writing 0 bytes. If we want to allow NULL at the top-level, that's not
too hard to arrange, but our CBB-based i2d functions do not.

Update-Note: Structures with missing mandatory fields can no longer be
encoded. Note that, apart from the cases already handled by preceding
CLs, tasn_new.c will fill in non-NULL empty objects everywhere. The main
downstream impact I've seen of this particular change is in combination
with other bugs. Consider a caller that does:

  GENERAL_NAME *name = GENERAL_NAME_new();
  name->type = GEN_DNS;
  name->d.dNSName = DoSomethingComplicated(...);

Suppose DoSomethingComplicated() was actually fallible and returned
NULL, but the caller forgot to check. They'd now construct a
GENERAL_NAME with a missing field. Previously, this would silently
serialize some garbage (omitted field) or empty string. Now we fail to
encode, but the true error was the uncaught DoSomethingComplicated()
failure. (Which likely was itself a bug.)

Bug: 429
Change-Id: I37fe618761be64a619be9fdc8d416f24ecbb8c46
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49350
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/.clang-format b/.clang-format
index 6de1483e..8f9cb6c 100644
--- a/.clang-format
+++ b/.clang-format
@@ -10,6 +10,7 @@
 IncludeBlocks: Preserve
 TypenameMacros: ['LHASH_OF', 'STACK_OF']
 StatementMacros:
+  - "ASN1_SEQUENCE_END"
   - "DECLARE_ASN1_ALLOC_FUNCTIONS"
   - "DECLARE_ASN1_ALLOC_FUNCTIONS_name"
   - "DECLARE_ASN1_ENCODE_FUNCTIONS"
diff --git a/crypto/asn1/a_object.c b/crypto/asn1/a_object.c
index 91ebb7a..b88f06b 100644
--- a/crypto/asn1/a_object.c
+++ b/crypto/asn1/a_object.c
@@ -70,13 +70,8 @@
 int i2d_ASN1_OBJECT(const ASN1_OBJECT *a, unsigned char **pp)
 {
     if (a == NULL) {
-        /* Match |ASN1_item_i2d| and "successfully" omit the object when passed
-         * NULL.
-         *
-         * TODO(davidben): Should this and |ASN1_item_i2d| be an error? OpenSSL
-         * did not do this, but our |CBB|-based functions like
-         * |i2d_RSAPrivateKey| don't allow NULL at all. */
-        return 0;
+        OPENSSL_PUT_ERROR(ASN1, ERR_R_PASSED_NULL_PARAMETER);
+        return -1;
     }
 
     if (a->length == 0) {
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 5cd17af..9119dea 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1131,7 +1131,7 @@
 DECLARE_ASN1_FUNCTIONS(ASN1_LINKED_LIST)
 
 ASN1_SEQUENCE(ASN1_LINKED_LIST) = {
-  ASN1_OPT(ASN1_LINKED_LIST, next, ASN1_LINKED_LIST),
+    ASN1_OPT(ASN1_LINKED_LIST, next, ASN1_LINKED_LIST),
 } ASN1_SEQUENCE_END(ASN1_LINKED_LIST)
 
 IMPLEMENT_ASN1_FUNCTIONS(ASN1_LINKED_LIST)
@@ -1182,15 +1182,13 @@
   ASN1_STRING *string;
 };
 
-// clang-format off
 DECLARE_ASN1_FUNCTIONS(IMPLICIT_CHOICE)
 
 ASN1_SEQUENCE(IMPLICIT_CHOICE) = {
-  ASN1_IMP(IMPLICIT_CHOICE, string, DIRECTORYSTRING, 0)
+    ASN1_IMP(IMPLICIT_CHOICE, string, DIRECTORYSTRING, 0),
 } ASN1_SEQUENCE_END(IMPLICIT_CHOICE)
 
 IMPLEMENT_ASN1_FUNCTIONS(IMPLICIT_CHOICE)
-// clang-format on
 
 // Test that the ASN.1 templates reject types with implicitly-tagged CHOICE
 // types.
@@ -1215,4 +1213,50 @@
   EXPECT_EQ(nullptr, d2i_IMPLICIT_CHOICE(nullptr, &ptr, sizeof(kInput2)));
 }
 
+struct REQUIRED_FIELD {
+  ASN1_INTEGER *value;
+  ASN1_INTEGER *value_imp;
+  ASN1_INTEGER *value_exp;
+  STACK_OF(ASN1_INTEGER) *seq;
+  STACK_OF(ASN1_INTEGER) *seq_imp;
+  STACK_OF(ASN1_INTEGER) *seq_exp;
+};
+
+DECLARE_ASN1_FUNCTIONS(REQUIRED_FIELD)
+ASN1_SEQUENCE(REQUIRED_FIELD) = {
+    ASN1_SIMPLE(REQUIRED_FIELD, value, ASN1_INTEGER),
+    ASN1_IMP(REQUIRED_FIELD, value_imp, ASN1_INTEGER, 0),
+    ASN1_EXP(REQUIRED_FIELD, value_exp, ASN1_INTEGER, 1),
+    ASN1_SEQUENCE_OF(REQUIRED_FIELD, seq, ASN1_INTEGER),
+    ASN1_IMP_SEQUENCE_OF(REQUIRED_FIELD, seq_imp, ASN1_INTEGER, 2),
+    ASN1_EXP_SEQUENCE_OF(REQUIRED_FIELD, seq_exp, ASN1_INTEGER, 3),
+} ASN1_SEQUENCE_END(REQUIRED_FIELD)
+IMPLEMENT_ASN1_FUNCTIONS(REQUIRED_FIELD)
+
+// Test that structures with missing required fields cannot be serialized. Test
+// the full combination of tagging and SEQUENCE OF.
+TEST(ASN1Test, MissingRequiredField) {
+  EXPECT_EQ(-1, i2d_REQUIRED_FIELD(nullptr, nullptr));
+
+  std::unique_ptr<REQUIRED_FIELD, decltype(&REQUIRED_FIELD_free)> obj(
+      nullptr, REQUIRED_FIELD_free);
+  for (auto field : {&REQUIRED_FIELD::value, &REQUIRED_FIELD::value_imp,
+                     &REQUIRED_FIELD::value_exp}) {
+    obj.reset(REQUIRED_FIELD_new());
+    ASSERT_TRUE(obj);
+    ASN1_INTEGER_free((*obj).*field);
+    (*obj).*field = nullptr;
+    EXPECT_EQ(-1, i2d_REQUIRED_FIELD(obj.get(), nullptr));
+  }
+
+  for (auto field : {&REQUIRED_FIELD::seq, &REQUIRED_FIELD::seq_imp,
+                     &REQUIRED_FIELD::seq_exp}) {
+    obj.reset(REQUIRED_FIELD_new());
+    ASSERT_TRUE(obj);
+    sk_ASN1_INTEGER_pop_free((*obj).*field, ASN1_INTEGER_free);
+    (*obj).*field = nullptr;
+    EXPECT_EQ(-1, i2d_REQUIRED_FIELD(obj.get(), nullptr));
+  }
+}
+
 #endif  // !WINDOWS || !SHARED_LIBRARY
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index b995ede..e30be16 100644
--- a/crypto/asn1/internal.h
+++ b/crypto/asn1/internal.h
@@ -124,10 +124,11 @@
                      ASN1_TLC *ctx);
 
 /* ASN1_item_ex_i2d encodes |*pval| as a value of type |it| to |out| under the
- * i2d output convention. It returns the length on success, -1 on error, and
- * zero if the object was omitted. If |tag| is -1. the tag and class come from
- * |it|. Otherwise, the tag number is |tag| and the class is the
- * |ASN1_TFLG_TAG_CLASS| bits of |aclass|. This is used for implicit tagging.
+ * i2d output convention. It returns a non-zero length on success and -1 on
+ * error. If |tag| is -1. the tag and class come from |it|. Otherwise, the tag
+ * number is |tag| and the class is the |ASN1_TFLG_TAG_CLASS| bits of |aclass|.
+ * This is used for implicit tagging. This function treats a missing value as an
+ * error, not an optional field.
  *
  * TODO(davidben): Historically, |aclass| contained other flags, but we may have
  * removed the last of them. */
diff --git a/crypto/asn1/tasn_enc.c b/crypto/asn1/tasn_enc.c
index 7e79da2..6dddb52 100644
--- a/crypto/asn1/tasn_enc.c
+++ b/crypto/asn1/tasn_enc.c
@@ -66,8 +66,12 @@
 #include "internal.h"
 
 
+static int asn1_item_ex_i2d_opt(ASN1_VALUE **pval, unsigned char **out,
+                                const ASN1_ITEM *it, int tag, int aclass,
+                                int optional);
 static int asn1_i2d_ex_primitive(ASN1_VALUE **pval, unsigned char **out,
-                                 const ASN1_ITEM *it, int tag, int aclass);
+                                 const ASN1_ITEM *it, int tag, int aclass,
+                                 int optional);
 static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cont, int *out_omit,
                        int *putype, const ASN1_ITEM *it);
 static int asn1_set_seq_out(STACK_OF(ASN1_VALUE) *sk, unsigned char **out,
@@ -114,14 +118,31 @@
 int ASN1_item_ex_i2d(ASN1_VALUE **pval, unsigned char **out,
                      const ASN1_ITEM *it, int tag, int aclass)
 {
+    int ret = asn1_item_ex_i2d_opt(pval, out, it, tag, aclass, /*optional=*/0);
+    assert(ret != 0);
+    return ret;
+}
+
+/* asn1_item_ex_i2d_opt behaves like |ASN1_item_ex_i2d| but, if |optional| is
+ * non-zero and |*pval| is omitted, it returns zero and writes no bytes. */
+int asn1_item_ex_i2d_opt(ASN1_VALUE **pval, unsigned char **out,
+                         const ASN1_ITEM *it, int tag, int aclass,
+                         int optional)
+{
     const ASN1_TEMPLATE *tt = NULL;
     int i, seqcontlen, seqlen;
-    const ASN1_EXTERN_FUNCS *ef;
     const ASN1_AUX *aux = it->funcs;
     ASN1_aux_cb *asn1_cb = NULL;
 
-    if ((it->itype != ASN1_ITYPE_PRIMITIVE) && !*pval)
-        return 0;
+    /* All fields are pointers, except for boolean |ASN1_ITYPE_PRIMITIVE|s.
+     * Optional primitives are handled later. */
+    if ((it->itype != ASN1_ITYPE_PRIMITIVE) && !*pval) {
+        if (optional) {
+            return 0;
+        }
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_VALUE);
+        return -1;
+    }
 
     if (aux && aux->asn1_cb)
         asn1_cb = aux->asn1_cb;
@@ -129,10 +150,14 @@
     switch (it->itype) {
 
     case ASN1_ITYPE_PRIMITIVE:
-        if (it->templates)
-            return asn1_template_ex_i2d(pval, out, it->templates,
-                                        tag, aclass);
-        return asn1_i2d_ex_primitive(pval, out, it, tag, aclass);
+        if (it->templates) {
+            if (it->templates->flags & ASN1_TFLG_OPTIONAL) {
+                OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE);
+                return -1;
+            }
+            return asn1_template_ex_i2d(pval, out, it->templates, tag, aclass);
+        }
+        return asn1_i2d_ex_primitive(pval, out, it, tag, aclass, optional);
 
     case ASN1_ITYPE_MSTRING:
         /*
@@ -143,7 +168,7 @@
             OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE);
             return -1;
         }
-        return asn1_i2d_ex_primitive(pval, out, it, -1, aclass);
+        return asn1_i2d_ex_primitive(pval, out, it, -1, aclass, optional);
 
     case ASN1_ITYPE_CHOICE: {
         /*
@@ -162,6 +187,10 @@
             return -1;
         }
         const ASN1_TEMPLATE *chtt = it->templates + i;
+        if (chtt->flags & ASN1_TFLG_OPTIONAL) {
+            OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE);
+            return -1;
+        }
         ASN1_VALUE **pchval = asn1_get_field_ptr(pval, chtt);
         int ret = asn1_template_ex_i2d(pchval, out, chtt, -1, aclass);
         if (ret < 0 ||
@@ -171,10 +200,19 @@
         return ret;
     }
 
-    case ASN1_ITYPE_EXTERN:
+    case ASN1_ITYPE_EXTERN: {
         /* If new style i2d it does all the work */
-        ef = it->funcs;
-        return ef->asn1_ex_i2d(pval, out, it, tag, aclass);
+        const ASN1_EXTERN_FUNCS *ef = it->funcs;
+        int ret = ef->asn1_ex_i2d(pval, out, it, tag, aclass);
+        if (ret == 0) {
+            /* |asn1_ex_i2d| should never return zero. We have already checked
+             * for optional values generically, and |ASN1_ITYPE_EXTERN| fields
+             * must be pointers. */
+            OPENSSL_PUT_ERROR(ASN1, ERR_R_INTERNAL_ERROR);
+            return -1;
+        }
+        return ret;
+    }
 
     case ASN1_ITYPE_SEQUENCE:
         i = asn1_enc_restore(&seqcontlen, out, pval, it);
@@ -236,9 +274,10 @@
     }
 }
 
-/* asn1_template_ex_i2d behaves like |ASN1_item_ex_i2d| but uses an
+/* asn1_template_ex_i2d behaves like |asn1_item_ex_i2d_opt| but uses an
  * |ASN1_TEMPLATE| instead of an |ASN1_ITEM|. An |ASN1_TEMPLATE| wraps an
- * |ASN1_ITEM| with modifiers such as tagging, SEQUENCE or SET, etc. */
+ * |ASN1_ITEM| with modifiers such as tagging, SEQUENCE or SET, etc. Instead of
+ * taking an |optional| parameter, it uses the |ASN1_TFLG_OPTIONAL| flag. */
 static int asn1_template_ex_i2d(ASN1_VALUE **pval, unsigned char **out,
                                 const ASN1_TEMPLATE *tt, int tag, int iclass)
 {
@@ -274,6 +313,8 @@
      */
     iclass &= ~ASN1_TFLG_TAG_CLASS;
 
+    const int optional = (flags & ASN1_TFLG_OPTIONAL) != 0;
+
     /*
      * At this point 'ttag' contains the outer tag to use, 'tclass' is the
      * class and iclass is any flags passed to this function.
@@ -286,8 +327,13 @@
         int skcontlen, sklen;
         ASN1_VALUE *skitem;
 
-        if (!*pval)
-            return 0;
+        if (!*pval) {
+            if (optional) {
+                return 0;
+            }
+            OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_VALUE);
+            return -1;
+        }
 
         if (flags & ASN1_TFLG_SET_OF) {
             isset = 1;
@@ -353,7 +399,8 @@
     if (flags & ASN1_TFLG_EXPTAG) {
         /* EXPLICIT tagging */
         /* Find length of tagged item */
-        i = ASN1_item_ex_i2d(pval, NULL, ASN1_ITEM_ptr(tt->item), -1, iclass);
+        i = asn1_item_ex_i2d_opt(pval, NULL, ASN1_ITEM_ptr(tt->item), -1,
+                                 iclass, optional);
         if (i <= 0)
             return i;
         /* Find length of EXPLICIT tag */
@@ -370,8 +417,8 @@
     }
 
     /* Either normal or IMPLICIT tagging: combine class and flags */
-    return ASN1_item_ex_i2d(pval, out, ASN1_ITEM_ptr(tt->item),
-                            ttag, tclass | iclass);
+    return asn1_item_ex_i2d_opt(pval, out, ASN1_ITEM_ptr(tt->item),
+                                ttag, tclass | iclass, optional);
 
 }
 
@@ -463,7 +510,8 @@
 /* asn1_i2d_ex_primitive behaves like |ASN1_item_ex_i2d| but |item| must be a
  * a PRIMITIVE or MSTRING type that is not an |ASN1_ITEM_TEMPLATE|. */
 static int asn1_i2d_ex_primitive(ASN1_VALUE **pval, unsigned char **out,
-                                 const ASN1_ITEM *it, int tag, int aclass)
+                                 const ASN1_ITEM *it, int tag, int aclass,
+                                 int optional)
 {
     /* Get length of content octets and maybe find out the underlying type. */
     int omit;
@@ -473,7 +521,11 @@
         return -1;
     }
     if (omit) {
-        return 0;
+        if (optional) {
+            return 0;
+        }
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_VALUE);
+        return -1;
     }
 
     /*
diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c
index 38cceb1..9d350bd 100644
--- a/crypto/x509/x_x509.c
+++ b/crypto/x509/x_x509.c
@@ -289,13 +289,15 @@
         return length;
     }
 
-    tmplen = i2d_X509_CERT_AUX(a->aux, pp);
-    if (tmplen < 0) {
-        if (start != NULL)
-            *pp = start;
-        return tmplen;
+    if (a->aux != NULL) {
+        tmplen = i2d_X509_CERT_AUX(a->aux, pp);
+        if (tmplen < 0) {
+            if (start != NULL)
+                *pp = start;
+            return tmplen;
+        }
+        length += tmplen;
     }
-    length += tmplen;
 
     return length;
 }