From 183d296770a6d7ad7f7eade3db18b8326844ee0f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 1 Oct 2023 23:53:03 -0400 Subject: [PATCH 1/8] Remove asn1_ex_clear from ASN1_EXTERN_FUNCS. This is never defined. Change-Id: I1ecaa00f780d6b2f000dc67514c2f49eb4cf2a45 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63528 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit 5e1496bf00a1bd740f5626aa4897b105a3561254) --- crypto/asn1/tasn_new.c | 9 +-------- crypto/x509/x_name.c | 1 - 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/crypto/asn1/tasn_new.c b/crypto/asn1/tasn_new.c index 143c47b924..870490ca56 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: 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, From 7899255ccc9ec2aef9aa5fa7ccfe2215299bd434 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 1 Oct 2023 17:48:20 -0400 Subject: [PATCH 2/8] Don't include NID_undef in short/long name tables NID_undef actually has names, but OBJ_sn2nid and OBJ_ln2nid's calling convention cannot distinguish finding NID_undef from finding nothing. Thus we may as well save 4 bytes by omitting this. Change-Id: I6102e67141a2f5524aacf0ea84e6a2b2d2add534 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63529 Commit-Queue: David Benjamin Reviewed-by: Bob Beck (cherry picked from commit 8a062a71124f84ed4c3ab304b75ee215b7439de3) --- crypto/obj/obj_dat.h | 2 -- crypto/obj/obj_test.cc | 3 +++ crypto/obj/objects.go | 12 ++++++++++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/crypto/obj/obj_dat.h b/crypto/obj/obj_dat.h index 0da1e911d1..8d90d5246f 100644 --- a/crypto/obj/obj_dat.h +++ b/crypto/obj/obj_dat.h @@ -9101,7 +9101,6 @@ static const uint16_t kNIDsInShortNameOrder[] = { 143 /* SXNetID */, 981 /* SecP256r1Kyber768Draft00 */, 458 /* UID */, - 0 /* UNDEF */, 948 /* X25519 */, 982 /* X25519Kyber768Draft00 */, 961 /* X448 */, @@ -10815,7 +10814,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..025e1f0a55 100644 --- a/crypto/obj/obj_test.cc +++ b/crypto/obj/obj_test.cc @@ -56,6 +56,9 @@ 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")); } TEST(ObjTest, TestSignatureAlgorithms) { diff --git a/crypto/obj/objects.go b/crypto/obj/objects.go index a4d8395aa2..a6b406aee4 100644 --- a/crypto/obj/objects.go +++ b/crypto/obj/objects.go @@ -640,7 +640,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 +660,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") From 58e191e2662f00430e1165178c9eddc082f23018 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 1 Oct 2023 18:01:31 -0400 Subject: [PATCH 3/8] Store NID_undef's ASN1_OBJECT outside the table tasn_*.c have two dependencies on the OID table: initializing ASN1_OBJECTs to the undef object, and the ADB (ANY DEFINED BY) machinery. Fix the first by pulling the entry out of the table. The latter will be fixed by rewriting the certificate policy parser. Bug: 551 Change-Id: I7c423ff9ce78b850555203a31c2d220d92d04f35 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63530 Commit-Queue: David Benjamin Reviewed-by: Bob Beck (cherry picked from commit 26d84fdf024cf85e461aa10b59f9484699167533) --- crypto/asn1/tasn_new.c | 2 +- crypto/obj/obj.c | 42 +++++++++++++++++++++++++++++++--------- crypto/obj/obj_dat.h | 1 - crypto/obj/obj_test.cc | 1 + crypto/obj/objects.go | 6 ++++++ crypto/x509/x509_test.cc | 2 +- include/openssl/obj.h | 4 ++++ 7 files changed, 46 insertions(+), 12 deletions(-) diff --git a/crypto/asn1/tasn_new.c b/crypto/asn1/tasn_new.c index 870490ca56..7d10319db4 100644 --- a/crypto/asn1/tasn_new.c +++ b/crypto/asn1/tasn_new.c @@ -278,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/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 8d90d5246f..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}, diff --git a/crypto/obj/obj_test.cc b/crypto/obj/obj_test.cc index 025e1f0a55..abea30d7e9 100644 --- a/crypto/obj/obj_test.cc +++ b/crypto/obj/obj_test.cc @@ -59,6 +59,7 @@ TEST(ObjTest, TestBasic) { 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 a6b406aee4..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 diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 910fc63022..4d39f39d10 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)); } diff --git a/include/openssl/obj.h b/include/openssl/obj.h index b98760414a..408de196bc 100644 --- a/include/openssl/obj.h +++ b/include/openssl/obj.h @@ -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); From f3e5ebc9a2cf8eb0598866f11aff6f7365d5d182 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 1 Oct 2023 17:41:05 -0400 Subject: [PATCH 4/8] Improve X509Test.NameAttributeValues coverage Add a negative ENUMERATED. This is redundant with ASN1Test.NegativeEnumeratedMultistring, but once we fix the X509_NAME value representation, d2i_ASN1_PRINTABLE will be gone. In doing so, I noticed that we weren't really testing re-encoding, so fix that. Also add some negative tests, both capturing actual invalid values, and values which should be valid but aren't due to issue #412. Bug: 412 Change-Id: Iba7f2869607e6361d6cb913416de21a19cdd6275 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63527 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit a014bdad2ad475ab3cfe5fb2bd20183b1a72ecd4) --- crypto/x509/x509_test.cc | 187 +++++++++++++++++++++++++++++++++++++++ include/openssl/asn1t.h | 1 - 2 files changed, 187 insertions(+), 1 deletion(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 4d39f39d10..f621ce565b 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -6744,3 +6744,190 @@ 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); + } +} 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. */ From 637afcda8f495ade222e29e5e2e79ff770bf0211 Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Sat, 30 Sep 2023 12:55:31 -0700 Subject: [PATCH 5/8] crypto: remove kBoringSSLBinaryTag This was never updated and not in use. Remove this to simplify the code and avoid an issue on ARM64 Windows. ~~~ D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\boringssl\crypto\mem.c(152): error C2220: the following warning is treated as an error D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\boringssl\crypto\mem.c(152): warning C4746: volatile access of 'kBoringSSLBinaryTag' is subject to /volatile: setting; consider using __iso_volatile_load/store intrinsic functions ~~~ Change-Id: Iedb0999e38c769add5bb1d5623e038b17f8f245f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63325 Reviewed-by: David Benjamin Reviewed-by: Adam Langley Commit-Queue: Adam Langley (cherry picked from commit 5d58c559ace6a24ea6613e412b26bd4c50668ab3) --- crypto/mem.c | 33 --------------------------------- 1 file changed, 33 deletions(-) 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; } From 7f2e21ba685b41fda1c384378ada2769a3929381 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 9 Oct 2023 13:55:56 -0400 Subject: [PATCH 6/8] Add some IWYU export pragmas We split nid.h out of obj.h and evp_errors.h out of evp.h, but folks who include the original headers can reasonably assume that the child header is included. Change-Id: I81c40fd88df58500a0f10bfa864b8bc98451dbc0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63485 Auto-Submit: David Benjamin Reviewed-by: Adam Langley Commit-Queue: David Benjamin (cherry picked from commit 9e6144382ca4752591910b38b71a3301d97999df) --- include/openssl/evp.h | 2 +- include/openssl/obj.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 408de196bc..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" { From 2ed11afe625fcc860cab5ff59b99d3af24b7cf01 Mon Sep 17 00:00:00 2001 From: Theo Buehler Date: Thu, 19 Oct 2023 06:50:23 +0200 Subject: [PATCH 7/8] Error check X509_ALGOR_set0() If |param_type| is different from |V_ASN1_UNDEF|, there will usually be a call to |ASN1_TYPE_new| which allocates and can thus fail. The result of a failure is that |pval| will leak, which is the case in both callers in the RSA-PSS code. This changeset leaves out the call in |X509_ALGOR_set_md|, which is a void function. This could be fixed in three ways: change its signature to allow error checking, call |X509_ALGOR_set0| up front to preallocate, or inline the function in its only internal caller and remove it from the public API. Change-Id: I25ed3593947f9ee58208b980a95730d37789c9e1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63585 Reviewed-by: David Benjamin Commit-Queue: David Benjamin (cherry picked from commit 39d7ee9c8262d9cd3338735bf3e95649857375e5) --- crypto/x509/algorithm.c | 3 +-- crypto/x509/rsa_pss.c | 8 ++++++-- 2 files changed, 7 insertions(+), 4 deletions(-) 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; From db36c3b357c1b8976c558ac59b228e1332f6c248 Mon Sep 17 00:00:00 2001 From: Bob Beck Date: Wed, 29 Mar 2023 11:17:46 -0600 Subject: [PATCH 8/8] Convert X509_NAME_get_text_by_[NID|OBJ] to return UTF-8 Callers to these functions are usually using them to grab subject name components and universally use the result as a C string, whereas the OpenSSL versions return raw ASN1_STRING bytes and ignore the encoding, which "usually works" in a "hold my beer here are some bytes" sort of way until the object is not encoded as you hoped. Make this safer for the callers by making the returned result be at least "text" and a C string. This converts the ASN1_STRING bytes to UTF-8, and will introduce new failure cases for this function if either memory allocation fails for the UTF-8 conversion, or if the resulting UTF-8 contains a 0 codepoint and would produce an artificially truncated C string. Additionally if the provided buffer is not NULL but is too small to hold the output, we fail rather than returning a truncated output. Fixed: 436 Change-Id: I487c10a5ff5188e94df520ef4c8982e593c680d7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58445 Reviewed-by: David Benjamin Commit-Queue: Bob Beck (cherry picked from commit 3763efb56b5282cf92d71c259576352555c1a8f8) --- crypto/x509/x509_test.cc | 65 ++++++++++++++++++++++++++++++++++++++++ crypto/x509/x509name.c | 34 +++++++++++++++++---- include/openssl/x509.h | 30 ++++++++++--------- 3 files changed, 109 insertions(+), 20 deletions(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index f621ce565b..ae2385c39e 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -6931,3 +6931,68 @@ TEST(X509Test, NameAttributeValues) { 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/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);