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>

3 files changed, 26 insertions, 17 deletions

diff --git a/crypto/asn1/a_strex.c b/crypto/asn1/a_strex.c
index 7e9afad..516f7df 100644
--- a/crypto/asn1/a_strex.c
+++ b/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) {
diff --git a/crypto/asn1/a_type.c b/crypto/asn1/a_type.c
index 0c2fd13..af603a3 100644
--- a/crypto/asn1/a_type.c
+++ b/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:
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index 414b5a9..57e6966 100644
--- a/crypto/asn1/internal.h
+++ b/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);