Add X509_OBJECT_new and X509_OBJECT_free
This is a bit of a mess. The immediate motivation here is that there is
no legitimate reason to ever call X509_OBJECT_free_contents outside of
the library. Unsurprisingly, this means rust-openssl uses it.
rust-openssl uses it because they want to be able to free X509_OBJECTs.
Add OpenSSL 1.1.x's X509_OBJECT_free, which is what they should be using
it instead. As it turns out, they don't *actually* need to free
X509_OBJECTs. This is just some design mistake that cause them to need
free functions for types they never free. On top of that, the only
reason rust-openssl references X509_OBJECT is for
X509_STORE_get0_objects, but their use of that API is a Rust safety
violation anyway. It's all a mess.
As for whether freeing it ever makes sense, the question is whether
X509_STORE_get_by_subject needs to be a public API. In so far as it is
public, callers would need to create empty X509_OBJECTs as an output,
now that X509_OBJECT is opaque. There are also other users of
X509_STORE_get0_objects that might benefit from an
X509_STORE_get1_objects, in which case X509_OBJECT_free will be useful.
For now just to unblock fixing the more immediate rust-openssl mistake
(rather than the underlying mistake), add the APIs that
X509_STORE_get_by_subject callers would need if they existed.
There's quite a bit to clean up around X509_OBJECT, but start by adding
these APIs.
As part of this, since rust-openssl prevents us from removing
X509_OBJECT_free_contents, deprecate it and fix it to leave the
X509_OBJECT in a self-consistent state. (This is moot because
rust-openssl will never call it, but still.)
Change-Id: I78708f2d2464eb9a18844fef0d62cb0a727b9f47
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64129
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c
index 814f0ce..d113cee 100644
--- a/crypto/x509/x509_lu.c
+++ b/crypto/x509/x509_lu.c
@@ -205,21 +205,6 @@
return 1;
}
-static void cleanup(X509_OBJECT *a) {
- if (a == NULL) {
- return;
- }
- if (a->type == X509_LU_X509) {
- X509_free(a->data.x509);
- } else if (a->type == X509_LU_CRL) {
- X509_CRL_free(a->data.crl);
- } else {
- // abort();
- }
-
- OPENSSL_free(a);
-}
-
void X509_STORE_free(X509_STORE *vfy) {
size_t j;
STACK_OF(X509_LOOKUP) *sk;
@@ -242,7 +227,7 @@
X509_LOOKUP_free(lu);
}
sk_X509_LOOKUP_free(sk);
- sk_X509_OBJECT_pop_free(vfy->objs, cleanup);
+ sk_X509_OBJECT_pop_free(vfy->objs, X509_OBJECT_free);
if (vfy->param) {
X509_VERIFY_PARAM_free(vfy->param);
@@ -316,7 +301,7 @@
return 0;
}
- X509_OBJECT *const obj = (X509_OBJECT *)OPENSSL_malloc(sizeof(X509_OBJECT));
+ X509_OBJECT *const obj = X509_OBJECT_new();
if (obj == NULL) {
return 0;
}
@@ -342,8 +327,7 @@
CRYPTO_MUTEX_unlock_write(&ctx->objs_lock);
if (!added) {
- X509_OBJECT_free_contents(obj);
- OPENSSL_free(obj);
+ X509_OBJECT_free(obj);
}
return ret;
@@ -357,6 +341,18 @@
return x509_store_add(ctx, x, /*is_crl=*/1);
}
+X509_OBJECT *X509_OBJECT_new(void) {
+ return OPENSSL_zalloc(sizeof(X509_OBJECT));
+}
+
+void X509_OBJECT_free(X509_OBJECT *obj) {
+ if (obj == NULL) {
+ return;
+ }
+ X509_OBJECT_free_contents(obj);
+ OPENSSL_free(obj);
+}
+
int X509_OBJECT_up_ref_count(X509_OBJECT *a) {
switch (a->type) {
case X509_LU_X509:
@@ -378,6 +374,8 @@
X509_CRL_free(a->data.crl);
break;
}
+
+ OPENSSL_memset(a, 0, sizeof(X509_OBJECT));
}
int X509_OBJECT_get_type(const X509_OBJECT *a) { return a->type; }
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index df3dd58..dfe66c5 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -2582,6 +2582,13 @@
OPENSSL_EXPORT X509_STORE_CTX *X509_STORE_CTX_get0_parent_ctx(
X509_STORE_CTX *ctx);
+// X509_OBJECT_free_contents sets |obj| to the empty object, freeing any values
+// that were previously there.
+//
+// TODO(davidben): Unexport this function after rust-openssl is fixed to no
+// longer call it.
+OPENSSL_EXPORT void X509_OBJECT_free_contents(X509_OBJECT *obj);
+
// Private structures.
@@ -2723,6 +2730,7 @@
certificate chain.
*/
+#define X509_LU_NONE 0
#define X509_LU_X509 1
#define X509_LU_CRL 2
#define X509_LU_PKEY 3
@@ -2900,10 +2908,23 @@
STACK_OF(X509_OBJECT) *h, int type, X509_NAME *name);
OPENSSL_EXPORT X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
X509_OBJECT *x);
+
+// X509_OBJECT_new returns a newly-allocated, empty |X509_OBJECT| or NULL on
+// error.
+OPENSSL_EXPORT X509_OBJECT *X509_OBJECT_new(void);
+
+// X509_OBJECT_free releases memory associated with |obj|.
+OPENSSL_EXPORT void X509_OBJECT_free(X509_OBJECT *obj);
+
+// X509_OBJECT_get_type returns the type of |obj|, which will be one of the
+// |X509_LU_*| constants.
+OPENSSL_EXPORT int X509_OBJECT_get_type(const X509_OBJECT *obj);
+
+// X509_OBJECT_get0_X509 returns |obj| as a certificate, or NULL if |obj| is not
+// a certificate.
+OPENSSL_EXPORT X509 *X509_OBJECT_get0_X509(const X509_OBJECT *obj);
+
OPENSSL_EXPORT int X509_OBJECT_up_ref_count(X509_OBJECT *a);
-OPENSSL_EXPORT void X509_OBJECT_free_contents(X509_OBJECT *a);
-OPENSSL_EXPORT int X509_OBJECT_get_type(const X509_OBJECT *a);
-OPENSSL_EXPORT X509 *X509_OBJECT_get0_X509(const X509_OBJECT *a);
OPENSSL_EXPORT X509_STORE *X509_STORE_new(void);
OPENSSL_EXPORT int X509_STORE_up_ref(X509_STORE *store);
OPENSSL_EXPORT void X509_STORE_free(X509_STORE *v);
