Do not access value.ptr with V_ASN1_BOOLEAN. This fixes a bug in ASN1_TYPE_get. Partly imported from upstream's 261ec72d58af64327214a78ca1c54b169ad93c28, though I don't believe ASN1_TYPE_set was broken per se. There's also a lot more than in that commit. I've added a test to ensure we maintain the unused bits invariant anyway, in case external code relies on it. (The invariant comes from the pointer being NULL-initialized and from ASN1_primitive_free zeroing *pval on free.) Change-Id: I4c0c57519a7628041d81c26cd850317e01409556 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46324 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com>

diff --git a/crypto/asn1/a_type.c b/crypto/asn1/a_type.c
index f320e49..16709d3 100644
--- a/crypto/asn1/a_type.c
+++ b/crypto/asn1/a_type.c

@@ -66,18 +66,28 @@
 
 int ASN1_TYPE_get(const ASN1_TYPE *a)
 {
-    if ((a->value.ptr != NULL) || (a->type == V_ASN1_NULL))
-        return (a->type);
-    else
-        return (0);
+    if (a->type == V_ASN1_BOOLEAN || a->type == V_ASN1_NULL ||
+        a->value.ptr != NULL) {
+        return a->type;
+    }
+    return 0;
+}
+
+const void *asn1_type_value_as_pointer(const ASN1_TYPE *a)
+{
+    if (a->type == V_ASN1_BOOLEAN) {
+        return a->value.boolean ? (void *)0xff : NULL;
+    }
+    if (a->type == V_ASN1_NULL) {
+        return NULL;
+    }
+    return a->value.ptr;
 }
 
 void ASN1_TYPE_set(ASN1_TYPE *a, int type, void *value)
 {
-    if (a->value.ptr != NULL) {
-        ASN1_TYPE **tmp_a = &a;
-        ASN1_primitive_free((ASN1_VALUE **)tmp_a, NULL);
-    }
+    ASN1_TYPE **tmp_a = &a;
+    ASN1_primitive_free((ASN1_VALUE **)tmp_a, NULL);
     a->type = type;
     if (type == V_ASN1_BOOLEAN)
         a->value.boolean = value ? 0xff : 0;

diff --git a/crypto/asn1/asn1_locl.h b/crypto/asn1/asn1_locl.h
index bf90ea2..9b79c3e 100644
--- a/crypto/asn1/asn1_locl.h
+++ b/crypto/asn1/asn1_locl.h

@@ -126,6 +126,11 @@
 int asn1_enc_save(ASN1_VALUE **pval, const unsigned char *in, int inlen,
                   const ASN1_ITEM *it);
 
+/* asn1_type_value_as_pointer returns |a|'s value in pointer form. This is
+ * usually the value object but, for BOOLEAN values, is 0 or 0xff cast to
+ * a pointer. */
+const void *asn1_type_value_as_pointer(const ASN1_TYPE *a);
+
 
 #if defined(__cplusplus)
 }  /* extern C */

diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 7b09ba5..058e07a 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc

@@ -130,6 +130,66 @@
   TestSerialize(0x00, i2d_ASN1_BOOLEAN, kFalse);
 }
 
