diff --git a/crypto/asn1/tasn_new.c b/crypto/asn1/tasn_new.c index 143c47b924..7d10319db4 100644 --- a/crypto/asn1/tasn_new.c +++ b/crypto/asn1/tasn_new.c @@ -194,16 +194,9 @@ static int asn1_item_ex_combine_new(ASN1_VALUE **pval, const ASN1_ITEM *it, } static void asn1_item_clear(ASN1_VALUE **pval, const ASN1_ITEM *it) { - const ASN1_EXTERN_FUNCS *ef; - switch (it->itype) { case ASN1_ITYPE_EXTERN: - ef = it->funcs; - if (ef && ef->asn1_ex_clear) { - ef->asn1_ex_clear(pval, it); - } else { - *pval = NULL; - } + *pval = NULL; break; case ASN1_ITYPE_PRIMITIVE: @@ -285,7 +278,7 @@ static int ASN1_primitive_new(ASN1_VALUE **pval, const ASN1_ITEM *it) { } switch (utype) { case V_ASN1_OBJECT: - *pval = (ASN1_VALUE *)OBJ_nid2obj(NID_undef); + *pval = (ASN1_VALUE *)OBJ_get_undef(); return 1; case V_ASN1_BOOLEAN: diff --git a/crypto/mem.c b/crypto/mem.c index 7235537a19..2b0085a5c7 100644 --- a/crypto/mem.c +++ b/crypto/mem.c @@ -162,31 +162,6 @@ int CRYPTO_set_mem_functions( return 1; } -// kBoringSSLBinaryTag is a distinctive byte sequence to identify binaries that -// are linking in BoringSSL and, roughly, what version they are using. -static const uint8_t kBoringSSLBinaryTag[18] = { - // 16 bytes of magic tag. - 0x8c, - 0x62, - 0x20, - 0x0b, - 0xd2, - 0xa0, - 0x72, - 0x58, - 0x44, - 0xa8, - 0x96, - 0x69, - 0xad, - 0x55, - 0x7e, - 0xec, - // Current source iteration. Incremented ~monthly. - 3, - 0, -}; - void *OPENSSL_malloc(size_t size) { if (malloc_impl != NULL) { assert(OPENSSL_memory_alloc == NULL); @@ -208,14 +183,6 @@ void *OPENSSL_malloc(size_t size) { } if (size + OPENSSL_MALLOC_PREFIX < size) { - // |OPENSSL_malloc| is a central function in BoringSSL thus a reference to - // |kBoringSSLBinaryTag| is created here so that the tag isn't discarded by - // the linker. The following is sufficient to stop GCC, Clang, and MSVC - // optimising away the reference at the time of writing. Since this - // probably results in an actual memory reference, it is put in this very - // rare code path. - uint8_t unused = *(volatile uint8_t *)kBoringSSLBinaryTag; - (void) unused; goto err; } diff --git a/crypto/obj/obj.c b/crypto/obj/obj.c index cfe4e11bc1..848ac57ea8 100644 --- a/crypto/obj/obj.c +++ b/crypto/obj/obj.c @@ -187,12 +187,19 @@ size_t OBJ_length(const ASN1_OBJECT *obj) { return (size_t)obj->length; } +static const ASN1_OBJECT *get_builtin_object(int nid) { + // |NID_undef| is stored separately, so all the indices are off by one. The + // caller of this function must have a valid built-in, non-undef NID. + BSSL_CHECK(nid > 0 && nid < NUM_NID); + return &kObjects[nid - 1]; +} + // obj_cmp is called to search the kNIDsInOIDOrder array. The |key| argument is // an |ASN1_OBJECT|* that we're looking for and |element| is a pointer to an // unsigned int in the array. static int obj_cmp(const void *key, const void *element) { uint16_t nid = *((const uint16_t *)element); - return OBJ_cmp(key, &kObjects[nid]); + return OBJ_cmp(key, get_builtin_object(nid)); } int OBJ_obj2nid(const ASN1_OBJECT *obj) { @@ -223,7 +230,7 @@ int OBJ_obj2nid(const ASN1_OBJECT *obj) { return NID_undef; } - return kObjects[*nid_ptr].nid; + return get_builtin_object(*nid_ptr)->nid; } int OBJ_cbs2nid(const CBS *cbs) { @@ -246,7 +253,7 @@ static int short_name_cmp(const void *key, const void *element) { const char *name = (const char *)key; uint16_t nid = *((const uint16_t *)element); - return strcmp(name, kObjects[nid].sn); + return strcmp(name, get_builtin_object(nid)->sn); } int OBJ_sn2nid(const char *short_name) { @@ -271,7 +278,7 @@ int OBJ_sn2nid(const char *short_name) { return NID_undef; } - return kObjects[*nid_ptr].nid; + return get_builtin_object(*nid_ptr)->nid; } // long_name_cmp is called to search the kNIDsInLongNameOrder array. The @@ -281,7 +288,7 @@ static int long_name_cmp(const void *key, const void *element) { const char *name = (const char *)key; uint16_t nid = *((const uint16_t *)element); - return strcmp(name, kObjects[nid].ln); + return strcmp(name, get_builtin_object(nid)->ln); } int OBJ_ln2nid(const char *long_name) { @@ -305,7 +312,7 @@ int OBJ_ln2nid(const char *long_name) { return NID_undef; } - return kObjects[*nid_ptr].nid; + return get_builtin_object(*nid_ptr)->nid; } int OBJ_txt2nid(const char *s) { @@ -332,12 +339,29 @@ OPENSSL_EXPORT int OBJ_nid2cbb(CBB *out, int nid) { return 1; } +const ASN1_OBJECT *OBJ_get_undef(void) { + static const ASN1_OBJECT kUndef = { + /*sn=*/SN_undef, + /*ln=*/LN_undef, + /*nid=*/NID_undef, + /*length=*/0, + /*data=*/NULL, + /*flags=*/0, + }; + return &kUndef; +} + ASN1_OBJECT *OBJ_nid2obj(int nid) { - if (nid >= 0 && nid < NUM_NID) { - if (nid != NID_undef && kObjects[nid].nid == NID_undef) { + if (nid == NID_undef) { + return (ASN1_OBJECT *)OBJ_get_undef(); + } + + if (nid > 0 && nid < NUM_NID) { + const ASN1_OBJECT *obj = get_builtin_object(nid); + if (nid != NID_undef && obj->nid == NID_undef) { goto err; } - return (ASN1_OBJECT *)&kObjects[nid]; + return (ASN1_OBJECT *)obj; } CRYPTO_STATIC_MUTEX_lock_read(&global_added_lock); diff --git a/crypto/obj/obj_dat.h b/crypto/obj/obj_dat.h index 0da1e911d1..7d621f180d 100644 --- a/crypto/obj/obj_dat.h +++ b/crypto/obj/obj_dat.h @@ -7221,7 +7221,6 @@ static const uint8_t kObjectData[] = { }; static const ASN1_OBJECT kObjects[NUM_NID] = { - {"UNDEF", "undefined", NID_undef, 0, NULL, 0}, {"rsadsi", "RSA Data Security, Inc.", NID_rsadsi, 6, &kObjectData[0], 0}, {"pkcs", "RSA Data Security, Inc. PKCS", NID_pkcs, 7, &kObjectData[6], 0}, {"MD2", "md2", NID_md2, 8, &kObjectData[13], 0}, @@ -9101,7 +9100,6 @@ static const uint16_t kNIDsInShortNameOrder[] = { 143 /* SXNetID */, 981 /* SecP256r1Kyber768Draft00 */, 458 /* UID */, - 0 /* UNDEF */, 948 /* X25519 */, 982 /* X25519Kyber768Draft00 */, 961 /* X448 */, @@ -10815,7 +10813,6 @@ static const uint16_t kNIDsInLongNameOrder[] = { 106 /* title */, 682 /* tpBasis */, 436 /* ucl */, - 0 /* undefined */, 888 /* uniqueMember */, 55 /* unstructuredAddress */, 49 /* unstructuredName */, diff --git a/crypto/obj/obj_test.cc b/crypto/obj/obj_test.cc index 08796e2b94..abea30d7e9 100644 --- a/crypto/obj/obj_test.cc +++ b/crypto/obj/obj_test.cc @@ -56,6 +56,10 @@ TEST(ObjTest, TestBasic) { }; CBS_init(&cbs, kUnknownDER, sizeof(kUnknownDER)); ASSERT_EQ(NID_undef, OBJ_cbs2nid(&cbs)); + + EXPECT_EQ(NID_undef, OBJ_sn2nid("UNDEF")); + EXPECT_EQ(NID_undef, OBJ_ln2nid("undefined")); + EXPECT_EQ(OBJ_get_undef(), OBJ_nid2obj(NID_undef)); } TEST(ObjTest, TestSignatureAlgorithms) { diff --git a/crypto/obj/objects.go b/crypto/obj/objects.go index a4d8395aa2..7573db9f92 100644 --- a/crypto/obj/objects.go +++ b/crypto/obj/objects.go @@ -614,6 +614,12 @@ func writeData(path string, objs *objects) error { // Emit an ASN1_OBJECT for each object. fmt.Fprintf(&b, "\nstatic const ASN1_OBJECT kObjects[NUM_NID] = {\n") for nid, obj := range objs.byNID { + // Skip the entry for NID_undef. It is stored separately, so that + // OBJ_get_undef avoids pulling in the table. + if nid == 0 { + continue + } + if len(obj.name) == 0 { fmt.Fprintf(&b, "{NULL, NULL, NID_undef, 0, NULL, 0},\n") continue @@ -640,7 +646,11 @@ func writeData(path string, objs *objects) error { fmt.Fprintf(&b, "\nstatic const uint16_t kNIDsInShortNameOrder[] = {\n") for _, nid := range nids { - fmt.Fprintf(&b, "%d /* %s */,\n", nid, objs.byNID[nid].shortName) + // Including NID_undef in the table does not do anything. Whether OBJ_sn2nid + // finds the object or not, it will return NID_undef. + if nid != 0 { + fmt.Fprintf(&b, "%d /* %s */,\n", nid, objs.byNID[nid].shortName) + } } fmt.Fprintf(&b, "};\n") @@ -656,7 +666,11 @@ func writeData(path string, objs *objects) error { fmt.Fprintf(&b, "\nstatic const uint16_t kNIDsInLongNameOrder[] = {\n") for _, nid := range nids { - fmt.Fprintf(&b, "%d /* %s */,\n", nid, objs.byNID[nid].longName) + // Including NID_undef in the table does not do anything. Whether OBJ_ln2nid + // finds the object or not, it will return NID_undef. + if nid != 0 { + fmt.Fprintf(&b, "%d /* %s */,\n", nid, objs.byNID[nid].longName) + } } fmt.Fprintf(&b, "};\n") diff --git a/crypto/x509/algorithm.c b/crypto/x509/algorithm.c index 2c69ca47a9..5f42231b32 100644 --- a/crypto/x509/algorithm.c +++ b/crypto/x509/algorithm.c @@ -122,8 +122,7 @@ int x509_digest_sign_algorithm(EVP_MD_CTX *ctx, X509_ALGOR *algor) { // it. int paramtype = (EVP_PKEY_id(pkey) == EVP_PKEY_RSA) ? V_ASN1_NULL : V_ASN1_UNDEF; - X509_ALGOR_set0(algor, OBJ_nid2obj(sign_nid), paramtype, NULL); - return 1; + return X509_ALGOR_set0(algor, OBJ_nid2obj(sign_nid), paramtype, NULL); } int x509_digest_verify_init(EVP_MD_CTX *ctx, const X509_ALGOR *sigalg, diff --git a/crypto/x509/rsa_pss.c b/crypto/x509/rsa_pss.c index 232748025e..7b4680c573 100644 --- a/crypto/x509/rsa_pss.c +++ b/crypto/x509/rsa_pss.c @@ -149,7 +149,9 @@ static int rsa_md_to_mgf1(X509_ALGOR **palg, const EVP_MD *mgf1md) { if (!*palg) { goto err; } - X509_ALGOR_set0(*palg, OBJ_nid2obj(NID_mgf1), V_ASN1_SEQUENCE, stmp); + if (!X509_ALGOR_set0(*palg, OBJ_nid2obj(NID_mgf1), V_ASN1_SEQUENCE, stmp)) { + goto err; + } stmp = NULL; err: @@ -244,7 +246,9 @@ int x509_rsa_ctx_to_pss(EVP_MD_CTX *ctx, X509_ALGOR *algor) { goto err; } - X509_ALGOR_set0(algor, OBJ_nid2obj(NID_rsassaPss), V_ASN1_SEQUENCE, os); + if (!X509_ALGOR_set0(algor, OBJ_nid2obj(NID_rsassaPss), V_ASN1_SEQUENCE, os)) { + goto err; + } os = NULL; ret = 1; diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 910fc63022..ae2385c39e 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -6638,7 +6638,7 @@ TEST(X509Test, AddUnserializableExtension) { MakeTestCert("Issuer", "Subject", key.get(), /*is_ca=*/true); ASSERT_TRUE(x509); bssl::UniquePtr ext(X509_EXTENSION_new()); - ASSERT_TRUE(X509_EXTENSION_set_object(ext.get(), OBJ_nid2obj(NID_undef))); + ASSERT_TRUE(X509_EXTENSION_set_object(ext.get(), OBJ_get_undef())); EXPECT_FALSE(X509_add_ext(x509.get(), ext.get(), /*loc=*/-1)); } @@ -6744,3 +6744,255 @@ TEST(X509Test, X509_OBJECT_heap) { ASSERT_TRUE(x509_object); X509_OBJECT_free(x509_object); } + +TEST(X509Test, NameAttributeValues) { + // 1.2.840.113554.4.1.72585.0. We use an unrecognized OID because using an + // arbitrary ASN.1 type as the value for commonName is invalid. Our parser + // does not check this, but best to avoid unrelated errors in tests, in case + // we decide to later. + static const uint8_t kOID[] = {0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, + 0x04, 0x01, 0x84, 0xb7, 0x09, 0x00}; + static const char kOIDText[] = "1.2.840.113554.4.1.72585.0"; + + auto encode_single_attribute_name = + [](CBS_ASN1_TAG tag, + const std::string &contents) -> std::vector { + bssl::ScopedCBB cbb; + CBB seq, rdn, attr, attr_type, attr_value; + if (!CBB_init(cbb.get(), 128) || + !CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE) || + !CBB_add_asn1(&seq, &rdn, CBS_ASN1_SET) || + !CBB_add_asn1(&rdn, &attr, CBS_ASN1_SEQUENCE) || + !CBB_add_asn1(&attr, &attr_type, CBS_ASN1_OBJECT) || + !CBB_add_bytes(&attr_type, kOID, sizeof(kOID)) || + !CBB_add_asn1(&attr, &attr_value, tag) || + !CBB_add_bytes(&attr_value, + reinterpret_cast(contents.data()), + contents.size()) || + !CBB_flush(cbb.get())) { + ADD_FAILURE() << "Could not encode name"; + return {}; + }; + return std::vector(CBB_data(cbb.get()), + CBB_data(cbb.get()) + CBB_len(cbb.get())); + }; + + const struct { + CBS_ASN1_TAG der_tag; + std::string der_contents; + int str_type; + std::string str_contents; + } kTests[] = { + // String types are parsed as string types. + {CBS_ASN1_BITSTRING, std::string("\0", 1), V_ASN1_BIT_STRING, ""}, + {CBS_ASN1_UTF8STRING, "abc", V_ASN1_UTF8STRING, "abc"}, + {CBS_ASN1_NUMERICSTRING, "123", V_ASN1_NUMERICSTRING, "123"}, + {CBS_ASN1_PRINTABLESTRING, "abc", V_ASN1_PRINTABLESTRING, "abc"}, + {CBS_ASN1_T61STRING, "abc", V_ASN1_T61STRING, "abc"}, + {CBS_ASN1_IA5STRING, "abc", V_ASN1_IA5STRING, "abc"}, + {CBS_ASN1_UNIVERSALSTRING, std::string("\0\0\0a", 4), + V_ASN1_UNIVERSALSTRING, std::string("\0\0\0a", 4)}, + {CBS_ASN1_BMPSTRING, std::string("\0a", 2), V_ASN1_BMPSTRING, + std::string("\0a", 2)}, + + // ENUMERATED is supported but, currently, INTEGER is not. + {CBS_ASN1_ENUMERATED, "\x01", V_ASN1_ENUMERATED, "\x01"}, + + // Test negative values. These are interesting because, when encoding, the + // ASN.1 type must be determined from the string type, but the string type + // has an extra |V_ASN1_NEG| bit. + {CBS_ASN1_ENUMERATED, "\xff", V_ASN1_NEG_ENUMERATED, "\x01"}, + + // SEQUENCE is supported but, currently, SET is not. Note the + // |ASN1_STRING| representation will include the tag and length. + {CBS_ASN1_SEQUENCE, "", V_ASN1_SEQUENCE, std::string("\x30\x00", 2)}, + + // These types are not actually supported by the library but, + // historically, we would parse them, and not other unsupported types, due + // to quirks of |ASN1_tag2bit|. + {7, "", V_ASN1_OBJECT_DESCRIPTOR, ""}, + {8, "", V_ASN1_EXTERNAL, ""}, + {9, "", V_ASN1_REAL, ""}, + {11, "", 11 /* EMBEDDED PDV */, ""}, + {13, "", 13 /* RELATIVE-OID */, ""}, + {14, "", 14 /* TIME */, ""}, + {15, "", 15 /* not a type; reserved value */, ""}, + {29, "", 29 /* CHARACTER STRING */, ""}, + + // TODO(crbug.com/boringssl/412): Attribute values are an ANY DEFINED BY + // type, so we actually shoudl be accepting all ASN.1 types. We currently + // do not and only accept the above types. Extend this test when we fix + // this. + }; + for (const auto &t : kTests) { + SCOPED_TRACE(t.der_tag); + SCOPED_TRACE(Bytes(t.der_contents)); + + // Construct an X.509 name containing a single RDN with a single attribute: + // kOID with the specified value. + auto encoded = encode_single_attribute_name(t.der_tag, t.der_contents); + ASSERT_FALSE(encoded.empty()); + SCOPED_TRACE(Bytes(encoded)); + + // The input should parse. + const uint8_t *inp = encoded.data(); + bssl::UniquePtr name( + d2i_X509_NAME(nullptr, &inp, encoded.size())); + ASSERT_TRUE(name); + EXPECT_EQ(inp, encoded.data() + encoded.size()) + << "input was not fully consumed"; + + // Check there is a single attribute with the expected in-memory + // representation. + ASSERT_EQ(1, X509_NAME_entry_count(name.get())); + const X509_NAME_ENTRY *entry = X509_NAME_get_entry(name.get(), 0); + const ASN1_OBJECT *obj = X509_NAME_ENTRY_get_object(entry); + EXPECT_EQ(Bytes(OBJ_get0_data(obj), OBJ_length(obj)), Bytes(kOID)); + const ASN1_STRING *value = X509_NAME_ENTRY_get_data(entry); + EXPECT_EQ(ASN1_STRING_type(value), t.str_type); + EXPECT_EQ(Bytes(ASN1_STRING_get0_data(value), ASN1_STRING_length(value)), + Bytes(t.str_contents)); + + // The name should re-encode with the same input. + uint8_t *der = nullptr; + int der_len = i2d_X509_NAME(name.get(), &der); + ASSERT_GE(der_len, 0); + bssl::UniquePtr free_der(der); + EXPECT_EQ(Bytes(der, der_len), Bytes(encoded)); + + // X509_NAME internally caches its encoding, which means the check above + // does not fully test re-encoding. Repeat the test by constructing an + // |X509_NAME| from the string representation. + name.reset(X509_NAME_new()); + ASSERT_TRUE(name); + ASSERT_TRUE(X509_NAME_add_entry_by_txt( + name.get(), kOIDText, t.str_type, + reinterpret_cast(t.str_contents.data()), + t.str_contents.size(), /*loc=*/-1, /*set=*/0)); + + // The name should re-encode with the same input. + der = nullptr; + der_len = i2d_X509_NAME(name.get(), &der); + ASSERT_GE(der_len, 0); + free_der.reset(der); + EXPECT_EQ(Bytes(der, der_len), Bytes(encoded)); + } + + const struct { + CBS_ASN1_TAG der_tag; + std::string der_contents; + } kInvalidTests[] = { + // Errors in supported universal types should be handled. + {CBS_ASN1_NULL, "not null"}, + {CBS_ASN1_BOOLEAN, "not bool"}, + {CBS_ASN1_OBJECT, ""}, + {CBS_ASN1_INTEGER, std::string("\0\0", 2)}, + {CBS_ASN1_ENUMERATED, std::string("\0\0", 2)}, + {CBS_ASN1_BITSTRING, ""}, + {CBS_ASN1_UTF8STRING, "not utf-8 \xff"}, + {CBS_ASN1_BMPSTRING, "not utf-16 "}, + {CBS_ASN1_UNIVERSALSTRING, "not utf-32"}, + {CBS_ASN1_UTCTIME, "not utctime"}, + {CBS_ASN1_GENERALIZEDTIME, "not generalizedtime"}, + {CBS_ASN1_UTF8STRING | CBS_ASN1_CONSTRUCTED, ""}, + {CBS_ASN1_SEQUENCE & ~CBS_ASN1_CONSTRUCTED, ""}, + + // TODO(crbug.com/boringssl/412): The following inputs should parse, but + // are currently rejected because they cannot be represented in + // |ASN1_PRINTABLE|, either because they don't fit in |ASN1_STRING| or + // simply in the |B_ASN1_PRINTABLE| bitmask. + {CBS_ASN1_NULL, ""}, + {CBS_ASN1_BOOLEAN, std::string("\x00", 1)}, + {CBS_ASN1_BOOLEAN, "\xff"}, + {CBS_ASN1_OBJECT, "\x01\x02\x03\x04"}, + {CBS_ASN1_INTEGER, "\x01"}, + {CBS_ASN1_INTEGER, "\xff"}, + {CBS_ASN1_OCTETSTRING, ""}, + {CBS_ASN1_UTCTIME, "700101000000Z"}, + {CBS_ASN1_GENERALIZEDTIME, "19700101000000Z"}, + {CBS_ASN1_SET, ""}, + {CBS_ASN1_APPLICATION | CBS_ASN1_CONSTRUCTED | 42, ""}, + {CBS_ASN1_APPLICATION | 42, ""}, + }; + for (const auto &t : kInvalidTests) { + SCOPED_TRACE(t.der_tag); + SCOPED_TRACE(Bytes(t.der_contents)); + + // Construct an X.509 name containing a single RDN with a single attribute: + // kOID with the specified value. + auto encoded = encode_single_attribute_name(t.der_tag, t.der_contents); + ASSERT_FALSE(encoded.empty()); + SCOPED_TRACE(Bytes(encoded)); + + // The input should not parse. + const uint8_t *inp = encoded.data(); + bssl::UniquePtr name( + d2i_X509_NAME(nullptr, &inp, encoded.size())); + EXPECT_FALSE(name); + } +} + +TEST(X509Test, GetTextByOBJ) { + struct OBJTestCase { + const char *content; + int content_type; + int len; + int expected_result; + const char *expected_string; + } kTests[] = { + {"", V_ASN1_UTF8STRING, 0, 0, ""}, + {"derp", V_ASN1_UTF8STRING, 4, 4, "derp"}, + {"\x30\x00", // Empty sequence can not be converted to UTF-8 + V_ASN1_SEQUENCE, 2, -1, ""}, + { + "der\0p", + V_ASN1_TELETEXSTRING, + 5, + -1, + "", + }, + { + "0123456789ABCDEF", + V_ASN1_IA5STRING, + 16, + 16, + "0123456789ABCDEF", + }, + { + "\x07\xff", + V_ASN1_BMPSTRING, + 2, + 2, + "\xdf\xbf", + }, + { + "\x00\xc3\x00\xaf", + V_ASN1_BMPSTRING, + 4, + 4, + "\xc3\x83\xc2\xaf", + }, + }; + for (const auto &test : kTests) { + bssl::UniquePtr name(X509_NAME_new()); + ASSERT_TRUE(name); + ASSERT_TRUE(X509_NAME_add_entry_by_NID( + name.get(), NID_commonName, test.content_type, + reinterpret_cast(test.content), test.len, /*loc=*/-1, + /*set=*/0)); + char text[256] = {}; + EXPECT_EQ(test.expected_result, + X509_NAME_get_text_by_NID(name.get(), NID_commonName, text, + sizeof(text))); + EXPECT_STREQ(text, test.expected_string); + if (test.expected_result > 0) { + // Test truncation. The function writes a trailing NUL byte so the + // buffer needs to be one bigger than the expected result. + char small[2] = "a"; + EXPECT_EQ( + -1, X509_NAME_get_text_by_NID(name.get(), NID_commonName, small, 1)); + // The buffer should be unmodified by truncation failure. + EXPECT_STREQ(small, "a"); + } + } +} diff --git a/crypto/x509/x509name.c b/crypto/x509/x509name.c index eec2c8e02a..8d2d202f88 100644 --- a/crypto/x509/x509name.c +++ b/crypto/x509/x509name.c @@ -57,6 +57,7 @@ #include #include +#include #include #include #include @@ -86,13 +87,34 @@ int X509_NAME_get_text_by_OBJ(const X509_NAME *name, const ASN1_OBJECT *obj, } const ASN1_STRING *data = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i)); - i = (data->length > (len - 1)) ? (len - 1) : data->length; - if (buf == NULL) { - return data->length; + unsigned char *text = NULL; + int ret = -1; + int text_len = ASN1_STRING_to_UTF8(&text, data); + // Fail if we could not encode as UTF-8. + if (text_len < 0) { + goto out; + } + CBS cbs; + CBS_init(&cbs, text, text_len); + // Fail if the UTF-8 encoding constains a 0 byte because this is + // returned as a C string and callers very often do not check. + if (CBS_contains_zero_byte(&cbs)) { + goto out; + } + // We still support the "pass NULL to find out how much" API + if (buf != NULL) { + if (text_len >= len || len <= 0 || + !CBS_copy_bytes(&cbs, (uint8_t *)buf, text_len)) { + goto out; + } + // It must be a C string + buf[text_len] = '\0'; } - OPENSSL_memcpy(buf, data->data, i); - buf[i] = '\0'; - return i; + ret = text_len; + +out: + OPENSSL_free(text); + return ret; } int X509_NAME_entry_count(const X509_NAME *name) { diff --git a/crypto/x509/x_name.c b/crypto/x509/x_name.c index 96eed9c713..2fe59d884e 100644 --- a/crypto/x509/x_name.c +++ b/crypto/x509/x_name.c @@ -123,7 +123,6 @@ static const ASN1_EXTERN_FUNCS x509_name_ff = { NULL, x509_name_ex_new, x509_name_ex_free, - 0, // Default clear behaviour is OK x509_name_ex_d2i, x509_name_ex_i2d, NULL, diff --git a/include/openssl/asn1t.h b/include/openssl/asn1t.h index 72be67278e..89046fbe5f 100644 --- a/include/openssl/asn1t.h +++ b/include/openssl/asn1t.h @@ -534,7 +534,6 @@ typedef struct ASN1_EXTERN_FUNCS_st { void *app_data; ASN1_ex_new_func *asn1_ex_new; ASN1_ex_free_func *asn1_ex_free; - ASN1_ex_free_func *asn1_ex_clear; ASN1_ex_d2i *asn1_ex_d2i; ASN1_ex_i2d *asn1_ex_i2d; /* asn1_ex_print is unused. */ diff --git a/include/openssl/evp.h b/include/openssl/evp.h index f6b4da0b10..6d787b1f4f 100644 --- a/include/openssl/evp.h +++ b/include/openssl/evp.h @@ -59,7 +59,7 @@ #include -#include +#include // IWYU pragma: export #include // OpenSSL included digest, cipher, and object functions in this header so we diff --git a/include/openssl/obj.h b/include/openssl/obj.h index b98760414a..b9704b868b 100644 --- a/include/openssl/obj.h +++ b/include/openssl/obj.h @@ -60,7 +60,7 @@ #include #include -#include +#include // IWYU pragma: export #if defined(__cplusplus) extern "C" { @@ -148,6 +148,10 @@ OPENSSL_EXPORT int OBJ_txt2nid(const char *s); // a non-const pointer and manage ownership. OPENSSL_EXPORT ASN1_OBJECT *OBJ_nid2obj(int nid); +// OBJ_get_undef returns the object for |NID_undef|. Prefer this function over +// |OBJ_nid2obj| to avoid pulling in the full OID table. +OPENSSL_EXPORT const ASN1_OBJECT *OBJ_get_undef(void); + // OBJ_nid2sn returns the short name for |nid|, or NULL if |nid| is unknown. OPENSSL_EXPORT const char *OBJ_nid2sn(int nid); diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 5068f0aef7..6e09f02a1b 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2105,20 +2105,22 @@ OPENSSL_EXPORT ASN1_TIME *X509_CRL_get_nextUpdate(X509_CRL *crl); OPENSSL_EXPORT ASN1_INTEGER *X509_get_serialNumber(X509 *x509); // X509_NAME_get_text_by_OBJ finds the first attribute with type |obj| in -// |name|. If found, it ignores the value's ASN.1 type, writes the raw -// |ASN1_STRING| representation to |buf|, followed by a NUL byte, and -// returns the number of bytes in output, excluding the NUL byte. -// -// This function writes at most |len| bytes, including the NUL byte. If |len| is -// not large enough, it silently truncates the output to fit. If |buf| is NULL, -// it instead writes enough and returns the number of bytes in the output, -// excluding the NUL byte. -// -// WARNING: Do not use this function. It does not return enough information for -// the caller to correctly interpret its output. The attribute value may be of -// any type, including one of several ASN.1 string encodings, but this function -// only outputs the raw |ASN1_STRING| representation. See -// https://crbug.com/boringssl/436. +// |name|. If found, it writes the value's UTF-8 representation to |buf|. +// followed by a NUL byte, and returns the number of bytes in the output, +// excluding the NUL byte. This is unlike OpenSSL which returns the raw +// ASN1_STRING data. The UTF-8 encoding of the |ASN1_STRING| may not contain a 0 +// codepoint. +// +// This function writes at most |len| bytes, including the NUL byte. If |buf| +// is NULL, it writes nothing and returns the number of bytes in the +// output, excluding the NUL byte that would be required for the full UTF-8 +// output. +// +// This function may return -1 if an error occurs for any reason, including the +// value not being a recognized string type, |len| being of insufficient size to +// hold the full UTF-8 encoding and NUL byte, memory allocation failures, an +// object with type |obj| not existing in |name|, or if the UTF-8 encoding of +// the string contains a zero byte. OPENSSL_EXPORT int X509_NAME_get_text_by_OBJ(const X509_NAME *name, const ASN1_OBJECT *obj, char *buf, int len);