From: Dr. David von Oheimb Date: Wed, 3 Mar 2021 19:10:34 +0000 (+0100) Subject: X509{,_LOOKUP}: Improve distinction between not found and fatal/internal error X-Git-Tag: openssl-3.2.0-alpha1~2709 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0ce8271c20c95d21d9641c0ead76a86f818c45e9;p=thirdparty%2Fopenssl.git X509{,_LOOKUP}: Improve distinction between not found and fatal/internal error Reviewed-by: Tomas Mraz Reviewed-by: Shane Lontis Reviewed-by: David von Oheimb (Merged from https://github.com/openssl/openssl/pull/14417) --- diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index d02777fa582..278fbe581c4 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c @@ -305,10 +305,15 @@ X509_OBJECT *X509_STORE_CTX_get_obj_by_subject(X509_STORE_CTX *vs, return ret; } -/* Also fill the cache with all matching certificates */ -int X509_STORE_CTX_get_by_subject(const X509_STORE_CTX *vs, - X509_LOOKUP_TYPE type, - const X509_NAME *name, X509_OBJECT *ret) +/* + * Returns 1 if successful, + * 0 if not found or X509_LOOKUP_by_subject_ex() returns an error, + * -1 on failure + */ +static int ossl_x509_store_ctx_get_by_subject(const X509_STORE_CTX *vs, + X509_LOOKUP_TYPE type, + const X509_NAME *name, + X509_OBJECT *ret) { X509_STORE *store = vs->store; X509_LOOKUP *lu; @@ -323,16 +328,19 @@ int X509_STORE_CTX_get_by_subject(const X509_STORE_CTX *vs, if (!X509_STORE_lock(store)) return 0; - tmp = X509_OBJECT_retrieve_by_subject(store->objs, type, name); X509_STORE_unlock(store); if (tmp == NULL || type == X509_LU_CRL) { for (i = 0; i < sk_X509_LOOKUP_num(store->get_cert_methods); i++) { lu = sk_X509_LOOKUP_value(store->get_cert_methods, i); - j = X509_LOOKUP_by_subject_ex(lu, type, name, &stmp, vs->libctx, - vs->propq); - if (j) { + if (lu->skip) + continue; + if (lu->method == NULL) + return -1; + j = X509_LOOKUP_by_subject_ex(lu, type, name, &stmp, + vs->libctx, vs->propq); + if (j != 0) { /* non-zero value is considered success here */ tmp = &stmp; break; } @@ -340,16 +348,22 @@ int X509_STORE_CTX_get_by_subject(const X509_STORE_CTX *vs, if (tmp == NULL) return 0; } - if (!X509_OBJECT_up_ref_count(tmp)) - return 0; + return -1; ret->type = tmp->type; ret->data.ptr = tmp->data.ptr; - return 1; } +/* Also fill the cache with all matching certificates */ +int X509_STORE_CTX_get_by_subject(const X509_STORE_CTX *vs, + X509_LOOKUP_TYPE type, + const X509_NAME *name, X509_OBJECT *ret) +{ + return ossl_x509_store_ctx_get_by_subject(vs, type, name, ret) > 0; +} + static int x509_store_add(X509_STORE *store, void *x, int crl) { X509_OBJECT *obj; int ret = 0, added = 0; @@ -499,13 +513,13 @@ void X509_OBJECT_free(X509_OBJECT *a) OPENSSL_free(a); } +/* Returns -1 if not found, but also on error */ static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, X509_LOOKUP_TYPE type, const X509_NAME *name, int *pnmatch) { X509_OBJECT stmp; X509 x509_s; X509_CRL crl_s; - int idx; stmp.type = type; switch (type) { @@ -518,12 +532,12 @@ static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, X509_LOOKUP_TYPE type, crl_s.crl.issuer = (X509_NAME *)name; /* won't modify it */ break; case X509_LU_NONE: + default: /* abort(); */ return -1; } - idx = sk_X509_OBJECT_find_all(h, &stmp, pnmatch); - return idx; + return sk_X509_OBJECT_find_all(h, &stmp, pnmatch); } int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, X509_LOOKUP_TYPE type, @@ -536,8 +550,8 @@ X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h, X509_LOOKUP_TYPE type, const X509_NAME *name) { - int idx; - idx = X509_OBJECT_idx_by_subject(h, type, name); + int idx = X509_OBJECT_idx_by_subject(h, type, name); + if (idx == -1) return NULL; return sk_X509_OBJECT_value(h, idx); @@ -581,6 +595,7 @@ STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *store) return NULL; } +/* Returns NULL on internal/fatal error, empty stack if not found */ STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, const X509_NAME *nm) { @@ -591,7 +606,7 @@ STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, X509_STORE *store = ctx->store; if (store == NULL) - return NULL; + return sk_X509_new_null(); if (!X509_STORE_lock(store)) return NULL; @@ -605,24 +620,26 @@ STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, X509_OBJECT *xobj = X509_OBJECT_new(); X509_STORE_unlock(store); - if (xobj == NULL) return NULL; - if (!X509_STORE_CTX_get_by_subject(ctx, X509_LU_X509, nm, xobj)) { + i = ossl_x509_store_ctx_get_by_subject(ctx, X509_LU_X509, nm, xobj); + if (i <= 0) { X509_OBJECT_free(xobj); - return NULL; + return i < 0 ? NULL : sk_X509_new_null(); } X509_OBJECT_free(xobj); if (!X509_STORE_lock(store)) return NULL; idx = x509_object_idx_cnt(store->objs, X509_LU_X509, nm, &cnt); if (idx < 0) { - X509_STORE_unlock(store); - return NULL; + sk = sk_X509_new_null(); + goto end; } } sk = sk_X509_new_null(); + if (sk == NULL) + goto end; for (i = 0; i < cnt; i++, idx++) { obj = sk_X509_OBJECT_value(store->objs, idx); x = obj->data.x509; @@ -632,14 +649,16 @@ STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, return NULL; } } + end: X509_STORE_unlock(store); return sk; } +/* Returns NULL on internal/fatal error, empty stack if not found */ STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(const X509_STORE_CTX *ctx, const X509_NAME *nm) { - int i, idx, cnt; + int i = 1, idx, cnt; STACK_OF(X509_CRL) *sk = sk_X509_CRL_new_null(); X509_CRL *x; X509_OBJECT *obj, *xobj = X509_OBJECT_new(); @@ -647,14 +666,16 @@ STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(const X509_STORE_CTX *ctx, /* Always do lookup to possibly add new CRLs to cache */ if (sk == NULL - || xobj == NULL - || store == NULL - || !X509_STORE_CTX_get_by_subject(ctx, X509_LU_CRL, nm, xobj)) { + || xobj == NULL + || (i = ossl_x509_store_ctx_get_by_subject(ctx, X509_LU_CRL, + nm, xobj)) < 0) { X509_OBJECT_free(xobj); sk_X509_CRL_free(sk); return NULL; } X509_OBJECT_free(xobj); + if (i == 0) + return sk; if (!X509_STORE_lock(store)) { sk_X509_CRL_free(sk); return NULL; @@ -662,8 +683,7 @@ STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(const X509_STORE_CTX *ctx, idx = x509_object_idx_cnt(store->objs, X509_LU_CRL, nm, &cnt); if (idx < 0) { X509_STORE_unlock(store); - sk_X509_CRL_free(sk); - return NULL; + return sk; } for (i = 0; i < cnt; i++, idx++) { @@ -733,10 +753,10 @@ int X509_STORE_CTX_get1_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x) return -1; *issuer = NULL; xn = X509_get_issuer_name(x); - ok = X509_STORE_CTX_get_by_subject(ctx, X509_LU_X509, xn, obj); + ok = ossl_x509_store_ctx_get_by_subject(ctx, X509_LU_X509, xn, obj); if (ok != 1) { X509_OBJECT_free(obj); - return 0; + return ok; } /* If certificate matches and is currently valid all OK */ if (ctx->check_issued(ctx, x, obj->data.x509)) { diff --git a/crypto/x509/x509_trust.c b/crypto/x509/x509_trust.c index 08ea6106873..da29526d277 100644 --- a/crypto/x509/x509_trust.c +++ b/crypto/x509/x509_trust.c @@ -62,6 +62,7 @@ int (*X509_TRUST_set_default(int (*trust) (int, X509 *, int))) (int, X509 *, return oldtrust; } +/* Returns X509_TRUST_TRUSTED, X509_TRUST_REJECTED, or X509_TRUST_UNTRUSTED */ int X509_check_trust(X509 *x, int id, int flags) { X509_TRUST *pt; @@ -253,7 +254,7 @@ static int obj_trust(int id, X509 *x, int flags) X509_CERT_AUX *ax = x->aux; int i; - if (ax && ax->reject) { + if (ax != NULL && ax->reject != NULL) { for (i = 0; i < sk_ASN1_OBJECT_num(ax->reject); i++) { ASN1_OBJECT *obj = sk_ASN1_OBJECT_value(ax->reject, i); int nid = OBJ_obj2nid(obj); @@ -264,7 +265,7 @@ static int obj_trust(int id, X509 *x, int flags) } } - if (ax && ax->trust) { + if (ax != NULL && ax->trust != NULL) { for (i = 0; i < sk_ASN1_OBJECT_num(ax->trust); i++) { ASN1_OBJECT *obj = sk_ASN1_OBJECT_value(ax->trust, i); int nid = OBJ_obj2nid(obj); diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index d25c422d805..df7cb7d5ea8 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -180,6 +180,7 @@ static int verify_cb_crl(X509_STORE_CTX *ctx, int err) return ctx->verify_cb(0, ctx); } +/* Sadly, returns 0 also on internal error in ctx->verify_cb(). */ static int check_auth_level(X509_STORE_CTX *ctx) { int i; @@ -207,7 +208,10 @@ static int check_auth_level(X509_STORE_CTX *ctx) return 1; } -/* Returns -1 on internal error */ +/*- + * Returns -1 on internal error. + * Sadly, returns 0 also on internal error in ctx->verify_cb(). + */ static int verify_chain(X509_STORE_CTX *ctx) { int err; @@ -258,6 +262,10 @@ int X509_STORE_CTX_verify(X509_STORE_CTX *ctx) return X509_verify_cert(ctx); } +/*- + * Returns -1 on internal error. + * Sadly, returns 0 also on internal error in ctx->verify_cb(). + */ int X509_verify_cert(X509_STORE_CTX *ctx) { int ret; @@ -370,7 +378,7 @@ static int get_issuer_sk(X509 **issuer, X509_STORE_CTX *ctx, X509 *x) /*- * Alternative lookup method: look from a STACK stored in other_ctx. - * Returns NULL on internal error (such as out of memory). + * Returns NULL on internal/fatal error, empty stack if not found. */ static STACK_OF(X509) *lookup_certs_sk(X509_STORE_CTX *ctx, const X509_NAME *nm) @@ -397,7 +405,7 @@ static STACK_OF(X509) *lookup_certs_sk(X509_STORE_CTX *ctx, /* * Check EE or CA certificate purpose. For trusted certificates explicit local * auxiliary trust can be used to override EKU-restrictions. - * Sadly, returns 0 also on internal error. + * Sadly, returns 0 also on internal error in ctx->verify_cb(). */ static int check_purpose(X509_STORE_CTX *ctx, X509 *x, int purpose, int depth, int must_be_ca) @@ -430,7 +438,7 @@ static int check_purpose(X509_STORE_CTX *ctx, X509 *x, int purpose, int depth, return 1; case X509_TRUST_REJECTED: break; - default: + default: /* can only be X509_TRUST_UNTRUSTED */ switch (X509_check_purpose(x, purpose, must_be_ca > 0)) { case 1: return 1; @@ -446,9 +454,9 @@ static int check_purpose(X509_STORE_CTX *ctx, X509 *x, int purpose, int depth, return verify_cb_cert(ctx, x, depth, X509_V_ERR_INVALID_PURPOSE); } -/* +/*- * Check extensions of a cert chain for consistency with the supplied purpose. - * Sadly, returns 0 also on internal error. + * Sadly, returns 0 also on internal error in ctx->verify_cb(). */ static int check_extensions(X509_STORE_CTX *ctx) { @@ -644,7 +652,10 @@ static int has_san_id(X509 *x, int gtype) return ret; } -/* Returns -1 on internal error */ +/*- + * Returns -1 on internal error. + * Sadly, returns 0 also on internal error in ctx->verify_cb(). + */ static int check_name_constraints(X509_STORE_CTX *ctx) { int i; @@ -917,7 +928,7 @@ static int check_revocation(X509_STORE_CTX *ctx) last = sk_X509_num(ctx->chain) - 1; } else { /* If checking CRL paths this isn't the EE certificate */ - if (ctx->parent) + if (ctx->parent != NULL) return 1; last = 0; } @@ -1628,6 +1639,7 @@ static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x) return 1; } +/* Sadly, returns 0 also on internal error in ctx->verify_cb(). */ static int check_policy(X509_STORE_CTX *ctx) { int ret; @@ -1703,6 +1715,7 @@ static int check_policy(X509_STORE_CTX *ctx) * the validation status. * * Return 1 on success, 0 otherwise. + * Sadly, returns 0 also on internal error in ctx->verify_cb(). */ int ossl_x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int depth) { @@ -1732,7 +1745,7 @@ int ossl_x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int depth) /* * Verify the issuer signatures and cert times of ctx->chain. - * Sadly, returns 0 also on internal error. + * Sadly, returns 0 also on internal error in ctx->verify_cb(). */ static int internal_verify(X509_STORE_CTX *ctx) { @@ -2897,6 +2910,7 @@ static void dane_reset(SSL_DANE *dane) dane->pdpth = -1; } +/* Sadly, returns 0 also on internal error in ctx->verify_cb(). */ static int check_leaf_suiteb(X509_STORE_CTX *ctx, X509 *cert) { int err = X509_chain_check_suiteb(NULL, cert, NULL, ctx->param->flags); @@ -2984,7 +2998,10 @@ static int get1_trusted_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *cert) return ok; } -/* Returns -1 on internal error */ +/*- + * Returns -1 on internal error. + * Sadly, returns 0 also on internal error in ctx->verify_cb(). + */ static int build_chain(X509_STORE_CTX *ctx) { SSL_DANE *dane = ctx->dane; diff --git a/doc/build.info b/doc/build.info index ea159c6d430..01ae209e505 100644 --- a/doc/build.info +++ b/doc/build.info @@ -2671,6 +2671,10 @@ DEPEND[html/man3/X509_SIG_get0.html]=man3/X509_SIG_get0.pod GENERATE[html/man3/X509_SIG_get0.html]=man3/X509_SIG_get0.pod DEPEND[man/man3/X509_SIG_get0.3]=man3/X509_SIG_get0.pod GENERATE[man/man3/X509_SIG_get0.3]=man3/X509_SIG_get0.pod +DEPEND[html/man3/X509_STORE_CTX_get_by_subject.html]=man3/X509_STORE_CTX_get_by_subject.pod +GENERATE[html/man3/X509_STORE_CTX_get_by_subject.html]=man3/X509_STORE_CTX_get_by_subject.pod +DEPEND[man/man3/X509_STORE_CTX_get_by_subject.3]=man3/X509_STORE_CTX_get_by_subject.pod +GENERATE[man/man3/X509_STORE_CTX_get_by_subject.3]=man3/X509_STORE_CTX_get_by_subject.pod DEPEND[html/man3/X509_STORE_CTX_get_error.html]=man3/X509_STORE_CTX_get_error.pod GENERATE[html/man3/X509_STORE_CTX_get_error.html]=man3/X509_STORE_CTX_get_error.pod DEPEND[man/man3/X509_STORE_CTX_get_error.3]=man3/X509_STORE_CTX_get_error.pod @@ -3399,6 +3403,7 @@ html/man3/X509_NAME_get_index_by_NID.html \ html/man3/X509_NAME_print_ex.html \ html/man3/X509_PUBKEY_new.html \ html/man3/X509_SIG_get0.html \ +html/man3/X509_STORE_CTX_get_by_subject.html \ html/man3/X509_STORE_CTX_get_error.html \ html/man3/X509_STORE_CTX_new.html \ html/man3/X509_STORE_CTX_set_verify_cb.html \ @@ -3994,6 +3999,7 @@ man/man3/X509_NAME_get_index_by_NID.3 \ man/man3/X509_NAME_print_ex.3 \ man/man3/X509_PUBKEY_new.3 \ man/man3/X509_SIG_get0.3 \ +man/man3/X509_STORE_CTX_get_by_subject.3 \ man/man3/X509_STORE_CTX_get_error.3 \ man/man3/X509_STORE_CTX_new.3 \ man/man3/X509_STORE_CTX_set_verify_cb.3 \ diff --git a/doc/man3/X509_LOOKUP.pod b/doc/man3/X509_LOOKUP.pod index 4d2fe38f25a..f888d28467f 100644 --- a/doc/man3/X509_LOOKUP.pod +++ b/doc/man3/X509_LOOKUP.pod @@ -91,7 +91,8 @@ associates and retrieves a pointer to application data to and from the given B, respectively. X509_LOOKUP_ctrl_ex() is used to set or get additional data to or from -a B structure or its associated L. +a B structure using any control function in the +associated L. The arguments of the control command are passed via I and I, its return value via I<*ret>. The library context I and property query I are used when fetching algorithms from providers. @@ -195,21 +196,29 @@ or NULL on error. X509_LOOKUP_init() and X509_LOOKUP_shutdown() return 1 on success, or 0 on error. -X509_LOOKUP_ctrl() returns -1 if the B doesn't have an +X509_LOOKUP_ctrl_ex() and X509_LOOKUP_ctrl() +return -1 if the B doesn't have an associated B, or 1 if the X<509_LOOKUP_METHOD> doesn't have a control function. Otherwise, it returns what the control function in the -B returns, which is usually 1 on success and 0 in -error. +B returns, which is usually 1 on success and 0 on error +but could also be -1 on failure. X509_LOOKUP_get_store() returns a B pointer if there is one, otherwise NULL. -X509_LOOKUP_by_subject_ex(), X509_LOOKUP_by_subject(), +X509_LOOKUP_by_subject_ex() returns 0 if there is no B +that implements any of the get_by_subject_ex() or get_by_subject() functions. +It calls get_by_subject_ex() if present, otherwise get_by_subject(), and returns +the result of the function, which is usually 1 on success and 0 on error. + +X509_LOOKUP_by_subject() is similar to X509_LOOKUP_by_subject_ex() +but passes NULL for both the libctx and propq. + X509_LOOKUP_by_issuer_serial(), X509_LOOKUP_by_fingerprint(), and X509_LOOKUP_by_alias() all return 0 if there is no B or that method doesn't implement the corresponding function. -Otherwise, it returns what the corresponding function in the +Otherwise, they return what the corresponding function in the B returns, which is usually 1 on success and 0 in error. diff --git a/doc/man3/X509_LOOKUP_meth_new.pod b/doc/man3/X509_LOOKUP_meth_new.pod index 20217499356..49776e71260 100644 --- a/doc/man3/X509_LOOKUP_meth_new.pod +++ b/doc/man3/X509_LOOKUP_meth_new.pod @@ -149,7 +149,7 @@ object. Implementations must add objects they find to the B object using X509_STORE_add_cert() or X509_STORE_add_crl(). This increments -its reference count. However, the X509_STORE_CTX_get_by_subject() +its reference count. However, the L function also increases the reference count which leads to one too many references being held. Therefore, applications should additionally call X509_free() or X509_CRL_free() to decrement the @@ -178,6 +178,7 @@ pointers. =head1 SEE ALSO +L, L, L =head1 HISTORY diff --git a/doc/man3/X509_STORE_CTX_get_by_subject.pod b/doc/man3/X509_STORE_CTX_get_by_subject.pod new file mode 100644 index 00000000000..a08f52592c4 --- /dev/null +++ b/doc/man3/X509_STORE_CTX_get_by_subject.pod @@ -0,0 +1,51 @@ +=pod + +=head1 NAME + +X509_STORE_CTX_get_by_subject, +X509_STORE_CTX_get_obj_by_subject +- X509 and X509_CRL lookup functions + +=head1 SYNOPSIS + + #include + + int X509_STORE_CTX_get_by_subject(const X509_STORE_CTX *vs, + X509_LOOKUP_TYPE type, + const X509_NAME *name, X509_OBJECT *ret); + X509_OBJECT *X509_STORE_CTX_get_obj_by_subject(X509_STORE_CTX *vs, + X509_LOOKUP_TYPE type, + const X509_NAME *name); + +=head1 DESCRIPTION + +X509_STORE_CTX_get_by_subject() tries to find an object +of given I, which may be B or B, +and subject I from the store in the provided store context I. +If found and I is not NULL, it increments the reference count and +stores the looked up object in I. + +X509_STORE_CTX_get_obj_by_subject() is like X509_STORE_CTX_get_by_subject() +but returns the found object on success, else NULL. + +=head1 RETURN VALUES + +X509_STORE_CTX_get_by_subject() returns 1 if the lookup was successful, else 0. + +X509_STORE_CTX_get_obj_by_subject() returns an object on success, else NULL. + +=head1 SEE ALSO + +L, +L + +=head1 COPYRIGHT + +Copyright 2022 The OpenSSL Project Authors. All Rights Reserved. + +Licensed under the Apache License 2.0 (the "License"). You may not use +this file except in compliance with the License. You can obtain a copy +in the file LICENSE in the source distribution or at +L. + +=cut diff --git a/util/missingcrypto.txt b/util/missingcrypto.txt index 4d2fd7f6b71..78a37e7f5a0 100644 --- a/util/missingcrypto.txt +++ b/util/missingcrypto.txt @@ -1273,9 +1273,7 @@ X509_STORE_CTX_get0_policy_tree(3) X509_STORE_CTX_get0_store(3) X509_STORE_CTX_get1_certs(3) X509_STORE_CTX_get1_crls(3) -X509_STORE_CTX_get_by_subject(3) X509_STORE_CTX_get_explicit_policy(3) -X509_STORE_CTX_get_obj_by_subject(3) X509_STORE_CTX_set0_dane(3) X509_STORE_CTX_set_depth(3) X509_STORE_CTX_set_flags(3)