+TEST(ASN1Test, ASN1Type) {
+  const struct {
+    int type;
+    std::vector<uint8_t> der;
+  } kTests[] = {
+      // BOOLEAN { TRUE }
+      {V_ASN1_BOOLEAN, {0x01, 0x01, 0xff}},
+      // BOOLEAN { FALSE }
+      {V_ASN1_BOOLEAN, {0x01, 0x01, 0x00}},
+      // OCTET_STRING { "a" }
+      {V_ASN1_OCTET_STRING, {0x04, 0x01, 0x61}},
+      // BIT_STRING { `01` `00` }
+      {V_ASN1_BIT_STRING, {0x03, 0x02, 0x01, 0x00}},
+      // INTEGER { -1 }
+      {V_ASN1_INTEGER, {0x02, 0x01, 0xff}},
+      // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2 }
+      {V_ASN1_OBJECT,
+       {0x06, 0x0c, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7,
+        0x09, 0x02}},
+      // NULL {}
+      {V_ASN1_NULL, {0x05, 0x00}},
+      // SEQUENCE {}
+      {V_ASN1_SEQUENCE, {0x30, 0x00}},
+      // SET {}
+      {V_ASN1_SET, {0x31, 0x00}},
+      // [0] { UTF8String { "a" } }
+      {V_ASN1_OTHER, {0xa0, 0x03, 0x0c, 0x01, 0x61}},
+  };
+  for (const auto &t : kTests) {
+    SCOPED_TRACE(Bytes(t.der));
+
+    // The input should successfully parse.
+    const uint8_t *ptr = t.der.data();
+    bssl::UniquePtr<ASN1_TYPE> val(d2i_ASN1_TYPE(nullptr, &ptr, t.der.size()));
+    ASSERT_TRUE(val);
+
+    EXPECT_EQ(ASN1_TYPE_get(val.get()), t.type);
+    EXPECT_EQ(val->type, t.type);
+    TestSerialize(val.get(), i2d_ASN1_TYPE, t.der);
+  }
+}
+
+// Test that reading |value.ptr| from a FALSE |ASN1_TYPE| behaves correctly. The
+// type historically supported this, so maintain the invariant in case external
+// code relies on it.
+TEST(ASN1Test, UnusedBooleanBits) {
+  // OCTET_STRING { "a" }
+  static const uint8_t kDER[] = {0x04, 0x01, 0x61};
+  const uint8_t *ptr = kDER;
+  bssl::UniquePtr<ASN1_TYPE> val(d2i_ASN1_TYPE(nullptr, &ptr, sizeof(kDER)));
+  ASSERT_TRUE(val);
+  EXPECT_EQ(V_ASN1_OCTET_STRING, val->type);
+  EXPECT_TRUE(val->value.ptr);
+
+  // Set |val| to a BOOLEAN containing FALSE.
+  ASN1_TYPE_set(val.get(), V_ASN1_BOOLEAN, NULL);
+  EXPECT_EQ(V_ASN1_BOOLEAN, val->type);
+  EXPECT_FALSE(val->value.ptr);
+}
+
 // 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)

diff --git a/crypto/asn1/tasn_fre.c b/crypto/asn1/tasn_fre.c
index a1e7315..44cc125 100644
--- a/crypto/asn1/tasn_fre.c
+++ b/crypto/asn1/tasn_fre.c

@@ -192,7 +192,7 @@
         ASN1_TYPE *typ = (ASN1_TYPE *)*pval;
         utype = typ->type;
         pval = &typ->value.asn1_value;
-        if (!*pval)
+        if (utype != V_ASN1_BOOLEAN && !*pval)
             return;
     } else if (it->itype == ASN1_ITYPE_MSTRING) {
         utype = -1;

diff --git a/crypto/x509/x509_att.c b/crypto/x509/x509_att.c
index 85d65e7..8e13de8 100644
--- a/crypto/x509/x509_att.c
+++ b/crypto/x509/x509_att.c

@@ -62,6 +62,9 @@
 #include <openssl/stack.h>
 #include <openssl/x509.h>
 
+#include "../asn1/asn1_locl.h"
+
+
 int X509at_get_attr_count(const STACK_OF(X509_ATTRIBUTE) *x)
 {
     return sk_X509_ATTRIBUTE_num(x);
@@ -355,7 +358,7 @@
 }
 
 void *X509_ATTRIBUTE_get0_data(X509_ATTRIBUTE *attr, int idx,
-                               int atrtype, void *data)
+                               int atrtype, void *unused)
 {
     ASN1_TYPE *ttmp;
     ttmp = X509_ATTRIBUTE_get0_type(attr, idx);
@@ -365,7 +368,7 @@
         OPENSSL_PUT_ERROR(X509, X509_R_WRONG_TYPE);
         return NULL;
     }
