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>
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 @@
// 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 @@
}
}
+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 @@
// 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);
diff --git a/crypto/x509/x509_att.c b/crypto/x509/x509_att.c
index 062168e..e3d0c6a 100644
--- a/crypto/x509/x509_att.c
+++ b/crypto/x509/x509_att.c
@@ -137,54 +137,57 @@
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 & MBSTRING_FLAG) {
- stmp = ASN1_STRING_set_by_NID(NULL, data, len, attrtype,
- OBJ_obj2nid(attr->object));
- if (!stmp) {
- 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)) {
- 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);
+ // 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;
}
- if (!(ttmp = ASN1_TYPE_new())) {
- goto err;
+
+ ASN1_TYPE *typ = ASN1_TYPE_new();
+ if (typ == NULL) {
+ return 0;
}
- if ((len == -1) && !(attrtype & MBSTRING_FLAG)) {
- if (!ASN1_TYPE_set1(ttmp, attrtype, data)) {
+
+ // This function is several functions in one.
+ if (attrtype & MBSTRING_FLAG) {
+ // |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);
goto err;
}
+ 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;
+ }
+ asn1_type_set0_string(typ, str);
} else {
- ASN1_TYPE_set(ttmp, atype, stmp);
- stmp = NULL;
+ // |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;
+ }
}
- 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;
}
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index f8c930f..a590e08 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -3832,46 +3832,62 @@
// 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));
+ 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)));
- // 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, 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++;
+ }
- // |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);
+ 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 @@
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 @@
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 @@
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
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 66668a6..852a96f 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -2101,21 +2101,21 @@
// 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.
