Skip to content

Commit

Permalink
Unexport various unused X509_OBJECT and X509_LOOKUP functions.
Browse files Browse the repository at this point in the history 
Some things of note:

- Anyone calling X509_OBJECT_up_ref_count is breaking X509_OBJECT's
  internal invariants, or relying on someone else handing back an
  X509_OBJECT with broken invariants.

- X509_LOOKUP_by_subject hands back an X509_OBJECT with broken internal
  invariants. Fortunately, it is never called, so unexport it as a the
  first step to cleaning this up.

Change-Id: Ia67693f802671cf857bf51aec6e20f27d1525212
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64130
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit d9b81bb43a24b3adb6e8a616a4828e1bad89c486)
  • Loading branch information
@davidben @justsmth
davidben authored and justsmth committed Apr 3, 2024
1 parent f496035 commit d54a7bc
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 49 deletions.
53 changes: 29 additions & 24 deletions crypto/x509/x509_lu.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,22 @@
#include "../internal.h"
#include "internal.h"

X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method) {
X509_LOOKUP *ret;

ret = (X509_LOOKUP *)OPENSSL_zalloc(sizeof(X509_LOOKUP));
static int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
X509_NAME *name);
static X509_OBJECT *X509_OBJECT_retrieve_by_subject(
STACK_OF(X509_OBJECT) *h, int type, X509_NAME *name);
static X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
X509_OBJECT *x);
static int X509_OBJECT_up_ref_count(X509_OBJECT *a);

static X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method);
static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
X509_OBJECT *ret);
static int X509_LOOKUP_shutdown(X509_LOOKUP *ctx);

