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, ¶m_type, ¶m_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(), ¶m_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,
