Remove support for malformed X509_ATTRIBUTEs.

The X509_ATTRIBUTE structure includes a hack to tolerate malformed
attributes that encode the value directly instead of a set of values.
This form is never created by OpenSSL and shouldn't be needed any more.

(Imported from upstream's e20b57270dece66ce2c68aeb5d14dd6d9f3c5d68.)

This also changes X509_ATTRIBUTE_set1_data slightly. Previously,
set1_data would override whatever was previously in the X509_ATTRIBUTE,
but leak memory. Now set1_data appends to the set. (PKCS#10 attributes
use SET OF ANY as value.) It's unclear to me if this was intentional on
upstream's part. (The attrtype == 0 case only makes sense in the old
behavior.) Since there is no other way to create a two-element SET and
upstream has long since released this behavior, I left it matching
upstream.

Update-Note: Given OpenSSL hasn't accepted these for five years, it's
unlikely anything depends on it. If something breaks, we can revert this
and revisit. No one calls X509_ATTRIBUTE_set1_data on a non-empty
X509_ATTRIBUTE, so the behavior change there should be safe.

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

crypto/x509/internal.h

@@ -39,12 +39,7 @@ struct X509_pubkey_st {
 
 struct x509_attributes_st {
   ASN1_OBJECT *object;
-  int single;  // 0 for a set, 1 for a single item (which is wrong)
-  union {
-    char *ptr;
-    /* 0 */ STACK_OF(ASN1_TYPE) *set;
-    /* 1 */ ASN1_TYPE *single;
-  } value;
+  STACK_OF(ASN1_TYPE) *set;
 } /* X509_ATTRIBUTE */;
 
 

crypto/x509/x509_att.c

@@ -311,9 +311,6 @@ int X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype,
             goto err;
         atype = attrtype;
     }
-    if (!(attr->value.set = sk_ASN1_TYPE_new_null()))
-        goto err;
-    attr->single = 0;
     /*
      * This is a bit naughty because the attribute should really have at
      * least one value but some types use and zero length SET and require
@@ -332,7 +329,7 @@ int X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype,
         ASN1_TYPE_set(ttmp, atype, stmp);
         stmp = NULL;
     }
-    if (!sk_ASN1_TYPE_push(attr->value.set, ttmp))
+    if (!sk_ASN1_TYPE_push(attr->set, ttmp))
         goto err;
     return 1;
  err:
@@ -344,11 +341,7 @@ int X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype,
 
 int X509_ATTRIBUTE_count(X509_ATTRIBUTE *attr)
 {
-    if (!attr->single)
-        return sk_ASN1_TYPE_num(attr->value.set);
-    if (attr->value.single)
-        return 1;
-    return 0;
+    return sk_ASN1_TYPE_num(attr->set);
 }
 
 ASN1_OBJECT *X509_ATTRIBUTE_get0_object(X509_ATTRIBUTE *attr)
@@ -375,11 +368,8 @@ void *X509_ATTRIBUTE_get0_data(X509_ATTRIBUTE *attr, int idx,
 ASN1_TYPE *X509_ATTRIBUTE_get0_type(X509_ATTRIBUTE *attr, int idx)
 {
     if (attr == NULL)
-        return (NULL);
+        return NULL;
     if (idx >= X509_ATTRIBUTE_count(attr))
         return NULL;
-    if (!attr->single)
-        return sk_ASN1_TYPE_value(attr->value.set, idx);
-    else
-        return attr->value.single;
+    return sk_ASN1_TYPE_value(attr->set, idx);
 }

crypto/x509/x_attrib.c

@@ -62,25 +62,9 @@
 #include "internal.h"
 
 
-/*
- * X509_ATTRIBUTE: this has the following form: typedef struct
- * x509_attributes_st { ASN1_OBJECT *object; int single; union { char *ptr;
- * STACK_OF(ASN1_TYPE) *set; ASN1_TYPE *single; } value; } X509_ATTRIBUTE;
- * this needs some extra thought because the CHOICE type is merged with the
- * main structure and because the value can be anything at all we *must* try
- * the SET OF first because the ASN1_ANY type will swallow anything including
- * the whole SET OF structure.
- */
-
-ASN1_CHOICE(X509_ATTRIBUTE_SET) = {
-        ASN1_SET_OF(X509_ATTRIBUTE, value.set, ASN1_ANY),
-        ASN1_SIMPLE(X509_ATTRIBUTE, value.single, ASN1_ANY)
-} ASN1_CHOICE_END_selector(X509_ATTRIBUTE, X509_ATTRIBUTE_SET, single)
-
 ASN1_SEQUENCE(X509_ATTRIBUTE) = {
         ASN1_SIMPLE(X509_ATTRIBUTE, object, ASN1_OBJECT),
-        /* CHOICE type merged with parent */
-        ASN1_EX_COMBINE(0, 0, X509_ATTRIBUTE_SET)
+        ASN1_SET_OF(X509_ATTRIBUTE, set, ASN1_ANY),
 } ASN1_SEQUENCE_END(X509_ATTRIBUTE)
 
 IMPLEMENT_ASN1_FUNCTIONS(X509_ATTRIBUTE)
@@ -100,10 +84,7 @@ X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int atrtype, void *value)
     }
 
     ret->object = obj;
-    ret->single = 0;
-    ret->value.set = sk_ASN1_TYPE_new_null();
-    if (ret->value.set == NULL ||
-        !sk_ASN1_TYPE_push(ret->value.set, val)) {
+    if (!sk_ASN1_TYPE_push(ret->set, val)) {
         goto err;
     }