Correctly handle invalid ASN1_OBJECTs when encoding.

asn1_ex_i2c actually does have an error condition, it just wasn't being
handled.

628b3c7f2f, imported from upstream's
f3f8e72f494b36d05e0d04fe418f92b692fbb261, tried to check for OID-less
ASN1_OBJECTs and return an error. But it and the upstream change didn't
actually work. -1 in this function means to omit the object, so OpenSSL
was silently misinterpreting the input structure.

This changes the calling convention for asn1_ex_i2c to support this. It
is, unfortunately, a little messy because:

1. One cannot check for object presense without walking the
   ASN1_ITEM/ASN1_TEMPLATE structures. You can *almost* check if *pval
   is NULL, but ASN1_BOOLEAN is an int with -1 to indicate an omitted
   optional. There are also FBOOLEAN/TBOOLEAN types that omit FALSE/TRUE
   for DEFAULT. Thus, without more invasive changes, asn1_ex_i2c must be
   able to report an omitted element.

2. While the i2d functions report an omitted element by successfully
   writing zero bytes, i2c only writes the contents. It thus must
   distinguish between an omitted element and an element with
   zero-length contents.

3. i2c_ASN1_INTEGER and i2c_ASN1_BIT_STRING return zero on error rather
   than -1. Those error paths are not actually reachable because they
   only check for NULL. In fact, OpenSSL has even unexported them. But I
   found a few callers. Rather than unwind all this and change the
   calling convention, I've just made it handle 0 and map to -1 for now.
   It's all a no-op anyway, and hopefully we can redo all this with CBB
   later.

I've just added an output parameter for now.

In writing tests, I also noticed that the hand-written i2d_ASN1_OBJECT
and i2d_ASN1_BOOLEAN return the wrong value for errors, so I've fixed
that.

