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)
