X509{,_LOOKUP}: Improve distinction between not found and fatal/internal error
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from https://github.com/openssl/openssl/pull/14417)
diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c
index d02777f..278fbe5 100644
--- a/crypto/x509/x509_lu.c
+++ b/crypto/x509/x509_lu.c
@@ -305,10 +305,15 @@
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 @@
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 @@
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 @@
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 @@
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_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 @@
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 @@
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 @@
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 @@
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 @@
/* 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 @@
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 @@
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 08ea610..da29526 100644
--- a/crypto/x509/x509_trust.c
+++ b/crypto/x509/x509_trust.c
@@ -62,6 +62,7 @@
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 @@
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 @@
}
}
- 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 d25c422..df7cb7d 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -180,6 +180,7 @@
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 @@
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 @@
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 @@
/*-
* 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 @@
/*
* 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
* 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 @@
/*
* 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 @@
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 @@
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 ea159c6..01ae209 100644
--- a/doc/build.info
+++ b/doc/build.info
@@ -2671,6 +2671,10 @@
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_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_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 4d2fe38..f888d28 100644
--- a/doc/man3/X509_LOOKUP.pod
+++ b/doc/man3/X509_LOOKUP.pod
@@ -91,7 +91,8 @@
given B<X509_LOOKUP>, respectively.
X509_LOOKUP_ctrl_ex() is used to set or get additional data to or from
-a B<X509_LOOKUP> structure or its associated L<X509_LOOKUP_METHOD(3)>.
+a B<X509_LOOKUP> structure using any control function in the
+associated L<X509_LOOKUP_METHOD(3)>.
The arguments of the control command are passed via I<argc> and I<argl>,
its return value via I<*ret>. The library context I<libctx> and property
query I<propq> are used when fetching algorithms from providers.
@@ -195,21 +196,29 @@
X509_LOOKUP_init() and X509_LOOKUP_shutdown() return 1 on success, or
0 on error.
-X509_LOOKUP_ctrl() returns -1 if the B<X509_LOOKUP> doesn't have an
+X509_LOOKUP_ctrl_ex() and X509_LOOKUP_ctrl()
+return -1 if the B<X509_LOOKUP> doesn't have an
associated B<X509_LOOKUP_METHOD>, or 1 if the X<509_LOOKUP_METHOD>
doesn't have a control function.
Otherwise, it returns what the control function in the
-B<X509_LOOKUP_METHOD> returns, which is usually 1 on success and 0 in
-error.
+B<X509_LOOKUP_METHOD> 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<X509_STORE> 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<X509_LOOKUP_METHOD>
+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<X509_LOOKUP_METHOD> 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<X509_LOOKUP_METHOD> 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 2021749..49776e7 100644
--- a/doc/man3/X509_LOOKUP_meth_new.pod
+++ b/doc/man3/X509_LOOKUP_meth_new.pod
@@ -149,7 +149,7 @@
Implementations must add objects they find to the B<X509_STORE> 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<X509_STORE_CTX_get_by_subject(3)>
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 @@
=head1 SEE ALSO
+L<X509_STORE_CTX_get_by_subject(3)>,
L<X509_STORE_new(3)>, L<SSL_CTX_set_cert_store(3)>
=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 0000000..a08f525
--- /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 <openssl/x509_vfy.h>
+
+ 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<type>, which may be B<X509_LU_X509> or B<X509_LU_CRL>,
+and subject I<name> from the store in the provided store context I<vs>.
+If found and I<ret> is not NULL, it increments the reference count and
+stores the looked up object in I<ret>.
+
+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<X509_LOOKUP_meth_set_get_by_subject(3)>,
+L<X509_LOOKUP_by_subject(3)>
+
+=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<https://www.openssl.org/source/license.html>.
+
+=cut
diff --git a/util/missingcrypto.txt b/util/missingcrypto.txt
index 4d2fd7f..78a37e7 100644
--- a/util/missingcrypto.txt
+++ b/util/missingcrypto.txt
@@ -1273,9 +1273,7 @@
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)
