diff --git a/crypto/asn1/a_strex.c b/crypto/asn1/a_strex.c index 7e9afad0b5..516f7df54d 100644 --- a/crypto/asn1/a_strex.c +++ b/crypto/asn1/a_strex.c @@ -68,6 +68,7 @@ #include #include "../bytestring/internal.h" +#include "../internal.h" #include "internal.h" @@ -238,22 +239,8 @@ static int do_dump(unsigned long flags, BIO *out, const ASN1_STRING *str) { // 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 0c2fd136d3..af603a3e8e 100644 --- a/crypto/asn1/a_type.c +++ b/crypto/asn1/a_type.c @@ -56,7 +56,8 @@ #include -#include +#include + #include #include #include @@ -89,6 +90,23 @@ const void *asn1_type_value_as_pointer(const ASN1_TYPE *a) { } } +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 414b5a97de..57e69668d3 100644 --- a/crypto/asn1/internal.h +++ b/crypto/asn1/internal.h @@ -210,6 +210,10 @@ void asn1_encoding_clear(ASN1_ENCODING *enc); // 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 062168eaf7..e3d0c6a595 100644 --- a/crypto/x509/x509_att.c +++ b/crypto/x509/x509_att.c @@ -137,54 +137,57 @@ int X509_ATTRIBUTE_set1_object(X509_ATTRIBUTE *attr, const ASN1_OBJECT *obj) { 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 == 0) { + // 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; + } + + ASN1_TYPE *typ = ASN1_TYPE_new(); + if (typ == NULL) { + return 0; + } + + // This function is several functions in one. if (attrtype & MBSTRING_FLAG) { - stmp = ASN1_STRING_set_by_NID(NULL, data, len, attrtype, - OBJ_obj2nid(attr->object)); - if (!stmp) { + // |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); - 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)) { + 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; } - 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); - return 1; - } - if (!(ttmp = ASN1_TYPE_new())) { - goto err; - } - if ((len == -1) && !(attrtype & MBSTRING_FLAG)) { - if (!ASN1_TYPE_set1(ttmp, attrtype, data)) { + asn1_type_set0_string(typ, str); + } else { + // |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; } - } else { - ASN1_TYPE_set(ttmp, atype, stmp); - stmp = NULL; } - 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 f8c930fb54..a590e08769 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -3832,46 +3832,62 @@ TEST(X509Test, X509AlgorExtract) { // 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)); - - // 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( - 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); + 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))); + + // |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( + 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++; + } + + 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 str(ASN1_STRING_type_new(V_ASN1_BMPSTRING)); @@ -3883,7 +3899,7 @@ TEST(X509Test, Attribute) { 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 @@ TEST(X509Test, Attribute) { 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 @@ TEST(X509Test, Attribute) { 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 66668a635d..852a96f5e9 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2101,21 +2101,21 @@ OPENSSL_EXPORT int X509_ATTRIBUTE_set1_object(X509_ATTRIBUTE *attr, // 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.