Make ASN1_OBJECT opaque. This cleans up the story with https://boringssl-review.googlesource.com/c/boringssl/+/46164. None of our exported functions mutate ASN1_OBJECTS, with the exception of ASN1_OBJECT_free, the object reuse mode of c2i_ASN1_OBJECT, and their callers. Those functions check flags to correctly handle static ASN1_OBJECTs. For now, I've kept the struct definition in crypto/asn1 even though ASN1_OBJECT is partially in crypto/obj. Since we prefer to cut dependencies to crypto/asn1, we probably should rearrange this later. I've also, for now, kept crypto/asn1/internal.h at C-style comments, though our style story here is weird. (Maybe it's time to clang-format crypto/asn1 and crypto/x509? Patches from upstream rarely directly apply anyway, since we're a mix of 1.0.2 and 1.1.1 in crypto/x509.) Update-Note: ASN1_OBJECT is now opaque. Callers should use accessors. Change-Id: I655e6bd8afda98a2d1e676c3abeb873aa8de6691 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48326 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/a_object.c b/crypto/asn1/a_object.c index 665e0d6..de27e87 100644 --- a/crypto/asn1/a_object.c +++ b/crypto/asn1/a_object.c
@@ -64,6 +64,7 @@ #include <openssl/obj.h> #include "../internal.h" +#include "internal.h" int i2d_ASN1_OBJECT(const ASN1_OBJECT *a, unsigned char **pp)
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h index a2dbf76..4e42c70 100644 --- a/crypto/asn1/internal.h +++ b/crypto/asn1/internal.h
@@ -86,6 +86,26 @@ /* Internal ASN1 structures and functions: not for application use */ +/* These are used internally in the ASN1_OBJECT to keep track of + * whether the names and data need to be free()ed */ +#define ASN1_OBJECT_FLAG_DYNAMIC 0x01 /* internal use */ +#define ASN1_OBJECT_FLAG_DYNAMIC_STRINGS 0x04 /* internal use */ +#define ASN1_OBJECT_FLAG_DYNAMIC_DATA 0x08 /* internal use */ + +/* An asn1_object_st (aka |ASN1_OBJECT|) represents an ASN.1 OBJECT IDENTIFIER. + * Note: Mutating an |ASN1_OBJECT| is only permitted when initializing it. The + * library maintains a table of static |ASN1_OBJECT|s, which may be referenced + * by non-const |ASN1_OBJECT| pointers. Code which receives an |ASN1_OBJECT| + * pointer externally must assume it is immutable, even if the pointer is not + * const. */ +struct asn1_object_st { + const char *sn, *ln; + int nid; + int length; + const unsigned char *data; /* data remains const after init */ + int flags; /* Should we free this one */ +}; + int asn1_utctime_to_tm(struct tm *tm, const ASN1_UTCTIME *d); int asn1_generalizedtime_to_tm(struct tm *tm, const ASN1_GENERALIZEDTIME *d);
diff --git a/crypto/digest_extra/digest_extra.c b/crypto/digest_extra/digest_extra.c index 311c5cb..a93601c 100644 --- a/crypto/digest_extra/digest_extra.c +++ b/crypto/digest_extra/digest_extra.c
@@ -58,11 +58,12 @@ #include <string.h> -#include <openssl/asn1.h> #include <openssl/blake2.h> #include <openssl/bytestring.h> +#include <openssl/obj.h> #include <openssl/nid.h> +#include "../asn1/internal.h" #include "../internal.h" #include "../fipsmodule/digest/internal.h" @@ -152,13 +153,14 @@ } const EVP_MD *EVP_get_digestbyobj(const ASN1_OBJECT *obj) { - // Handle objects with no corresponding OID. + // Handle objects with no corresponding OID. Note we don't use |OBJ_obj2nid| + // here to avoid pulling in the OID table. if (obj->nid != NID_undef) { return EVP_get_digestbynid(obj->nid); } CBS cbs; - CBS_init(&cbs, obj->data, obj->length); + CBS_init(&cbs, OBJ_get0_data(obj), OBJ_length(obj)); return cbs_to_md(&cbs); }
diff --git a/crypto/obj/obj.c b/crypto/obj/obj.c index 6310785..958625d 100644 --- a/crypto/obj/obj.c +++ b/crypto/obj/obj.c
@@ -67,10 +67,13 @@ #include <openssl/mem.h> #include <openssl/thread.h> -#include "obj_dat.h" +#include "../asn1/internal.h" #include "../internal.h" #include "../lhash/internal.h" +// obj_data.h must be included after the definition of |ASN1_OBJECT|. +#include "obj_dat.h" + DEFINE_LHASH_OF(ASN1_OBJECT)
diff --git a/crypto/obj/obj_test.cc b/crypto/obj/obj_test.cc index e623843..08796e2 100644 --- a/crypto/obj/obj_test.cc +++ b/crypto/obj/obj_test.cc
@@ -75,14 +75,16 @@ static bool ExpectObj2Txt(const uint8_t *der, size_t der_len, bool always_return_oid, const char *expected) { - ASN1_OBJECT obj; - OPENSSL_memset(&obj, 0, sizeof(obj)); - obj.data = der; - obj.length = static_cast<int>(der_len); + bssl::UniquePtr<ASN1_OBJECT> obj( + ASN1_OBJECT_create(NID_undef, der, static_cast<int>(der_len), + /*sn=*/nullptr, /*ln=*/nullptr)); + if (!obj) { + return false; + } int expected_len = static_cast<int>(strlen(expected)); - int len = OBJ_obj2txt(nullptr, 0, &obj, always_return_oid); + int len = OBJ_obj2txt(nullptr, 0, obj.get(), always_return_oid); if (len != expected_len) { fprintf(stderr, "OBJ_obj2txt of %s with out_len = 0 returned %d, wanted %d.\n", @@ -92,7 +94,7 @@ char short_buf[1]; OPENSSL_memset(short_buf, 0xff, sizeof(short_buf)); - len = OBJ_obj2txt(short_buf, sizeof(short_buf), &obj, always_return_oid); + len = OBJ_obj2txt(short_buf, sizeof(short_buf), obj.get(), always_return_oid); if (len != expected_len) { fprintf(stderr, "OBJ_obj2txt of %s with out_len = 1 returned %d, wanted %d.\n", @@ -109,7 +111,7 @@ } char buf[256]; - len = OBJ_obj2txt(buf, sizeof(buf), &obj, always_return_oid); + len = OBJ_obj2txt(buf, sizeof(buf), obj.get(), always_return_oid); if (len != expected_len) { fprintf(stderr, "OBJ_obj2txt of %s with out_len = 256 returned %d, wanted %d.\n", @@ -166,17 +168,14 @@ ASSERT_TRUE(ExpectObj2Txt(nullptr, 0, false /* return name */, "")); ASSERT_TRUE(ExpectObj2Txt(nullptr, 0, true /* don't return name */, "")); - ASN1_OBJECT obj; - OPENSSL_memset(&obj, 0, sizeof(obj)); - // kNonMinimalOID is kBasicConstraints with the final component non-minimally // encoded. - static const uint8_t kNonMinimalOID[] = { - 0x55, 0x1d, 0x80, 0x13, - }; - obj.data = kNonMinimalOID; - obj.length = sizeof(kNonMinimalOID); - ASSERT_EQ(-1, OBJ_obj2txt(NULL, 0, &obj, 0)); + static const uint8_t kNonMinimalOID[] = {0x55, 0x1d, 0x80, 0x13}; + bssl::UniquePtr<ASN1_OBJECT> obj( + ASN1_OBJECT_create(NID_undef, kNonMinimalOID, sizeof(kNonMinimalOID), + /*sn=*/nullptr, /*ln=*/nullptr)); + ASSERT_TRUE(obj); + ASSERT_EQ(-1, OBJ_obj2txt(NULL, 0, obj.get(), 0)); // kOverflowOID is the DER representation of // 1.2.840.113554.4.1.72585.18446744073709551616. (The final value is 2^64.) @@ -184,16 +183,16 @@ 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7, 0x09, 0x82, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x00, }; - obj.data = kOverflowOID; - obj.length = sizeof(kOverflowOID); - ASSERT_EQ(-1, OBJ_obj2txt(NULL, 0, &obj, 0)); + obj.reset(ASN1_OBJECT_create(NID_undef, kOverflowOID, sizeof(kOverflowOID), + /*sn=*/nullptr, /*ln=*/nullptr)); + ASSERT_TRUE(obj); + ASSERT_EQ(-1, OBJ_obj2txt(NULL, 0, obj.get(), 0)); // kInvalidOID is a mis-encoded version of kBasicConstraints with the final // octet having the high bit set. - static const uint8_t kInvalidOID[] = { - 0x55, 0x1d, 0x93, - }; - obj.data = kInvalidOID; - obj.length = sizeof(kInvalidOID); - ASSERT_EQ(-1, OBJ_obj2txt(NULL, 0, &obj, 0)); + static const uint8_t kInvalidOID[] = {0x55, 0x1d, 0x93}; + obj.reset(ASN1_OBJECT_create(NID_undef, kInvalidOID, sizeof(kInvalidOID), + /*sn=*/nullptr, /*ln=*/nullptr)); + ASSERT_TRUE(obj); + ASSERT_EQ(-1, OBJ_obj2txt(NULL, 0, obj.get(), 0)); }
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index 06eeab0..db467fd 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h
@@ -593,29 +593,6 @@ #define MBSTRING_BMP (MBSTRING_FLAG | 2) #define MBSTRING_UNIV (MBSTRING_FLAG | 4) -// These are used internally in the ASN1_OBJECT to keep track of -// whether the names and data need to be free()ed -#define ASN1_OBJECT_FLAG_DYNAMIC 0x01 // internal use -#define ASN1_OBJECT_FLAG_DYNAMIC_STRINGS 0x04 // internal use -#define ASN1_OBJECT_FLAG_DYNAMIC_DATA 0x08 // internal use - -// An asn1_object_st (aka |ASN1_OBJECT|) represents an ASN.1 OBJECT IDENTIFIER. -// -// Note: Although the struct is exposed, mutating an |ASN1_OBJECT| is only -// permitted when initializing it. The library maintains a table of static -// |ASN1_OBJECT|s, which may be referenced by non-const |ASN1_OBJECT| pointers. -// Code which receives an |ASN1_OBJECT| pointer externally must assume it is -// immutable, even if the pointer is not const. -// -// TODO(davidben): Document this more completely in its own section. -struct asn1_object_st { - const char *sn, *ln; - int nid; - int length; - const unsigned char *data; // data remains const after init - int flags; // Should we free this one -}; - DEFINE_STACK_OF(ASN1_OBJECT) // ASN1_ENCODING structure: this is used to save the received
diff --git a/include/openssl/obj.h b/include/openssl/obj.h index 187586d..ad7271e 100644 --- a/include/openssl/obj.h +++ b/include/openssl/obj.h
@@ -84,7 +84,8 @@ // Basic operations. -// OBJ_dup returns a duplicate copy of |obj| or NULL on allocation failure. +// OBJ_dup returns a duplicate copy of |obj| or NULL on allocation failure. The +// caller must call |ASN1_OBJECT_free| on the result to release it. OPENSSL_EXPORT ASN1_OBJECT *OBJ_dup(const ASN1_OBJECT *obj); // OBJ_cmp returns a value less than, equal to or greater than zero if |a| is @@ -133,9 +134,9 @@ // OBJ_nid2obj returns the |ASN1_OBJECT| corresponding to |nid|, or NULL if // |nid| is unknown. // -// This function returns a static, immutable |ASN1_OBJECT|. Although the output -// is not const, callers may not mutate it. It is also not necessary to release -// the object with |ASN1_OBJECT_free|. +// Although the output is not const, this function returns a static, immutable +// |ASN1_OBJECT|. It is not necessary to release the object with +// |ASN1_OBJECT_free|. // // However, functions like |X509_ALGOR_set0| expect to take ownership of a // possibly dynamically-allocated |ASN1_OBJECT|. |ASN1_OBJECT_free| is a no-op