Fix X509_ATTRIBUTE_set1_data with negative attributes

One of the WARNINGs in this function is unambiguously a bug. Just fix
it. In doing so, add a helper for the ASN1_STRING -> ASN1_TYPE
conversion and make this function easier to follow.

Also document the attrtype == 0 case. I missed that one when enumerating
this overcomplicated calling convention. Move that check earlier so we
don't try to process the input. It's ignored anyway.

Change-Id: Ia6e2dcb7c69488b6e2e58a68c3b701040ed3d4ef
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64827
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/asn1/a_strex.c

@@ -68,6 +68,7 @@
 #include <openssl/mem.h>
 
 #include "../bytestring/internal.h"
+#include "../internal.h"
 #include "internal.h"
 
 
@@ -238,22 +239,8 @@ static int do_dump(unsigned long flags, BIO *out, const ASN1_STRING *str) {
   // Placing the ASN1_STRING in a temporary ASN1_TYPE allows the DER encoding
   // to readily obtained.
   ASN1_TYPE t;
-  t.type = str->type;
-  // Negative INTEGER and ENUMERATED values are the only case where
-  // |ASN1_STRING| and |ASN1_TYPE| types do not match.
-  //
-  // TODO(davidben): There are also some type fields which, in |ASN1_TYPE|, do
-  // not correspond to |ASN1_STRING|. It is unclear whether those are allowed
-  // in |ASN1_STRING| at all, or what the space of allowed types is.
-  // |ASN1_item_ex_d2i| will never produce such a value so, for now, we say
-  // this is an invalid input. But this corner of the library in general
-  // should be more robust.
-  if (t.type == V_ASN1_NEG_INTEGER) {
-    t.type = V_ASN1_INTEGER;
-  } else if (t.type == V_ASN1_NEG_ENUMERATED) {
-    t.type = V_ASN1_ENUMERATED;
-  }
-  t.value.asn1_string = (ASN1_STRING *)str;
+  OPENSSL_memset(&t, 0, sizeof(ASN1_TYPE));
+  asn1_type_set0_string(&t, (ASN1_STRING *)str);
   unsigned char *der_buf = NULL;
   int der_len = i2d_ASN1_TYPE(&t, &der_buf);
   if (der_len < 0) {

crypto/asn1/a_type.c

@@ -56,7 +56,8 @@
 
 #include <openssl/asn1.h>
 
-#include <openssl/asn1t.h>
+#include <assert.h>
+
 #include <openssl/err.h>
 #include <openssl/mem.h>
 #include <openssl/obj.h>
@@ -89,6 +90,23 @@ const void *asn1_type_value_as_pointer(const ASN1_TYPE *a) {
   }
 }
 
+void asn1_type_set0_string(ASN1_TYPE *a, ASN1_STRING *str) {
+  // |ASN1_STRING| types are almost the same as |ASN1_TYPE| types, except that
+  // the negative flag is not reflected into |ASN1_TYPE|.
+  int type = str->type;
+  if (type == V_ASN1_NEG_INTEGER) {
+    type = V_ASN1_INTEGER;
+  } else if (type == V_ASN1_NEG_ENUMERATED) {
+    type = V_ASN1_ENUMERATED;
+  }
+
+  // These types are not |ASN1_STRING| types and use a different
+  // representation when stored in |ASN1_TYPE|.
+  assert(type != V_ASN1_NULL && type != V_ASN1_OBJECT &&
+         type != V_ASN1_BOOLEAN);
+  ASN1_TYPE_set(a, type, str);
+}
+
 void asn1_type_cleanup(ASN1_TYPE *a) {
   switch (a->type) {
     case V_ASN1_NULL:

crypto/asn1/internal.h

@@ -210,6 +210,10 @@ void asn1_encoding_clear(ASN1_ENCODING *enc);
 // a pointer.
 const void *asn1_type_value_as_pointer(const ASN1_TYPE *a);
 
+// asn1_type_set0_string sets |a|'s value to the object represented by |str| and
+// takes ownership of |str|.
+void asn1_type_set0_string(ASN1_TYPE *a, ASN1_STRING *str);
+
 // asn1_type_cleanup releases memory associated with |a|'s value, without
 // freeing |a| itself.
 void asn1_type_cleanup(ASN1_TYPE *a);

crypto/x509/x509_att.c

@@ -137,54 +137,57 @@ int X509_ATTRIBUTE_set1_object(X509_ATTRIBUTE *attr, const ASN1_OBJECT *obj) {
 
 int X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype,
                              const void *data, int len) {
-  ASN1_TYPE *ttmp = NULL;
-  ASN1_STRING *stmp = NULL;
-  int atype = 0;
   if (!attr) {
     return 0;
   }
+
+  if (attrtype == 0) {
+    // Do nothing. This is used to create an empty value set in
+    // |X509_ATTRIBUTE_create_by_*|. This is invalid, but supported by OpenSSL.
+    return 1;
+  }
+
+  ASN1_TYPE *typ = ASN1_TYPE_new();
+  if (typ == NULL) {
+    return 0;
+  }
+
+  // This function is several functions in one.
   if (attrtype & MBSTRING_FLAG) {
-    stmp = ASN1_STRING_set_by_NID(NULL, data, len, attrtype,
-                                  OBJ_obj2nid(attr->object));
-    if (!stmp) {
+    // |data| is an encoded string. We must decode and re-encode it to |attr|'s
+    // preferred ASN.1 type. Note |len| may be -1, in which case
+    // |ASN1_STRING_set_by_NID| calls |strlen| automatically.
+    ASN1_STRING *str = ASN1_STRING_set_by_NID(NULL, data, len, attrtype,
+                                              OBJ_obj2nid(attr->object));
+    if (str == NULL) {
       OPENSSL_PUT_ERROR(X509, ERR_R_ASN1_LIB);
-      return 0;
-    }
-    atype = stmp->type;
-  } else if (len != -1) {
-    if (!(stmp = ASN1_STRING_type_new(attrtype))) {
       goto err;
     }
-    if (!ASN1_STRING_set(stmp, data, len)) {
+    asn1_type_set0_string(typ, str);
+  } else if (len != -1) {
+    // |attrtype| must be a valid |ASN1_STRING| type. |data| and |len| is a
+    // value in the corresponding |ASN1_STRING| representation.
+    ASN1_STRING *str = ASN1_STRING_type_new(attrtype);
+    if (str == NULL || !ASN1_STRING_set(str, data, len)) {
+      ASN1_STRING_free(str);
       goto err;
     }
-    atype = attrtype;
-  }
-  // 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
-  // this.
-  if (attrtype == 0) {
-    ASN1_STRING_free(stmp);
-    return 1;
-  }
-  if (!(ttmp = ASN1_TYPE_new())) {
-    goto err;
-  }
-  if ((len == -1) && !(attrtype & MBSTRING_FLAG)) {
-    if (!ASN1_TYPE_set1(ttmp, attrtype, data)) {
+    asn1_type_set0_string(typ, str);
+  } else {
+    // |attrtype| must be a valid |ASN1_TYPE| type. |data| is a pointer to an
+    // object of the corresponding type.
+    if (!ASN1_TYPE_set1(typ, attrtype, data)) {
       goto err;
     }
-  } else {
-    ASN1_TYPE_set(ttmp, atype, stmp);
-    stmp = NULL;
   }
-  if (!sk_ASN1_TYPE_push(attr->set, ttmp)) {
+
+  if (!sk_ASN1_TYPE_push(attr->set, typ)) {
     goto err;
   }
   return 1;
+
 err:
-  ASN1_TYPE_free(ttmp);
-  ASN1_STRING_free(stmp);
+  ASN1_TYPE_free(typ);
   return 0;
 }
 

crypto/x509/x509_test.cc

@@ -3832,46 +3832,62 @@ TEST(X509Test, X509AlgorExtract) {
 
 // Test the various |X509_ATTRIBUTE| creation functions.
 TEST(X509Test, Attribute) {
-  // The friendlyName attribute has a BMPString value. See RFC 2985,
-  // section 5.5.1.
+  // The expected attribute values are:
+  // 1. BMPString U+2603
+  // 2. BMPString "test"
+  // 3. INTEGER -1 (not valid for friendlyName)
   static const uint8_t kTest1[] = {0x26, 0x03};  // U+2603 SNOWMAN
   static const uint8_t kTest1UTF8[] = {0xe2, 0x98, 0x83};
   static const uint8_t kTest2[] = {0, 't', 0, 'e', 0, 's', 0, 't'};
 
-  auto check_attribute = [&](X509_ATTRIBUTE *attr, bool has_test2) {
+  constexpr uint32_t kTest1Mask = 1 << 0;
+  constexpr uint32_t kTest2Mask = 1 << 1;
+  constexpr uint32_t kTest3Mask = 1 << 2;
+  auto check_attribute = [&](X509_ATTRIBUTE *attr, uint32_t mask) {
     EXPECT_EQ(NID_friendlyName, OBJ_obj2nid(X509_ATTRIBUTE_get0_object(attr)));
 
-    EXPECT_EQ(has_test2 ? 2 : 1, X509_ATTRIBUTE_count(attr));
-
-    // The first attribute should contain |kTest1|.
-    const ASN1_TYPE *value = X509_ATTRIBUTE_get0_type(attr, 0);
-    ASSERT_TRUE(value);
-    EXPECT_EQ(V_ASN1_BMPSTRING, value->type);
-    EXPECT_EQ(Bytes(kTest1),
-              Bytes(ASN1_STRING_get0_data(value->value.bmpstring),
-                    ASN1_STRING_length(value->value.bmpstring)));
-
-    // |X509_ATTRIBUTE_get0_data| requires the type match.
-    EXPECT_FALSE(
-        X509_ATTRIBUTE_get0_data(attr, 0, V_ASN1_OCTET_STRING, nullptr));
-    const ASN1_BMPSTRING *bmpstring = static_cast<const ASN1_BMPSTRING *>(
-        X509_ATTRIBUTE_get0_data(attr, 0, V_ASN1_BMPSTRING, nullptr));
-    ASSERT_TRUE(bmpstring);
-    EXPECT_EQ(Bytes(kTest1), Bytes(ASN1_STRING_get0_data(bmpstring),
-                                   ASN1_STRING_length(bmpstring)));
-
-    if (has_test2) {
-      value = X509_ATTRIBUTE_get0_type(attr, 1);
+    int idx = 0;
+    if (mask & kTest1Mask) {
+      // The first attribute should contain |kTest1|.
+      const ASN1_TYPE *value = X509_ATTRIBUTE_get0_type(attr, idx);
+      ASSERT_TRUE(value);
+      EXPECT_EQ(V_ASN1_BMPSTRING, value->type);
+      EXPECT_EQ(Bytes(kTest1),
+                Bytes(ASN1_STRING_get0_data(value->value.bmpstring),
+                      ASN1_STRING_length(value->value.bmpstring)));
+
+      // |X509_ATTRIBUTE_get0_data| requires the type match.
+      EXPECT_FALSE(
+          X509_ATTRIBUTE_get0_data(attr, idx, V_ASN1_OCTET_STRING, nullptr));
+      const ASN1_BMPSTRING *bmpstring = static_cast<const ASN1_BMPSTRING *>(
+          X509_ATTRIBUTE_get0_data(attr, idx, V_ASN1_BMPSTRING, nullptr));
+      ASSERT_TRUE(bmpstring);
+      EXPECT_EQ(Bytes(kTest1), Bytes(ASN1_STRING_get0_data(bmpstring),
+                                     ASN1_STRING_length(bmpstring)));
+      idx++;
+    }
+
+    if (mask & kTest2Mask) {
+      const ASN1_TYPE *value = X509_ATTRIBUTE_get0_type(attr, idx);
       ASSERT_TRUE(value);
       EXPECT_EQ(V_ASN1_BMPSTRING, value->type);
       EXPECT_EQ(Bytes(kTest2),
                 Bytes(ASN1_STRING_get0_data(value->value.bmpstring),
                       ASN1_STRING_length(value->value.bmpstring)));
-    } else {
-      EXPECT_FALSE(X509_ATTRIBUTE_get0_type(attr, 1));
+      idx++;
     }
 
-    EXPECT_FALSE(X509_ATTRIBUTE_get0_type(attr, 2));
+    if (mask & kTest3Mask) {
+      const ASN1_TYPE *value = X509_ATTRIBUTE_get0_type(attr, idx);
+      ASSERT_TRUE(value);
+      EXPECT_EQ(V_ASN1_INTEGER, value->type);
+      int64_t v;
+      ASSERT_TRUE(ASN1_INTEGER_get_int64(&v, value->value.integer));
+      EXPECT_EQ(v, -1);
+      idx++;
+    }
+
+    EXPECT_FALSE(X509_ATTRIBUTE_get0_type(attr, idx));
   };
 
   bssl::UniquePtr<ASN1_STRING> str(ASN1_STRING_type_new(V_ASN1_BMPSTRING));
@@ -3883,7 +3899,7 @@ TEST(X509Test, Attribute) {
       X509_ATTRIBUTE_create(NID_friendlyName, V_ASN1_BMPSTRING, str.get()));
   ASSERT_TRUE(attr);
   str.release();  // |X509_ATTRIBUTE_create| takes ownership on success.
-  check_attribute(attr.get(), /*has_test2=*/false);
+  check_attribute(attr.get(), kTest1Mask);
 
   // Test the |MBSTRING_*| form of |X509_ATTRIBUTE_set1_data|.
   attr.reset(X509_ATTRIBUTE_new());
@@ -3892,12 +3908,19 @@ TEST(X509Test, Attribute) {
       X509_ATTRIBUTE_set1_object(attr.get(), OBJ_nid2obj(NID_friendlyName)));
   ASSERT_TRUE(X509_ATTRIBUTE_set1_data(attr.get(), MBSTRING_UTF8, kTest1UTF8,
                                        sizeof(kTest1UTF8)));
-  check_attribute(attr.get(), /*has_test2=*/false);
+  check_attribute(attr.get(), kTest1Mask);
 
   // Test the |ASN1_STRING| form of |X509_ATTRIBUTE_set1_data|.
   ASSERT_TRUE(X509_ATTRIBUTE_set1_data(attr.get(), V_ASN1_BMPSTRING, kTest2,
                                        sizeof(kTest2)));
-  check_attribute(attr.get(), /*has_test2=*/true);
+  check_attribute(attr.get(), kTest1Mask | kTest2Mask);
+
+  // The |ASN1_STRING| form of |X509_ATTRIBUTE_set1_data| should correctly
+  // handle negative integers.
+  const uint8_t kOne = 1;
+  ASSERT_TRUE(
+      X509_ATTRIBUTE_set1_data(attr.get(), V_ASN1_NEG_INTEGER, &kOne, 1));
+  check_attribute(attr.get(), kTest1Mask | kTest2Mask | kTest3Mask);
 
   // Test the |ASN1_TYPE| form of |X509_ATTRIBUTE_set1_data|.
   attr.reset(X509_ATTRIBUTE_new());
@@ -3909,7 +3932,13 @@ TEST(X509Test, Attribute) {
   ASSERT_TRUE(ASN1_STRING_set(str.get(), kTest1, sizeof(kTest1)));
   ASSERT_TRUE(
       X509_ATTRIBUTE_set1_data(attr.get(), V_ASN1_BMPSTRING, str.get(), -1));
-  check_attribute(attr.get(), /*has_test2=*/false);
+  check_attribute(attr.get(), kTest1Mask);
+
+  // An |attrtype| of zero leaves the attribute empty.
+  attr.reset(X509_ATTRIBUTE_create_by_NID(
+      nullptr, NID_friendlyName, /*attrtype=*/0, /*data=*/nullptr, /*len=*/0));
+  ASSERT_TRUE(attr);
+  check_attribute(attr.get(), 0);
 }
 
 // Test that, by default, |X509_V_FLAG_TRUSTED_FIRST| is set, which means we'll

include/openssl/x509.h

@@ -2101,21 +2101,21 @@ OPENSSL_EXPORT int X509_ATTRIBUTE_set1_object(X509_ATTRIBUTE *attr,
 // X509_ATTRIBUTE_set1_data appends a value to |attr|'s value set and returns
 // one on success or zero on error. The value is determined as follows:
 //
-// If |attrtype| is a |MBSTRING_*| constant, the value is an ASN.1 string. The
-// string is determined by decoding |len| bytes from |data| in the encoding
-// specified by |attrtype|, and then re-encoding it in a form appropriate for
-// |attr|'s type. If |len| is -1, |strlen(data)| is used instead. See
-// |ASN1_STRING_set_by_NID| for details.
+// If |attrtype| is zero, this function returns one and does nothing. This form
+// may be used when calling |X509_ATTRIBUTE_create_by_*| to create an attribute
+// with an empty value set. Such attributes are invalid, but OpenSSL supports
+// creating them.
+//
+// Otherwise, if |attrtype| is a |MBSTRING_*| constant, the value is an ASN.1
+// string. The string is determined by decoding |len| bytes from |data| in the
+// encoding specified by |attrtype|, and then re-encoding it in a form
+// appropriate for |attr|'s type. If |len| is -1, |strlen(data)| is used
+// instead. See |ASN1_STRING_set_by_NID| for details.
 //
 // Otherwise, if |len| is not -1, the value is an ASN.1 string. |attrtype| is an
 // |ASN1_STRING| type value and the |len| bytes from |data| are copied as the
 // type-specific representation of |ASN1_STRING|. See |ASN1_STRING| for details.
 //
-// WARNING: If this form is used to construct a negative INTEGER or ENUMERATED,
-// |attrtype| includes the |V_ASN1_NEG| flag for |ASN1_STRING|, but the function
-// forgets to clear the flag for |ASN1_TYPE|. This matches OpenSSL but is
-// probably a bug. For now, do not use this form with negative values.
-//
 // Otherwise, if |len| is -1, the value is constructed by passing |attrtype| and
 // |data| to |ASN1_TYPE_set1|. That is, |attrtype| is an |ASN1_TYPE| type value,
 // and |data| is cast to the corresponding pointer type.