Document and test X509_ATTRIBUTE creation functions. This is mostly to confirm the STACK_OF(ASN1_TYPE) was created the right number of times. Change-Id: I30c32f91cb6091e63bfcaebb0fe966270e503d93 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46984 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/x509/x509_att.c b/crypto/x509/x509_att.c index 90f722a..3773829 100644 --- a/crypto/x509/x509_att.c +++ b/crypto/x509/x509_att.c
@@ -218,7 +218,7 @@ } X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_NID(X509_ATTRIBUTE **attr, int nid, - int atrtype, const void *data, + int attrtype, const void *data, int len) { const ASN1_OBJECT *obj; @@ -228,12 +228,12 @@ OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_NID); return (NULL); } - return X509_ATTRIBUTE_create_by_OBJ(attr, obj, atrtype, data, len); + return X509_ATTRIBUTE_create_by_OBJ(attr, obj, attrtype, data, len); } X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_OBJ(X509_ATTRIBUTE **attr, const ASN1_OBJECT *obj, - int atrtype, const void *data, + int attrtype, const void *data, int len) { X509_ATTRIBUTE *ret; @@ -248,7 +248,7 @@ if (!X509_ATTRIBUTE_set1_object(ret, obj)) goto err; - if (!X509_ATTRIBUTE_set1_data(ret, atrtype, data, len)) + if (!X509_ATTRIBUTE_set1_data(ret, attrtype, data, len)) goto err; if ((attr != NULL) && (*attr == NULL)) @@ -261,17 +261,17 @@ } X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_txt(X509_ATTRIBUTE **attr, - const char *atrname, int type, + const char *attrname, int type, const unsigned char *bytes, int len) { ASN1_OBJECT *obj; X509_ATTRIBUTE *nattr; - obj = OBJ_txt2obj(atrname, 0); + obj = OBJ_txt2obj(attrname, 0); if (obj == NULL) { OPENSSL_PUT_ERROR(X509, X509_R_INVALID_FIELD_NAME); - ERR_add_error_data(2, "name=", atrname); + ERR_add_error_data(2, "name=", attrname); return (NULL); } nattr = X509_ATTRIBUTE_create_by_OBJ(attr, obj, type, bytes, len); @@ -339,7 +339,7 @@ return 0; } -int X509_ATTRIBUTE_count(X509_ATTRIBUTE *attr) +int X509_ATTRIBUTE_count(const X509_ATTRIBUTE *attr) { return sk_ASN1_TYPE_num(attr->set); } @@ -352,13 +352,13 @@ } void *X509_ATTRIBUTE_get0_data(X509_ATTRIBUTE *attr, int idx, - int atrtype, void *unused) + int attrtype, void *unused) { ASN1_TYPE *ttmp; ttmp = X509_ATTRIBUTE_get0_type(attr, idx); if (!ttmp) return NULL; - if (atrtype != ASN1_TYPE_get(ttmp)) { + if (attrtype != ASN1_TYPE_get(ttmp)) { OPENSSL_PUT_ERROR(X509, X509_R_WRONG_TYPE); return NULL; }
diff --git a/crypto/x509/x509_req.c b/crypto/x509/x509_req.c index d15c05f..202ae3f 100644 --- a/crypto/x509/x509_req.c +++ b/crypto/x509/x509_req.c
@@ -251,31 +251,31 @@ } int X509_REQ_add1_attr_by_OBJ(X509_REQ *req, - const ASN1_OBJECT *obj, int type, - const unsigned char *bytes, int len) + const ASN1_OBJECT *obj, int attrtype, + const unsigned char *data, int len) { if (X509at_add1_attr_by_OBJ(&req->req_info->attributes, obj, - type, bytes, len)) + attrtype, data, len)) return 1; return 0; } int X509_REQ_add1_attr_by_NID(X509_REQ *req, - int nid, int type, - const unsigned char *bytes, int len) + int nid, int attrtype, + const unsigned char *data, int len) { if (X509at_add1_attr_by_NID(&req->req_info->attributes, nid, - type, bytes, len)) + attrtype, data, len)) return 1; return 0; } int X509_REQ_add1_attr_by_txt(X509_REQ *req, - const char *attrname, int type, - const unsigned char *bytes, int len) + const char *attrname, int attrtype, + const unsigned char *data, int len) { if (X509at_add1_attr_by_txt(&req->req_info->attributes, attrname, - type, bytes, len)) + attrtype, data, len)) return 1; return 0; }
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 57641bb..048e9ce 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -33,6 +33,9 @@ #include "../internal.h" #include "../test/test_util.h" #include "../x509v3/internal.h" +#include "openssl/asn1.h" +#include "openssl/base.h" +#include "openssl/nid.h" std::string GetTestData(const char *path); @@ -3120,3 +3123,85 @@ } } } + +// Test the various |X509_ATTRIBUTE| creation functions. +TEST(X509Test, Attribute) { + // The friendlyName attribute has a BMPString value. See RFC2985, + // section 5.5.1. + 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) { + 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); + 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)); + } + + EXPECT_FALSE(X509_ATTRIBUTE_get0_type(attr, 2)); + }; + + bssl::UniquePtr<ASN1_STRING> str(ASN1_STRING_type_new(V_ASN1_BMPSTRING)); + ASSERT_TRUE(str); + ASSERT_TRUE(ASN1_STRING_set(str.get(), kTest1, sizeof(kTest1))); + + // Test |X509_ATTRIBUTE_create|. + bssl::UniquePtr<X509_ATTRIBUTE> attr( + 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); + + // Test the |MBSTRING_*| form of |X509_ATTRIBUTE_set1_data|. + attr.reset(X509_ATTRIBUTE_new()); + ASSERT_TRUE(attr); + ASSERT_TRUE( + 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); + + // 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); + + // Test the |ASN1_TYPE| form of |X509_ATTRIBUTE_set1_data|. + attr.reset(X509_ATTRIBUTE_new()); + ASSERT_TRUE(attr); + ASSERT_TRUE( + X509_ATTRIBUTE_set1_object(attr.get(), OBJ_nid2obj(NID_friendlyName))); + str.reset(ASN1_STRING_type_new(V_ASN1_BMPSTRING)); + ASSERT_TRUE(str); + 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); +}
diff --git a/crypto/x509/x_attrib.c b/crypto/x509/x_attrib.c index 3ee8bd4..91b3ee8 100644 --- a/crypto/x509/x_attrib.c +++ b/crypto/x509/x_attrib.c
@@ -70,7 +70,7 @@ IMPLEMENT_ASN1_FUNCTIONS(X509_ATTRIBUTE) IMPLEMENT_ASN1_DUP_FUNCTION(X509_ATTRIBUTE) -X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int atrtype, void *value) +X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int attrtype, void *value) { ASN1_OBJECT *obj = OBJ_nid2obj(nid); if (obj == NULL) { @@ -88,7 +88,7 @@ goto err; } - ASN1_TYPE_set(val, atrtype, value); + ASN1_TYPE_set(val, attrtype, value); return ret; err:
diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 168cac4..3eb7501 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h
@@ -980,7 +980,12 @@ DECLARE_ASN1_FUNCTIONS(X509_REQ) DECLARE_ASN1_FUNCTIONS(X509_ATTRIBUTE) -OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int atrtype, + +// X509_ATTRIBUTE_create returns a newly-allocated |X509_ATTRIBUTE|, or NULL on +// error. The attribute has type |nid| and contains a single value determined by +// |attrtype| and |value|, which are interpreted as in |ASN1_TYPE_set|. Note +// this function takes ownership of |value|. +OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int attrtype, void *value); DECLARE_ASN1_FUNCTIONS(X509_EXTENSION) @@ -1283,16 +1288,30 @@ // TODO(https://crbug.com/boringssl/407): |attr| should be const. OPENSSL_EXPORT int X509_REQ_add1_attr(X509_REQ *req, X509_ATTRIBUTE *attr); +// X509_REQ_add1_attr_by_OBJ appends a new attribute to |req| with type |obj|. +// It returns one on success and zero on error. The value is determined by +// |X509_ATTRIBUTE_set1_data|. +// +// WARNING: The interpretation of |attrtype|, |data|, and |len| is complex and +// error-prone. See |X509_ATTRIBUTE_set1_data| for details. OPENSSL_EXPORT int X509_REQ_add1_attr_by_OBJ(X509_REQ *req, - const ASN1_OBJECT *obj, int type, - const unsigned char *bytes, + const ASN1_OBJECT *obj, + int attrtype, + const unsigned char *data, int len); -OPENSSL_EXPORT int X509_REQ_add1_attr_by_NID(X509_REQ *req, int nid, int type, - const unsigned char *bytes, + +// X509_REQ_add1_attr_by_NID behaves like |X509_REQ_add1_attr_by_OBJ| except the +// attribute type is determined by |nid|. +OPENSSL_EXPORT int X509_REQ_add1_attr_by_NID(X509_REQ *req, int nid, + int attrtype, + const unsigned char *data, int len); + +// X509_REQ_add1_attr_by_txt behaves like |X509_REQ_add1_attr_by_OBJ| except the +// attribute type is determined by calling |OBJ_txt2obj| with |attrname|. OPENSSL_EXPORT int X509_REQ_add1_attr_by_txt(X509_REQ *req, - const char *attrname, int type, - const unsigned char *bytes, + const char *attrname, int attrtype, + const unsigned char *data, int len); // X509_CRL_set_version sets |crl|'s version to |version|, which should be one @@ -1632,22 +1651,96 @@ OPENSSL_EXPORT void *X509at_get0_data_by_OBJ(STACK_OF(X509_ATTRIBUTE) *x, ASN1_OBJECT *obj, int lastpos, int type); + +// X509_ATTRIBUTE_create_by_NID returns a newly-allocated |X509_ATTRIBUTE| of +// type |nid|, or NULL on error. The value is determined as in +// |X509_ATTRIBUTE_set1_data|. +// +// If |attr| is non-NULL, the resulting |X509_ATTRIBUTE| is also written to +// |*attr|. If |*attr| was non-NULL when the function was called, |*attr| is +// reused instead of creating a new object. +// +// WARNING: The interpretation of |attrtype|, |data|, and |len| is complex and +// error-prone. See |X509_ATTRIBUTE_set1_data| for details. +// +// WARNING: The object reuse form is deprecated and may be removed in the +// future. It also currently incorrectly appends to the reused object's value +// set rather than overwriting it. OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_NID( - X509_ATTRIBUTE **attr, int nid, int atrtype, const void *data, int len); + X509_ATTRIBUTE **attr, int nid, int attrtype, const void *data, int len); + +// X509_ATTRIBUTE_create_by_OBJ behaves like |X509_ATTRIBUTE_create_by_NID| +// except the attribute's type is determined by |obj|. OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_OBJ( - X509_ATTRIBUTE **attr, const ASN1_OBJECT *obj, int atrtype, + X509_ATTRIBUTE **attr, const ASN1_OBJECT *obj, int attrtype, const void *data, int len); + +// X509_ATTRIBUTE_create_by_txt behaves like |X509_ATTRIBUTE_create_by_NID| +// except the attribute's type is determined by calling |OBJ_txt2obj| with +// |attrname|. OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_txt( - X509_ATTRIBUTE **attr, const char *atrname, int type, + X509_ATTRIBUTE **attr, const char *attrname, int type, const unsigned char *bytes, int len); + +// X509_ATTRIBUTE_set1_object sets |attr|'s type to |obj|. It returns one on +// success and zero on error. OPENSSL_EXPORT int X509_ATTRIBUTE_set1_object(X509_ATTRIBUTE *attr, const ASN1_OBJECT *obj); + +// 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. +// +// TODO(davidben): Document |ASN1_STRING_set_by_NID| so the reference is useful. +// +// 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. +// +// WARNING: Despite the name, this function appends to |attr|'s value set, +// rather than overwriting it. To overwrite the value set, create a new +// |X509_ATTRIBUTE| with |X509_ATTRIBUTE_new|. +// +// WARNING: If using the |MBSTRING_*| form, pass a length rather than relying on +// |strlen|. In particular, |strlen| will not behave correctly if the input is +// |MBSTRING_BMP| or |MBSTRING_UNIV|. +// +// WARNING: This function currently misinterprets |V_ASN1_OTHER| as an +// |MBSTRING_*| constant. This matches OpenSSL but means it is impossible to +// construct a value with a non-universal tag. OPENSSL_EXPORT int X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype, const void *data, int len); + +// X509_ATTRIBUTE_get0_data returns the |idx|th value of |attr| in a +// type-specific representation to |attrtype|, or NULL if out of bounds or the +// type does not match. |attrtype| is one of the type values in |ASN1_TYPE|. On +// match, the return value uses the same representation as |ASN1_TYPE_set0|. See +// |ASN1_TYPE| for details. OPENSSL_EXPORT void *X509_ATTRIBUTE_get0_data(X509_ATTRIBUTE *attr, int idx, - int atrtype, void *unused); -OPENSSL_EXPORT int X509_ATTRIBUTE_count(X509_ATTRIBUTE *attr); + int attrtype, void *unused); + +// X509_ATTRIBUTE_count returns the number of values in |attr|. +OPENSSL_EXPORT int X509_ATTRIBUTE_count(const X509_ATTRIBUTE *attr); + +// X509_ATTRIBUTE_get0_object returns the type of |attr|. OPENSSL_EXPORT ASN1_OBJECT *X509_ATTRIBUTE_get0_object(X509_ATTRIBUTE *attr); + +// X509_ATTRIBUTE_get0_type returns the |idx|th value in |attr|, or NULL if out +// of bounds. Note this function returns one of |attr|'s values, not the type. OPENSSL_EXPORT ASN1_TYPE *X509_ATTRIBUTE_get0_type(X509_ATTRIBUTE *attr, int idx); @@ -1742,6 +1835,7 @@ BORINGSSL_MAKE_DELETER(X509, X509_free) BORINGSSL_MAKE_UP_REF(X509, X509_up_ref) BORINGSSL_MAKE_DELETER(X509_ALGOR, X509_ALGOR_free) +BORINGSSL_MAKE_DELETER(X509_ATTRIBUTE, X509_ATTRIBUTE_free) BORINGSSL_MAKE_DELETER(X509_CRL, X509_CRL_free) BORINGSSL_MAKE_UP_REF(X509_CRL, X509_CRL_up_ref) BORINGSSL_MAKE_DELETER(X509_CRL_METHOD, X509_CRL_METHOD_free)