-    return ttmp->value.ptr;
+    return (void *)asn1_type_value_as_pointer(ttmp);
 }
 
 ASN1_TYPE *X509_ATTRIBUTE_get0_type(X509_ATTRIBUTE *attr, int idx)

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 210e57d..57641bb 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc

@@ -3040,3 +3040,83 @@
     }
   }
 }
+
+// Test that extracting fields of an |X509_ALGOR| works correctly.
+TEST(X509Test, X509AlgorExtract) {
+  static const char kTestOID[] = "1.2.840.113554.4.1.72585.2";
+  const struct {
+    int param_type;
+    std::vector<uint8_t> param_der;
+  } kTests[] = {
+      // No parameter.
+      {V_ASN1_UNDEF, {}},
+      // BOOLEAN { TRUE }
+      {V_ASN1_BOOLEAN, {0x01, 0x01, 0xff}},
+      // BOOLEAN { FALSE }
+      {V_ASN1_BOOLEAN, {0x01, 0x01, 0x00}},
+      // OCTET_STRING { "a" }
+      {V_ASN1_OCTET_STRING, {0x04, 0x01, 0x61}},
+      // BIT_STRING { `01` `00` }
+      {V_ASN1_BIT_STRING, {0x03, 0x02, 0x01, 0x00}},
+      // INTEGER { -1 }
+      {V_ASN1_INTEGER, {0x02, 0x01, 0xff}},
+      // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2 }
+      {V_ASN1_OBJECT,
+       {0x06, 0x0c, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7,
+        0x09, 0x02}},
+      // NULL {}
+      {V_ASN1_NULL, {0x05, 0x00}},
+      // SEQUENCE {}
+      {V_ASN1_SEQUENCE, {0x30, 0x00}},
+      // SET {}
+      {V_ASN1_SET, {0x31, 0x00}},
+      // [0] { UTF8String { "a" } }
+      {V_ASN1_OTHER, {0xa0, 0x03, 0x0c, 0x01, 0x61}},
+  };
+  for (const auto &t : kTests) {
+    SCOPED_TRACE(Bytes(t.param_der));
+
+    // Assemble an AlgorithmIdentifier with the parameter.
+    bssl::ScopedCBB cbb;
+    CBB seq, oid;
+    ASSERT_TRUE(CBB_init(cbb.get(), 64));
+    ASSERT_TRUE(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE));
+    ASSERT_TRUE(CBB_add_asn1(&seq, &oid, CBS_ASN1_OBJECT));
+    ASSERT_TRUE(CBB_add_asn1_oid_from_text(&oid, kTestOID, strlen(kTestOID)));
+    ASSERT_TRUE(CBB_add_bytes(&seq, t.param_der.data(), t.param_der.size()));
+    ASSERT_TRUE(CBB_flush(cbb.get()));
+
+    const uint8_t *ptr = CBB_data(cbb.get());
+    bssl::UniquePtr<X509_ALGOR> alg(
+        d2i_X509_ALGOR(nullptr, &ptr, CBB_len(cbb.get())));
+    ASSERT_TRUE(alg);
+
+    const ASN1_OBJECT *obj;
+    int param_type;
+    const void *param_value;
+    X509_ALGOR_get0(&obj, &param_type, &param_value, alg.get());
+
+    EXPECT_EQ(param_type, t.param_type);
+    char oid_buf[sizeof(kTestOID)];
+    ASSERT_EQ(int(sizeof(oid_buf) - 1),
+              OBJ_obj2txt(oid_buf, sizeof(oid_buf), obj,
+                          /*always_return_oid=*/1));
+    EXPECT_STREQ(oid_buf, kTestOID);
+
+    // |param_type| and |param_value| must be consistent with |ASN1_TYPE|.
+    if (param_type == V_ASN1_UNDEF) {
+      EXPECT_EQ(nullptr, param_value);
+    } else {
+      bssl::UniquePtr<ASN1_TYPE> param(ASN1_TYPE_new());
+      ASSERT_TRUE(param);
+      ASSERT_TRUE(ASN1_TYPE_set1(param.get(), param_type, param_value));
+
+      uint8_t *param_der = nullptr;
+      int param_len = i2d_ASN1_TYPE(param.get(), &param_der);
+      ASSERT_GE(param_len, 0);
+      bssl::UniquePtr<uint8_t> free_param_der(param_der);
+
+      EXPECT_EQ(Bytes(param_der, param_len), Bytes(t.param_der));
+    }
+  }
+}

