From 8e6a26d128484b886e6dcbfa558b993d38950bb5 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 26 Dec 2023 08:28:47 -0500 Subject: [PATCH] Merge X509_PURPOSE/X509_TRUST IDs and indices OpenSSL's API uses this weird "index" intermediate integer representation, which is the same as the ID but offset bit. Just use the IDs throughout. Also document and deprecate the string-based APIs that rust-openssl uses. As a bonus, we remove some int/size_t casts. Change-Id: I3ffd2ab59bf3c9d96014a028b667b0bd3288b16b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65789 Commit-Queue: David Benjamin Reviewed-by: Bob Beck --- crypto/x509/internal.h | 5 +---- crypto/x509/v3_purp.c | 44 ++++++++++--------------------------- crypto/x509/x509_trs.c | 49 ++++++++++++++---------------------------- crypto/x509/x509_vfy.c | 8 +++---- crypto/x509/x509_vpm.c | 15 +++++++++++-- include/openssl/x509.h | 29 +++++++++++++++++++++---- 6 files changed, 70 insertions(+), 80 deletions(-) diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index d35c7f504..b2b6ad9a7 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h @@ -588,11 +588,8 @@ GENERAL_NAMES *v2i_GENERAL_NAMES(const X509V3_EXT_METHOD *method, // |X509_NAME| issue is resolved. int X509_check_akid(X509 *issuer, const AUTHORITY_KEYID *akid); -int X509_TRUST_set(int *t, int trust); -int X509_TRUST_get_by_id(int id); +int X509_is_valid_trust_id(int trust); -int X509_PURPOSE_set(int *p, int purpose); -int X509_PURPOSE_get_by_id(int id); int X509_PURPOSE_get_trust(const X509_PURPOSE *xp); diff --git a/crypto/x509/v3_purp.c b/crypto/x509/v3_purp.c index fe6cb0fea..78681c54a 100644 --- a/crypto/x509/v3_purp.c +++ b/crypto/x509/v3_purp.c @@ -54,8 +54,6 @@ * (eay@cryptsoft.com). This product includes software written by Tim * Hudson (tjh@cryptsoft.com). */ -#include -#include #include #include @@ -135,8 +133,8 @@ int X509_check_purpose(X509 *x, int id, int ca) { if (id == -1) { return 1; } - int idx = X509_PURPOSE_get_by_id(id); - if (idx == -1) { + const X509_PURPOSE *pt = X509_PURPOSE_get0(id); + if (pt == NULL) { return 0; } // Historically, |check_purpose| implementations other than |X509_PURPOSE_ANY| @@ -146,42 +144,22 @@ int X509_check_purpose(X509 *x, int id, int ca) { if (ca && id != X509_PURPOSE_ANY && !check_ca(x)) { return 0; } - const X509_PURPOSE *pt = X509_PURPOSE_get0(idx); return pt->check_purpose(pt, x, ca); } -int X509_PURPOSE_set(int *p, int purpose) { - if (X509_PURPOSE_get_by_id(purpose) == -1) { - OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_PURPOSE); - return 0; +const X509_PURPOSE *X509_PURPOSE_get0(int id) { + for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(xstandard); i++) { + if (xstandard[i].purpose == id) { + return &xstandard[i]; + } } - *p = purpose; - return 1; -} - -const X509_PURPOSE *X509_PURPOSE_get0(int idx) { - if (idx < 0 || (size_t)idx >= OPENSSL_ARRAY_SIZE(xstandard)) { - return NULL; - } - return xstandard + idx; + return NULL; } int X509_PURPOSE_get_by_sname(const char *sname) { - for (int i = 0; i < (int)OPENSSL_ARRAY_SIZE(xstandard); i++) { - const X509_PURPOSE *xptmp = X509_PURPOSE_get0(i); - if (!strcmp(xptmp->sname, sname)) { - return i; - } - } - return -1; -} - -int X509_PURPOSE_get_by_id(int purpose) { - for (size_t i = 0; i -#include - #include #include #include @@ -74,8 +71,6 @@ struct x509_trust_st { int nid; } /* X509_TRUST */; -static const X509_TRUST *X509_TRUST_get0(int idx); - static int trust_1oidany(const X509_TRUST *trust, X509 *x, int flags); static int trust_compat(const X509_TRUST *trust, X509 *x, int flags); @@ -89,6 +84,15 @@ static const X509_TRUST trstandard[] = { {X509_TRUST_OBJECT_SIGN, trust_1oidany, NID_code_sign}, {X509_TRUST_TSA, trust_1oidany, NID_time_stamp}}; +static const X509_TRUST *X509_TRUST_get0(int id) { + for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(trstandard); i++) { + if (trstandard[i].trust == id) { + return &trstandard[i]; + } + } + return NULL; +} + int X509_check_trust(X509 *x, int id, int flags) { if (id == -1) { return X509_TRUST_TRUSTED; @@ -101,39 +105,18 @@ int X509_check_trust(X509 *x, int id, int flags) { } return trust_compat(NULL, x, 0); } - int idx = X509_TRUST_get_by_id(id); - if (idx == -1) { + const X509_TRUST *pt = X509_TRUST_get0(id); + if (pt == NULL) { + // Unknown trust IDs are silently reintrepreted as NIDs. This is unreachable + // from the certificate verifier itself, but wpa_supplicant relies on it. + // Note this relies on commonly-used NIDs and trust IDs not colliding. return obj_trust(id, x, flags); } - const X509_TRUST *pt = X509_TRUST_get0(idx); return pt->check_trust(pt, x, flags); } -static const X509_TRUST *X509_TRUST_get0(int idx) { - if (idx < 0 || (size_t)idx >= OPENSSL_ARRAY_SIZE(trstandard)) { - return NULL; - } - return trstandard + idx; -} - -int X509_TRUST_get_by_id(int id) { - for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(trstandard); i++) { - if (trstandard[i].trust == id) { - static_assert(OPENSSL_ARRAY_SIZE(trstandard) <= INT_MAX, - "indices must fit in int"); - return (int)i; - } - } - return -1; -} - -int X509_TRUST_set(int *t, int trust) { - if (X509_TRUST_get_by_id(trust) == -1) { - OPENSSL_PUT_ERROR(X509, X509_R_INVALID_TRUST); - return 0; - } - *t = trust; - return 1; +int X509_is_valid_trust_id(int trust) { + return X509_TRUST_get0(trust) != NULL; } static int trust_1oidany(const X509_TRUST *trust, X509 *x, int flags) { diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index a6f6215dc..81e413e04 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -1485,13 +1485,13 @@ int X509_STORE_CTX_set_purpose(X509_STORE_CTX *ctx, int purpose) { return 1; } - int idx = X509_PURPOSE_get_by_id(purpose); - if (idx == -1) { + const X509_PURPOSE *pobj = X509_PURPOSE_get0(purpose); + if (pobj == NULL) { OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_PURPOSE_ID); return 0; } - int trust = X509_PURPOSE_get_trust(X509_PURPOSE_get0(idx)); + int trust = X509_PURPOSE_get_trust(pobj); if (!X509_STORE_CTX_set_trust(ctx, trust)) { return 0; } @@ -1508,7 +1508,7 @@ int X509_STORE_CTX_set_trust(X509_STORE_CTX *ctx, int trust) { return 1; } - if (X509_TRUST_get_by_id(trust) == -1) { + if (!X509_is_valid_trust_id(trust)) { OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_TRUST_ID); return 0; } diff --git a/crypto/x509/x509_vpm.c b/crypto/x509/x509_vpm.c index 7314c4438..0640fd08b 100644 --- a/crypto/x509/x509_vpm.c +++ b/crypto/x509/x509_vpm.c @@ -269,11 +269,22 @@ unsigned long X509_VERIFY_PARAM_get_flags(const X509_VERIFY_PARAM *param) { } int X509_VERIFY_PARAM_set_purpose(X509_VERIFY_PARAM *param, int purpose) { - return X509_PURPOSE_set(¶m->purpose, purpose); + if (X509_PURPOSE_get0(purpose) == NULL) { + OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_PURPOSE); + return 0; + } + param->purpose = purpose; + return 1; } int X509_VERIFY_PARAM_set_trust(X509_VERIFY_PARAM *param, int trust) { - return X509_TRUST_set(¶m->trust, trust); + if (!X509_is_valid_trust_id(trust)) { + OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_TRUST_ID); + return 0; + } + + param->trust = trust; + return 1; } void X509_VERIFY_PARAM_set_depth(X509_VERIFY_PARAM *param, int depth) { diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 60d183565..23d643b85 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -4313,6 +4313,31 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_chain(X509_STORE_CTX *ctx, OPENSSL_EXPORT STACK_OF(X509_OBJECT) *X509_STORE_get0_objects( X509_STORE *store); +// X509_PURPOSE_get_by_sname returns the |X509_PURPOSE_*| constant corresponding +// a short name |sname|, or -1 if |sname| was not recognized. +// +// Use |X509_PURPOSE_*| constants directly instead. The short names used by this +// function look like "sslserver" or "smimeencrypt", so they do not make +// especially good APIs. +// +// This function differs from OpenSSL, which returns an "index" to be passed to +// |X509_PURPOSE_get0|, followed by |X509_PURPOSE_get_id|, to finally obtain an +// |X509_PURPOSE_*| value suitable for use with |X509_VERIFY_PARAM_set_purpose|. +OPENSSL_EXPORT int X509_PURPOSE_get_by_sname(const char *sname); + +// X509_PURPOSE_get0 returns the |X509_PURPOSE| object corresponding to |id|, +// which should be one of the |X509_PURPOSE_*| constants, or NULL if none +// exists. +// +// This function differs from OpenSSL, which takes an "index", returned from +// |X509_PURPOSE_get_by_sname|. In BoringSSL, indices and |X509_PURPOSE_*| IDs +// are the same. +OPENSSL_EXPORT const X509_PURPOSE *X509_PURPOSE_get0(int id); + +// X509_PURPOSE_get_id returns |purpose|'s ID. This will be one of the +// |X509_PURPOSE_*| constants. +OPENSSL_EXPORT int X509_PURPOSE_get_id(const X509_PURPOSE *purpose); + // Private structures. @@ -4859,10 +4884,6 @@ OPENSSL_EXPORT X509_EXTENSION *X509V3_EXT_i2d(int ext_nid, int crit, OPENSSL_EXPORT int X509V3_add1_i2d(STACK_OF(X509_EXTENSION) **x, int nid, void *value, int crit, unsigned long flags); -OPENSSL_EXPORT int X509_PURPOSE_get_by_sname(const char *sname); -OPENSSL_EXPORT const X509_PURPOSE *X509_PURPOSE_get0(int idx); -OPENSSL_EXPORT int X509_PURPOSE_get_id(const X509_PURPOSE *purpose); - #if defined(__cplusplus) } // extern C