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