diff --git a/crypto/x509/x_algor.c b/crypto/x509/x_algor.c
index a7d5dd6..0406628 100644
--- a/crypto/x509/x_algor.c
+++ b/crypto/x509/x_algor.c

@@ -61,6 +61,8 @@
 #include <openssl/digest.h>
 #include <openssl/obj.h>
 
+#include "../asn1/asn1_locl.h"
+
 
 ASN1_SEQUENCE(X509_ALGOR) = {
         ASN1_SIMPLE(X509_ALGOR, algorithm, ASN1_OBJECT),
@@ -111,10 +113,10 @@
     }
     if (out_param_type != NULL) {
         int type = V_ASN1_UNDEF;
-        void *value = NULL;
+        const void *value = NULL;
         if (alg->parameter != NULL) {
             type = alg->parameter->type;
-            value = alg->parameter->value.ptr;
+            value = asn1_type_value_as_pointer(alg->parameter);
         }
         *out_param_type = type;
         if (out_param_value != NULL) {

diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 4857e3e..b32c0ca 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h

@@ -322,10 +322,11 @@
 // typically used used for ANY types. It contains a |type| field and a |value|
 // union dependent on |type|.
 //
-// WARNING: This struct has a complex representation. Callers performing
-// non-trivial operations on this type are encouraged to use |CBS| and |CBB|
-// from <openssl/bytestring.h>, and convert to or from |ASN1_TYPE| with
-// |d2i_ASN1_TYPE| or |i2d_ASN1_TYPE|.
+// WARNING: This struct has a complex representation. Callers must not construct
+// |ASN1_TYPE| values manually. Use |ASN1_TYPE_set| and |ASN1_TYPE_set1|
+// instead. Additionally, callers performing non-trivial operations on this type
+// are encouraged to use |CBS| and |CBB| from <openssl/bytestring.h>, and
+// convert to or from |ASN1_TYPE| with |d2i_ASN1_TYPE| or |i2d_ASN1_TYPE|.
 //
 // The |type| field corresponds to the tag of the ASN.1 element being
 // represented:
@@ -345,11 +346,7 @@
 // If |type| is |V_ASN1_NULL|, the tag is NULL. |value| contains a NULL pointer.
 //
 // If |type| is |V_ASN1_BOOLEAN|, the tag is BOOLEAN. |value| contains an
-// |ASN1_BOOLEAN|. While |ASN1_BOOLEAN| is an |int|, any unused bits of |value|
-// must also be zero, so callers must not construct these values manually.
-//
-// TODO(davidben): Fix code which relies on this. It's anything which looks at
-// |value.ptr| or |value.asn1_value| unconditionally.
+// |ASN1_BOOLEAN|.
 //
 // If |type| is |V_ASN1_SEQUENCE|, |V_ASN1_SET|, or |V_ASN1_OTHER|, the tag is
 // SEQUENCE, SET, or some non-universal tag, respectively. |value| is an

diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 3f26792..ccf2c0f 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h

@@ -1560,7 +1560,7 @@
 OPENSSL_EXPORT int X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype,
                                             const void *data, int len);
 OPENSSL_EXPORT void *X509_ATTRIBUTE_get0_data(X509_ATTRIBUTE *attr, int idx,
-                                              int atrtype, void *data);
+                                              int atrtype, void *unused);
 OPENSSL_EXPORT int X509_ATTRIBUTE_count(X509_ATTRIBUTE *attr);
 OPENSSL_EXPORT ASN1_OBJECT *X509_ATTRIBUTE_get0_object(X509_ATTRIBUTE *attr);
 OPENSSL_EXPORT ASN1_TYPE *X509_ATTRIBUTE_get0_type(X509_ATTRIBUTE *attr,