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