From 5e1496bf00a1bd740f5626aa4897b105a3561254 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 1 Oct 2023 23:53:03 -0400 Subject: [PATCH 1/3] 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 --- crypto/asn1/internal.h | 1 - crypto/asn1/tasn_new.c | 9 +-------- crypto/x509/x_name.c | 1 - crypto/x509/x_x509.c | 1 - 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h index 5dca7280c..414b5a97d 100644 --- a/crypto/asn1/internal.h +++ b/crypto/asn1/internal.h @@ -256,7 +256,6 @@ typedef void ASN1_ex_free_func(ASN1_VALUE **pval, const ASN1_ITEM *it); typedef struct ASN1_EXTERN_FUNCS_st { 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_EXTERN_FUNCS; diff --git a/crypto/asn1/tasn_new.c b/crypto/asn1/tasn_new.c index e896ead14..cc30bc8a0 100644 --- a/crypto/asn1/tasn_new.c +++ b/crypto/asn1/tasn_new.c @@ -183,16 +183,9 @@ auxerr: } 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 3063ce7b4..0bca6399e 100644 --- a/crypto/x509/x_name.c +++ b/crypto/x509/x_name.c @@ -122,7 +122,6 @@ ASN1_ITEM_TEMPLATE_END(X509_NAME_INTERNAL) static const ASN1_EXTERN_FUNCS x509_name_ff = { x509_name_ex_new, x509_name_ex_free, - 0, // Default clear behaviour is OK x509_name_ex_d2i, x509_name_ex_i2d, }; diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c index 37a11c6d9..2d4d5552e 100644 --- a/crypto/x509/x_x509.c +++ b/crypto/x509/x_x509.c @@ -342,7 +342,6 @@ static int x509_i2d_cb(ASN1_VALUE **pval, unsigned char **out, static const ASN1_EXTERN_FUNCS x509_extern_funcs = { x509_new_cb, x509_free_cb, - /*asn1_ex_clear=*/NULL, x509_d2i_cb, x509_i2d_cb, }; From 8a062a71124f84ed4c3ab304b75ee215b7439de3 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 1 Oct 2023 17:48:20 -0400 Subject: [PATCH 2/3] 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 --- 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 654b3c08e..4a3bb885a 100644 --- a/crypto/obj/obj_dat.h +++ b/crypto/obj/obj_dat.h @@ -8980,7 +8980,6 @@ static const uint16_t kNIDsInShortNameOrder[] = { 16 /* ST */, 143 /* SXNetID */, 458 /* UID */, - 0 /* UNDEF */, 948 /* X25519 */, 964 /* X25519Kyber768Draft00 */, 961 /* X448 */, @@ -10670,7 +10669,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 08796e2b9..025e1f0a5 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 077a6e12f..a0f35cb79 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 26d84fdf024cf85e461aa10b59f9484699167533 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 1 Oct 2023 18:01:31 -0400 Subject: [PATCH 3/3] 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 --- 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 cc30bc8a0..76c52c3e4 100644 --- a/crypto/asn1/tasn_new.c +++ b/crypto/asn1/tasn_new.c @@ -267,7 +267,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 651993365..41064247a 100644 --- a/crypto/obj/obj.c +++ b/crypto/obj/obj.c @@ -179,12 +179,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) { @@ -215,7 +222,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) { @@ -238,7 +245,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) { @@ -263,7 +270,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 @@ -273,7 +280,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) { @@ -297,7 +304,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) { @@ -324,12 +331,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_MUTEX_lock_read(&global_added_lock); diff --git a/crypto/obj/obj_dat.h b/crypto/obj/obj_dat.h index 4a3bb885a..71ef2d2bd 100644 --- a/crypto/obj/obj_dat.h +++ b/crypto/obj/obj_dat.h @@ -7140,7 +7140,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 025e1f0a5..abea30d7e 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 a0f35cb79..f938e126d 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 68d75d591..5a7556299 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -6456,7 +6456,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 3fb8bdeb9..51c5b30c0 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);