static X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method) {
X509_LOOKUP *ret = OPENSSL_zalloc(sizeof(X509_LOOKUP));
if (ret == NULL) {
return NULL;
}
Expand All @@ -91,18 +103,7 @@ void X509_LOOKUP_free(X509_LOOKUP *ctx) {
OPENSSL_free(ctx);
}

int X509_LOOKUP_init(X509_LOOKUP *ctx) {
if (ctx->method == NULL) {
return 0;
}
if (ctx->method->init != NULL) {
return ctx->method->init(ctx);
} else {
return 1;
}
}

int X509_LOOKUP_shutdown(X509_LOOKUP *ctx) {
static int X509_LOOKUP_shutdown(X509_LOOKUP *ctx) {
if (ctx->method == NULL) {
return 0;
}
Expand All @@ -125,14 +126,18 @@ int X509_LOOKUP_ctrl(X509_LOOKUP *ctx, int cmd, const char *argc, long argl,
}
}

int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
X509_OBJECT *ret) {
static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
X509_OBJECT *ret) {
if ((ctx->method == NULL) || (ctx->method->get_by_subject == NULL)) {
return 0;
}
if (ctx->skip) {
return 0;
}
// Note |get_by_subject| leaves |ret| in an inconsistent state. It has
// pointers to an |X509| or |X509_CRL|, but has not bumped the refcount yet.
// For now, the caller is expected to fix this, but ideally we'd fix the
// |X509_LOOKUP| convention itself.
return ctx->method->get_by_subject(ctx, type, name, ret) > 0;
}

Expand Down Expand Up @@ -365,7 +370,7 @@ void X509_OBJECT_free(X509_OBJECT *obj) {
OPENSSL_free(obj);
}

int X509_OBJECT_up_ref_count(X509_OBJECT *a) {
static int X509_OBJECT_up_ref_count(X509_OBJECT *a) {
switch (a->type) {
case X509_LU_X509:
X509_up_ref(a->data.x509);
Expand Down Expand Up @@ -473,13 +478,13 @@ static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, int type,
return (int)idx;
}

int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
X509_NAME *name) {
static int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
X509_NAME *name) {
return x509_object_idx_cnt(h, type, name, NULL);
}

X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h, int type,
X509_NAME *name) {
X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h,
int type, X509_NAME *name) {
int idx;
idx = X509_OBJECT_idx_by_subject(h, type, name);
if (idx == -1) {
Expand Down Expand Up @@ -574,8 +579,8 @@ STACK_OF(X509_CRL) *X509_STORE_get1_crls(X509_STORE_CTX *ctx, X509_NAME *nm) {
return sk;
}

X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
X509_OBJECT *x) {
static X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
X509_OBJECT *x) {
sk_X509_OBJECT_sort(h);
size_t idx;
if (!sk_X509_OBJECT_find_awslc(h, &idx, x)) {
Expand Down
11 changes: 2 additions & 9 deletions crypto/x509/x509_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1851,7 +1851,7 @@ TEST(X509Test, MatchFoundSetsPeername) {
char *peername = nullptr;
EXPECT_NE(1, X509_check_host(leaf.get(), kWrongHostname, strlen(kWrongHostname), 0, &peername));
ASSERT_EQ(nullptr, peername);

EXPECT_EQ(1, X509_check_host(leaf.get(), kHostname, strlen(kHostname), 0, &peername));
EXPECT_STREQ(peername, kHostname);
OPENSSL_free(peername);
Expand Down Expand Up @@ -5086,15 +5086,8 @@ TEST(X509Test, AddDuplicates) {
ASSERT_TRUE(X509_STORE_lock(store.get()));
// Sleep after taking the lock to cause contention. Sleep longer than the
// adder half of threads to ensure we hold the lock while they contend
// for it. |X509_OBJECT_retrieve_by_subject| is called because it doesn't
// take a lock on the store, thus avoiding deadlock.
// for it.
std::this_thread::sleep_for(std::chrono::microseconds(11 + (sleep_buf[0] % 5)));
EXPECT_TRUE(X509_OBJECT_retrieve_by_subject(
store->objs, X509_LU_X509, X509_get_subject_name(a.get())
));
EXPECT_TRUE(X509_OBJECT_retrieve_by_subject(
store->objs, X509_LU_X509, X509_get_subject_name(b.get())
));
ASSERT_TRUE(X509_STORE_unlock(store.get()));
});
}
Expand Down
23 changes: 7 additions & 16 deletions include/openssl/x509.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2658,6 +2658,13 @@ OPENSSL_EXPORT int X509_NAME_get_text_by_NID(const X509_NAME *name, int nid,
OPENSSL_EXPORT X509_STORE_CTX *X509_STORE_CTX_get0_parent_ctx(
X509_STORE_CTX *ctx);

// X509_LOOKUP_free releases memory associated with |ctx|. This function should
// never be used outside the library. No function in the public API hands
// ownership of an |X509_LOOKUP| to the caller.
//
// TODO(davidben): Unexport this function after rust-openssl is fixed to no
// longer call it.
OPENSSL_EXPORT void X509_LOOKUP_free(X509_LOOKUP *ctx);

// Private structures.

Expand Down Expand Up @@ -3016,13 +3023,6 @@ OPENSSL_EXPORT X509_OBJECT *X509_OBJECT_new(void);
// X509_OBJECT_free frees an |X509_OBJECT| from the heap.
OPENSSL_EXPORT void X509_OBJECT_free(X509_OBJECT *a);

OPENSSL_EXPORT int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h,
int type, X509_NAME *name);
OPENSSL_EXPORT X509_OBJECT *X509_OBJECT_retrieve_by_subject(
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);
Expand All @@ -3038,7 +3038,6 @@ OPENSSL_EXPORT int X509_OBJECT_get_type(const X509_OBJECT *obj);
// 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 int X509_OBJECT_get_type(const X509_OBJECT *a);
OPENSSL_EXPORT X509 *X509_OBJECT_get0_X509(const X509_OBJECT *a);
// X509_OBJECT_get0_X509_CRL returns the |X509_CRL| associated with |a|
Expand Down Expand Up @@ -3182,13 +3181,6 @@ OPENSSL_EXPORT int X509_load_crl_file(X509_LOOKUP *ctx, const char *file,
OPENSSL_EXPORT int X509_load_cert_crl_file(X509_LOOKUP *ctx, const char *file,
int type);

OPENSSL_EXPORT X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method);
OPENSSL_EXPORT void X509_LOOKUP_free(X509_LOOKUP *ctx);
OPENSSL_EXPORT int X509_LOOKUP_init(X509_LOOKUP *ctx);
OPENSSL_EXPORT int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type,
X509_NAME *name, X509_OBJECT *ret);
OPENSSL_EXPORT int X509_LOOKUP_shutdown(X509_LOOKUP *ctx);

OPENSSL_EXPORT int X509_STORE_load_locations(X509_STORE *ctx, const char *file,
const char *dir);
OPENSSL_EXPORT int X509_STORE_set_default_paths(X509_STORE *ctx);
Expand Down Expand Up @@ -3379,7 +3371,6 @@ BORINGSSL_MAKE_DELETER(X509_ATTRIBUTE, X509_ATTRIBUTE_free)
BORINGSSL_MAKE_DELETER(X509_CRL, X509_CRL_free)
BORINGSSL_MAKE_UP_REF(X509_CRL, X509_CRL_up_ref)
BORINGSSL_MAKE_DELETER(X509_EXTENSION, X509_EXTENSION_free)
BORINGSSL_MAKE_DELETER(X509_LOOKUP, X509_LOOKUP_free)
BORINGSSL_MAKE_DELETER(X509_OBJECT, X509_OBJECT_free)
BORINGSSL_MAKE_DELETER(X509_NAME, X509_NAME_free)
BORINGSSL_MAKE_DELETER(X509_NAME_ENTRY, X509_NAME_ENTRY_free)
Expand Down

0 comments on commit d54a7bc

Please sign in to comment.