Remove dynamic X509_TRUST and X509_PURPOSE registration
This is not thread-safe. Even if made thread-safe, it involves
registering globals, so it's just not a good API.
Note this means that there is no longer a way to configure custom trust
OIDs or purpose checks. Evidently no one was doing that. Should a use
case arise, I don't think it should be met by this API. The things one
might want to configure here are:
- Which OID to match against X509_add1_trust_object and
X509_add1_reject_object
- Whether self-signed certificates, if no trust objects are configured,
also count as trust anchors
- Which EKU OID to look for up the chain
- Which legacy Netscape certificate type to look for (can we remove
this?)
- Which key usage bits to look for in the leaf
We can simply add APIs for specifying those if we need them.
Interestingly, there's a call to check_ca inside the purpose checks
(which gets skipped if you don't configure a purpose!), but I think it
may be redundant with the X509_check_ca call in the path verifier.
Change-Id: If71ee3d0768b5fc71422852b4fcf7eb23e937dd2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64507
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/v3_purp.c b/crypto/x509/v3_purp.c
index de1e8be..9db45ba 100644
--- a/crypto/x509/v3_purp.c
+++ b/crypto/x509/v3_purp.c
@@ -94,9 +94,6 @@
static int no_check(const X509_PURPOSE *xp, const X509 *x, int ca);
static int ocsp_helper(const X509_PURPOSE *xp, const X509 *x, int ca);
-static int xp_cmp(const X509_PURPOSE *const *a, const X509_PURPOSE *const *b);
-static void xptable_free(X509_PURPOSE *p);
-
static X509_PURPOSE xstandard[] = {
{X509_PURPOSE_SSL_CLIENT, X509_TRUST_SSL_CLIENT, 0,
check_purpose_ssl_client, (char *)"SSL client", (char *)"sslclient", NULL},
@@ -121,14 +118,6 @@
(char *)"timestampsign", NULL},
};
-#define X509_PURPOSE_COUNT (sizeof(xstandard) / sizeof(X509_PURPOSE))
-
-static STACK_OF(X509_PURPOSE) *xptable = NULL;
-
-static int xp_cmp(const X509_PURPOSE *const *a, const X509_PURPOSE *const *b) {
- return (*a)->purpose - (*b)->purpose;
-}
-
// As much as I'd like to make X509_check_purpose use a "const" X509* I
// really can't because it does recalculate hashes and do other non-const
// things.
@@ -159,21 +148,13 @@
return 1;
}
-int X509_PURPOSE_get_count(void) {
- if (!xptable) {
- return X509_PURPOSE_COUNT;
- }
- return sk_X509_PURPOSE_num(xptable) + X509_PURPOSE_COUNT;
-}
+int X509_PURPOSE_get_count(void) { return OPENSSL_ARRAY_SIZE(xstandard); }
X509_PURPOSE *X509_PURPOSE_get0(int idx) {
- if (idx < 0) {
+ if (idx < 0 || (size_t)idx >= OPENSSL_ARRAY_SIZE(xstandard)) {
return NULL;
}
- if (idx < (int)X509_PURPOSE_COUNT) {
- return xstandard + idx;
- }
- return sk_X509_PURPOSE_value(xptable, idx - X509_PURPOSE_COUNT);
+ return xstandard + idx;
}
int X509_PURPOSE_get_by_sname(const char *sname) {
@@ -188,118 +169,10 @@
}
int X509_PURPOSE_get_by_id(int purpose) {
- X509_PURPOSE tmp;
- size_t idx;
-
- if ((purpose >= X509_PURPOSE_MIN) && (purpose <= X509_PURPOSE_MAX)) {
+ if (purpose >= X509_PURPOSE_MIN && purpose <= X509_PURPOSE_MAX) {
return purpose - X509_PURPOSE_MIN;
}
- tmp.purpose = purpose;
- if (!xptable) {
- return -1;
- }
-
- if (!sk_X509_PURPOSE_find(xptable, &idx, &tmp)) {
- return -1;
- }
- return idx + X509_PURPOSE_COUNT;
-}
-
-int X509_PURPOSE_add(int id, int trust, int flags,
- int (*ck)(const X509_PURPOSE *, const X509 *, int),
- const char *name, const char *sname, void *arg) {
- X509_PURPOSE *ptmp;
- char *name_dup, *sname_dup;
-
- // This is set according to what we change: application can't set it
- flags &= ~X509_PURPOSE_DYNAMIC;
- // This will always be set for application modified trust entries
- flags |= X509_PURPOSE_DYNAMIC_NAME;
- // Get existing entry if any
- int idx = X509_PURPOSE_get_by_id(id);
- // Need a new entry
- if (idx == -1) {
- if (!(ptmp = OPENSSL_malloc(sizeof(X509_PURPOSE)))) {
- return 0;
- }
- ptmp->flags = X509_PURPOSE_DYNAMIC;
- } else {
- ptmp = X509_PURPOSE_get0(idx);
- }
-
- // Duplicate the supplied names.
- name_dup = OPENSSL_strdup(name);
- sname_dup = OPENSSL_strdup(sname);
- if (name_dup == NULL || sname_dup == NULL) {
- if (name_dup != NULL) {
- OPENSSL_free(name_dup);
- }
- if (sname_dup != NULL) {
- OPENSSL_free(sname_dup);
- }
- if (idx == -1) {
- OPENSSL_free(ptmp);
- }
- return 0;
- }
-
- // OPENSSL_free existing name if dynamic
- if (ptmp->flags & X509_PURPOSE_DYNAMIC_NAME) {
- OPENSSL_free(ptmp->name);
- OPENSSL_free(ptmp->sname);
- }
- // dup supplied name
- ptmp->name = name_dup;
- ptmp->sname = sname_dup;
- // Keep the dynamic flag of existing entry
- ptmp->flags &= X509_PURPOSE_DYNAMIC;
- // Set all other flags
- ptmp->flags |= flags;
-
- ptmp->purpose = id;
- ptmp->trust = trust;
- ptmp->check_purpose = ck;
- ptmp->usr_data = arg;
-
- // If its a new entry manage the dynamic table
- if (idx == -1) {
- // TODO(davidben): This should be locked. Alternatively, remove the dynamic
- // registration mechanism entirely. The trouble is there no way to pass in
- // the various parameters into an |X509_VERIFY_PARAM| directly. You can only
- // register it in the global table and get an ID.
- if (!xptable && !(xptable = sk_X509_PURPOSE_new(xp_cmp))) {
- xptable_free(ptmp);
- return 0;
- }
- if (!sk_X509_PURPOSE_push(xptable, ptmp)) {
- xptable_free(ptmp);
- return 0;
- }
- sk_X509_PURPOSE_sort(xptable);
- }
- return 1;
-}
-
-static void xptable_free(X509_PURPOSE *p) {
- if (!p) {
- return;
- }
- if (p->flags & X509_PURPOSE_DYNAMIC) {
- if (p->flags & X509_PURPOSE_DYNAMIC_NAME) {
- OPENSSL_free(p->name);
- OPENSSL_free(p->sname);
- }
- OPENSSL_free(p);
- }
-}
-
-void X509_PURPOSE_cleanup(void) {
- unsigned int i;
- sk_X509_PURPOSE_pop_free(xptable, xptable_free);
- for (i = 0; i < X509_PURPOSE_COUNT; i++) {
- xptable_free(xstandard + i);
- }
- xptable = NULL;
+ return -1;
}
int X509_PURPOSE_get_id(const X509_PURPOSE *xp) { return xp->purpose; }
diff --git a/crypto/x509/x509_trs.c b/crypto/x509/x509_trs.c
index 53819b3..ce4194b 100644
--- a/crypto/x509/x509_trs.c
+++ b/crypto/x509/x509_trs.c
@@ -59,12 +59,10 @@
#include <openssl/obj.h>
#include <openssl/x509.h>
+#include "../internal.h"
#include "internal.h"
-static int tr_cmp(const X509_TRUST *const *a, const X509_TRUST *const *b);
-static void trtable_free(X509_TRUST *p);
-
static int trust_1oidany(X509_TRUST *trust, X509 *x, int flags);
static int trust_1oid(X509_TRUST *trust, X509 *x, int flags);
static int trust_compat(X509_TRUST *trust, X509 *x, int flags);
@@ -92,14 +90,6 @@
{X509_TRUST_TSA, 0, trust_1oidany, (char *)"TSA server", NID_time_stamp,
NULL}};
-#define X509_TRUST_COUNT (sizeof(trstandard) / sizeof(X509_TRUST))
-
-static STACK_OF(X509_TRUST) *trtable = NULL;
-
-static int tr_cmp(const X509_TRUST *const *a, const X509_TRUST *const *b) {
- return (*a)->trust - (*b)->trust;
-}
-
int X509_check_trust(X509 *x, int id, int flags) {
X509_TRUST *pt;
int idx;
@@ -123,38 +113,20 @@
return pt->check_trust(pt, x, flags);
}
-int X509_TRUST_get_count(void) {
- if (!trtable) {
- return X509_TRUST_COUNT;
- }
- return sk_X509_TRUST_num(trtable) + X509_TRUST_COUNT;
-}
+int X509_TRUST_get_count(void) { return OPENSSL_ARRAY_SIZE(trstandard); }
X509_TRUST *X509_TRUST_get0(int idx) {
- if (idx < 0) {
+ if (idx < 0 || (size_t)idx >= OPENSSL_ARRAY_SIZE(trstandard)) {
return NULL;
}
- if (idx < (int)X509_TRUST_COUNT) {
- return trstandard + idx;
- }
- return sk_X509_TRUST_value(trtable, idx - X509_TRUST_COUNT);
+ return trstandard + idx;
}
int X509_TRUST_get_by_id(int id) {
- X509_TRUST tmp;
- size_t idx;
-
- if ((id >= X509_TRUST_MIN) && (id <= X509_TRUST_MAX)) {
+ if (id >= X509_TRUST_MIN && id <= X509_TRUST_MAX) {
return id - X509_TRUST_MIN;
}
- tmp.trust = id;
- if (!trtable) {
- return -1;
- }
- if (!sk_X509_TRUST_find(trtable, &idx, &tmp)) {
- return -1;
- }
- return idx + X509_TRUST_COUNT;
+ return -1;
}
int X509_TRUST_set(int *t, int trust) {
@@ -166,92 +138,6 @@
return 1;
}
-int X509_TRUST_add(int id, int flags, int (*ck)(X509_TRUST *, X509 *, int),
- const char *name, int arg1, void *arg2) {
- int idx;
- X509_TRUST *trtmp;
- char *name_dup;
-
- // This is set according to what we change: application can't set it
- flags &= ~X509_TRUST_DYNAMIC;
- // This will always be set for application modified trust entries
- flags |= X509_TRUST_DYNAMIC_NAME;
- // Get existing entry if any
- idx = X509_TRUST_get_by_id(id);
- // Need a new entry
- if (idx == -1) {
- if (!(trtmp = OPENSSL_malloc(sizeof(X509_TRUST)))) {
- return 0;
- }
- trtmp->flags = X509_TRUST_DYNAMIC;
- } else {
- trtmp = X509_TRUST_get0(idx);
- }
-
- // Duplicate the supplied name.
- name_dup = OPENSSL_strdup(name);
- if (name_dup == NULL) {
- if (idx == -1) {
- OPENSSL_free(trtmp);
- }
- return 0;
- }
-
- // OPENSSL_free existing name if dynamic
- if (trtmp->flags & X509_TRUST_DYNAMIC_NAME) {
- OPENSSL_free(trtmp->name);
- }
- trtmp->name = name_dup;
- // Keep the dynamic flag of existing entry
- trtmp->flags &= X509_TRUST_DYNAMIC;
- // Set all other flags
- trtmp->flags |= flags;
-
- trtmp->trust = id;
- trtmp->check_trust = ck;
- trtmp->arg1 = arg1;
- trtmp->arg2 = arg2;
-
- // If its a new entry manage the dynamic table
- if (idx == -1) {
- // TODO(davidben): This should be locked. Alternatively, remove the dynamic
- // registration mechanism entirely. The trouble is there no way to pass in
- // the various parameters into an |X509_VERIFY_PARAM| directly. You can only
- // register it in the global table and get an ID.
- if (!trtable && !(trtable = sk_X509_TRUST_new(tr_cmp))) {
- trtable_free(trtmp);
- return 0;
- }
- if (!sk_X509_TRUST_push(trtable, trtmp)) {
- trtable_free(trtmp);
- return 0;
- }
- sk_X509_TRUST_sort(trtable);
- }
- return 1;
-}
-
-static void trtable_free(X509_TRUST *p) {
- if (!p) {
- return;
- }
- if (p->flags & X509_TRUST_DYNAMIC) {
- if (p->flags & X509_TRUST_DYNAMIC_NAME) {
- OPENSSL_free(p->name);
- }
- OPENSSL_free(p);
- }
-}
-
-void X509_TRUST_cleanup(void) {
- unsigned int i;
- for (i = 0; i < X509_TRUST_COUNT; i++) {
- trtable_free(trstandard + i);
- }
- sk_X509_TRUST_pop_free(trtable, trtable_free);
- trtable = NULL;
-}
-
int X509_TRUST_get_flags(const X509_TRUST *xp) { return xp->flags; }
char *X509_TRUST_get0_name(const X509_TRUST *xp) { return xp->name; }
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index aa219b1..b7352af 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -3137,11 +3137,6 @@
#define X509_TRUST_MIN 1
#define X509_TRUST_MAX 8
-
-// trust_flags values
-#define X509_TRUST_DYNAMIC 1
-#define X509_TRUST_DYNAMIC_NAME 2
-
// check_trust return codes
#define X509_TRUST_TRUSTED 1
@@ -3184,10 +3179,6 @@
OPENSSL_EXPORT int X509_TRUST_get_count(void);
OPENSSL_EXPORT X509_TRUST *X509_TRUST_get0(int idx);
OPENSSL_EXPORT int X509_TRUST_get_by_id(int id);
-OPENSSL_EXPORT int X509_TRUST_add(int id, int flags,
- int (*ck)(X509_TRUST *, X509 *, int),
- const char *name, int arg1, void *arg2);
-OPENSSL_EXPORT void X509_TRUST_cleanup(void);
OPENSSL_EXPORT int X509_TRUST_get_flags(const X509_TRUST *xp);
OPENSSL_EXPORT char *X509_TRUST_get0_name(const X509_TRUST *xp);
OPENSSL_EXPORT int X509_TRUST_get_trust(const X509_TRUST *xp);
@@ -3967,9 +3958,6 @@
#define NS_OBJSIGN_CA 0x01
#define NS_ANY_CA (NS_SSL_CA | NS_SMIME_CA | NS_OBJSIGN_CA)
-#define X509_PURPOSE_DYNAMIC 0x1
-#define X509_PURPOSE_DYNAMIC_NAME 0x2
-
typedef struct x509_purpose_st {
int purpose;
int trust; // Default trust ID
@@ -4237,15 +4225,9 @@
OPENSSL_EXPORT X509_PURPOSE *X509_PURPOSE_get0(int idx);
OPENSSL_EXPORT int X509_PURPOSE_get_by_sname(const char *sname);
OPENSSL_EXPORT int X509_PURPOSE_get_by_id(int id);
-OPENSSL_EXPORT int X509_PURPOSE_add(int id, int trust, int flags,
- int (*ck)(const X509_PURPOSE *,
- const X509 *, int),
- const char *name, const char *sname,
- void *arg);
OPENSSL_EXPORT char *X509_PURPOSE_get0_name(const X509_PURPOSE *xp);
OPENSSL_EXPORT char *X509_PURPOSE_get0_sname(const X509_PURPOSE *xp);
OPENSSL_EXPORT int X509_PURPOSE_get_trust(const X509_PURPOSE *xp);
-OPENSSL_EXPORT void X509_PURPOSE_cleanup(void);
OPENSSL_EXPORT int X509_PURPOSE_get_id(const X509_PURPOSE *);
// Flags for X509_check_* functions