Update-Note: A default-constructed object with a required ASN1_OBJECT
field can no longer be encoded without initializing the ASN1_OBJECT.
Note this affects X509: the signature algorithm is an ASN1_OBJECT. Tests
that try to serialize an X509_new() must fill in all required fields.
(Production code is unlikely to be affected because the output was
unparsable anyway, while tests sometimes wouldn't notice.)

Bug: 429
Change-Id: I04417f5ad6b994cc5ccca540c8a7714b9b3af33d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49348
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/asn1/a_bool.c

@@ -71,7 +71,7 @@ int i2d_ASN1_BOOLEAN(int a, unsigned char **pp)
     if (*pp == NULL) {
         if ((p = allocated = OPENSSL_malloc(r)) == NULL) {
             OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
-            return 0;
+            return -1;
         }
     } else {
         p = *pp;

crypto/asn1/a_object.c

@@ -69,20 +69,31 @@
 
 int i2d_ASN1_OBJECT(const ASN1_OBJECT *a, unsigned char **pp)
 {
-    unsigned char *p, *allocated = NULL;
-    int objsize;
+    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;
+    }
 
-    if ((a == NULL) || (a->data == NULL))
-        return (0);
+    if (a->length == 0) {
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_ILLEGAL_OBJECT);
+        return -1;
+    }
 
-    objsize = ASN1_object_size(0, a->length, V_ASN1_OBJECT);
-    if (pp == NULL || objsize == -1)
+    int objsize = ASN1_object_size(0, a->length, V_ASN1_OBJECT);
+    if (pp == NULL || objsize == -1) {
         return objsize;
+    }
 
+    unsigned char *p, *allocated = NULL;
     if (*pp == NULL) {
         if ((p = allocated = OPENSSL_malloc(objsize)) == NULL) {
             OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
-            return 0;
+            return -1;
         }
     } else {
         p = *pp;

crypto/asn1/asn1_test.cc

@@ -170,6 +170,8 @@ TEST(ASN1Test, ASN1Type) {
       {V_ASN1_BOOLEAN, {0x01, 0x01, 0x00}},
       // OCTET_STRING { "a" }
       {V_ASN1_OCTET_STRING, {0x04, 0x01, 0x61}},
+      // OCTET_STRING { }
+      {V_ASN1_OCTET_STRING, {0x04, 0x00}},
       // BIT_STRING { `01` `00` }
       {V_ASN1_BIT_STRING, {0x03, 0x02, 0x01, 0x00}},
       // INTEGER { -1 }
@@ -1083,6 +1085,17 @@ TEST(ASN1Test, InvalidChoice) {
   EXPECT_EQ(-1, i2d_GENERAL_NAMES(names.get(), nullptr));
 }
 
+// Encoding NID-only |ASN1_OBJECT|s should fail.
+TEST(ASN1Test, InvalidObject) {
+  EXPECT_EQ(-1, i2d_ASN1_OBJECT(OBJ_nid2obj(NID_kx_ecdhe), nullptr));
+
+  bssl::UniquePtr<X509_ALGOR> alg(X509_ALGOR_new());
+  ASSERT_TRUE(alg);
+  ASSERT_TRUE(X509_ALGOR_set0(alg.get(), OBJ_nid2obj(NID_kx_ecdhe),
+                              V_ASN1_UNDEF, nullptr));
+  EXPECT_EQ(-1, i2d_X509_ALGOR(alg.get(), nullptr));
+}
+
 // 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_enc.c

@@ -68,8 +68,8 @@
 
 static int asn1_i2d_ex_primitive(ASN1_VALUE **pval, unsigned char **out,
                                  const ASN1_ITEM *it, int tag, int aclass);
-static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cont, int *putype,
-                       const ASN1_ITEM *it);
+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,
                             int skcontlen, const ASN1_ITEM *item,
                             int do_sort, int iclass);
@@ -465,32 +465,24 @@ err:
 static int asn1_i2d_ex_primitive(ASN1_VALUE **pval, unsigned char **out,
                                  const ASN1_ITEM *it, int tag, int aclass)
 {
-    int len;
-    int utype;
-    int usetag;
-
-    utype = it->utype;
-
-    /*
-     * Get length of content octets and maybe find out the underlying type.
-     */
-
-    len = asn1_ex_i2c(pval, NULL, &utype, it);
-
-    /* -1 means omit type */
-    if (len == -1)
+    /* Get length of content octets and maybe find out the underlying type. */
+    int omit;
+    int utype = it->utype;
+    int len = asn1_ex_i2c(pval, NULL, &omit, &utype, it);
+    if (len < 0) {
+        return -1;
+    }
+    if (omit) {
         return 0;
+    }
 
     /*
      * If SEQUENCE, SET or OTHER then header is included in pseudo content
      * octets so don't include tag+length. We need to check here because the
      * call to asn1_ex_i2c() could change utype.
      */
-    if ((utype == V_ASN1_SEQUENCE) || (utype == V_ASN1_SET) ||
-        (utype == V_ASN1_OTHER))
-        usetag = 0;
-    else
-        usetag = 1;
+    int usetag = utype != V_ASN1_SEQUENCE && utype != V_ASN1_SET &&
+                 utype != V_ASN1_OTHER;
 
     /* If not implicitly tagged get tag from underlying type */
     if (tag == -1)
@@ -498,20 +490,28 @@ static int asn1_i2d_ex_primitive(ASN1_VALUE **pval, unsigned char **out,
 
     /* Output tag+length followed by content octets */
     if (out) {
-        if (usetag)
+        if (usetag) {
             ASN1_put_object(out, /*constructed=*/0, len, tag, aclass);
-        asn1_ex_i2c(pval, *out, &utype, it);
+        }
+        int len2 = asn1_ex_i2c(pval, *out, &omit, &utype, it);
+        if (len2 < 0) {
+          return -1;
+        }
+        assert(len == len2);
+        assert(!omit);
         *out += len;
     }
 
-    if (usetag)
+    if (usetag) {
         return ASN1_object_size(/*constructed=*/0, len, tag);
+    }
     return len;
 }
 
 /* asn1_ex_i2c writes the |*pval| to |cout| under the i2d output convention,
- * excluding the tag and length. It returns the number of bytes written on
- * success and -1 to if |*pval| should be omitted.
+ * excluding the tag and length. It returns the number of bytes written,
+ * possibly zero, on success or -1 on error. If |*pval| should be omitted, it
+ * returns zero and sets |*out_omit| to true.
  *
  * If |it| is an MSTRING or ANY type, it gets the underlying type from |*pval|,
  * which must be an |ASN1_STRING| or |ASN1_TYPE|, respectively. It then updates
@@ -522,10 +522,10 @@ static int asn1_i2d_ex_primitive(ASN1_VALUE **pval, unsigned char **out,
  *
  * Otherwise, |*putype| must contain |it->utype|.
  *
- * WARNING: Unlike most functions in this file, |asn1_ex_i2c| returns -1 to omit
- * the field instead of 0. It has no way to report an error condition. */
-static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cout, int *putype,
-                       const ASN1_ITEM *it)
+ * WARNING: Unlike most functions in this file, |asn1_ex_i2c| can return zero
+ * without omitting the element. ASN.1 values may have empty contents. */
+static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cout, int *out_omit,
+                       int *putype, const ASN1_ITEM *it)
 {
     ASN1_BOOLEAN *tbool = NULL;
     ASN1_STRING *strtmp;
@@ -539,11 +539,15 @@ static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cout, int *putype,
      * |ASN1_PRIMITIVE_FUNCS| table of callbacks. */
     assert(it->funcs == NULL);
 
+    *out_omit = 0;
+
     /* Should type be omitted? */
     if ((it->itype != ASN1_ITYPE_PRIMITIVE)
         || (it->utype != V_ASN1_BOOLEAN)) {
-        if (!*pval)
-            return -1;
+        if (!*pval) {
+            *out_omit = 1;
+            return 0;
+        }
     }
 
     if (it->itype == ASN1_ITYPE_MSTRING) {
@@ -580,8 +584,11 @@ static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cout, int *putype,
         otmp = (ASN1_OBJECT *)*pval;
         cont = otmp->data;
         len = otmp->length;
-        if (cont == NULL || len == 0)
+        if (len == 0) {
+            /* Some |ASN1_OBJECT|s do not have OIDs and cannot be serialized. */
+            OPENSSL_PUT_ERROR(ASN1, ASN1_R_ILLEGAL_OBJECT);
             return -1;
+        }
         break;
 
     case V_ASN1_NULL:
@@ -591,32 +598,39 @@ static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cout, int *putype,
 
     case V_ASN1_BOOLEAN:
         tbool = (ASN1_BOOLEAN *)pval;
-        if (*tbool == -1)
-            return -1;
+        if (*tbool == -1) {
+            *out_omit = 1;
+            return 0;
+        }
         if (it->utype != V_ASN1_ANY) {
             /*
              * Default handling if value == size field then omit
              */
-            if (*tbool && (it->size > 0))
-                return -1;
-            if (!*tbool && !it->size)
-                return -1;
+            if ((*tbool && (it->size > 0)) ||
+                (!*tbool && !it->size)) {
+                *out_omit = 1;
+                return 0;
+            }
         }
         c = *tbool ? 0xff : 0x00;
         cont = &c;
         len = 1;
         break;
 
-    case V_ASN1_BIT_STRING:
-        return i2c_ASN1_BIT_STRING((ASN1_BIT_STRING *)*pval,
-                                   cout ? &cout : NULL);
+    case V_ASN1_BIT_STRING: {
+        int ret = i2c_ASN1_BIT_STRING((ASN1_BIT_STRING *)*pval,
+                                      cout ? &cout : NULL);
+        /* |i2c_ASN1_BIT_STRING| returns zero on error instead of -1. */
+        return ret <= 0 ? -1 : ret;
+    }
 
     case V_ASN1_INTEGER:
-    case V_ASN1_ENUMERATED:
-        /*
-         * These are all have the same content format as ASN1_INTEGER
-         */
-        return i2c_ASN1_INTEGER((ASN1_INTEGER *)*pval, cout ? &cout : NULL);
+    case V_ASN1_ENUMERATED: {
+        /* |i2c_ASN1_INTEGER| also handles ENUMERATED. */
+        int ret = i2c_ASN1_INTEGER((ASN1_INTEGER *)*pval, cout ? &cout : NULL);
+        /* |i2c_ASN1_INTEGER| returns zero on error instead of -1. */
+        return ret <= 0 ? -1 : ret;
+    }
 
     case V_ASN1_OCTET_STRING:
     case V_ASN1_NUMERICSTRING: