Fix error-handling convention in x509_vfy.c and avoid -1 returns
This CL makes two changes. First, it removes the couple of places where
X509_verify_cert may return -1 and switches to our standard 0/1 return
convention. The only -1 cases were get_issuer returning < 0 and the
caller error cases at the top. It seems implausible that any caller
would care about the latter and the former is actually impossible.
get_issuer never returns < 0.
Second, OpenSSL's original implementation did not follow the usual
error-handling convention. The usual convention is that there's a
cleanup epilog, and a variable (usually called 'ret' or 'ok') that
stores the return value. This variable is initialized in the failure
case and may only be modified immediately before a goto or when falling
through to the epilog. This allows error conditions to simply 'goto err'
and rely on the variable's value.
X509_verify_cert instead overwrite 'ok' throughout the function, which
is tedious and error-prone. Fix this to follow the usual convention.
Also remove uses of this pattern when there isn't anything to cleanup.
As part of this cleanup, we fix a near miss: the three cert_self_signed
call sites did not correctly account for this non-standard pattern.
Fortunately (as demonstrated by existing unit tests), the first call
site is fine. The remainder are only called on "trusted" certificates
from the X509_STORE. An attacker with control over trust anchors already
controls certificate verification, so this is moot. Moreover, all such
certificates first go through get_issuer, which calls X509_check_issued,
which already handles EXFLAG_INVALID, so the error condition was
redundant.
Update-Note: X509_verify_cert no longer returns -1 on some error
conditions, only zero.
Change-Id: I88d5e845cd4cb8f48d5c5df6782bf6730c682642
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65067
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 35fa3e1..f64cfeb 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -182,7 +182,7 @@
if (ctx->cert == NULL) {
OPENSSL_PUT_ERROR(X509, X509_R_NO_CERT_SET_FOR_US_TO_VERIFY);
ctx->error = X509_V_ERR_INVALID_CALL;
- return -1;
+ return 0;
}
if (ctx->chain != NULL) {
@@ -190,7 +190,7 @@
// cannot do another one.
OPENSSL_PUT_ERROR(X509, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
ctx->error = X509_V_ERR_INVALID_CALL;
- return -1;
+ return 0;
}
if (ctx->param->flags &
@@ -200,7 +200,7 @@
// inadvertently skip a CRL check that the caller expects, fail closed.
OPENSSL_PUT_ERROR(X509, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
ctx->error = X509_V_ERR_INVALID_CALL;
- return -1;
+ return 0;
}
// first we make sure the chain we are going to build is present and that
@@ -258,7 +258,6 @@
if (issuer != NULL) {
if (!sk_X509_push(ctx->chain, issuer)) {
ctx->error = X509_V_ERR_OUT_OF_MEM;
- ok = 0;
goto end;
}
X509_up_ref(issuer);
@@ -303,8 +302,7 @@
ctx->current_cert = x;
ctx->error_depth = i - 1;
bad_chain = 1;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
+ if (!call_verify_cb(0, ctx)) {
goto end;
}
} else {
@@ -347,7 +345,6 @@
if (!sk_X509_push(ctx->chain, x)) {
X509_free(issuer);
ctx->error = X509_V_ERR_OUT_OF_MEM;
- ok = 0;
goto end;
}
num++;
@@ -358,7 +355,6 @@
// If explicitly rejected error
if (trust == X509_TRUST_REJECTED) {
- ok = 0;
goto end;
}
// If it's not explicitly trusted then check if there is an alternative
@@ -414,59 +410,33 @@
ctx->error_depth = num - 1;
bad_chain = 1;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
+ if (!call_verify_cb(0, ctx)) {
goto end;
}
}
// We have the chain complete: now we need to check its purpose
- ok = check_chain_extensions(ctx);
-
- if (!ok) {
+ if (!check_chain_extensions(ctx) || //
+ !check_id(ctx) ||
+ // We check revocation status after copying parameters because they may be
+ // needed for CRL signature verification.
+ !check_revocation(ctx) || //
+ !internal_verify(ctx) || //
+ !check_name_constraints(ctx) ||
+ // TODO(davidben): Does |check_policy| still need to be conditioned on
+ // |!bad_chain|? DoS concerns have been resolved.
+ (!bad_chain && !check_policy(ctx))) {
goto end;
}
- ok = check_id(ctx);
-
- if (!ok) {
- goto end;
- }
-
- // Check revocation status: we do this after copying parameters because
- // they may be needed for CRL signature verification.
- ok = check_revocation(ctx);
- if (!ok) {
- goto end;
- }
-
- // At this point, we have a chain and need to verify it
- ok = internal_verify(ctx);
- if (!ok) {
- goto end;
- }
-
- // Check name constraints
- ok = check_name_constraints(ctx);
- if (!ok) {
- goto end;
- }
-
- // If we get this far, evaluate policies.
- if (!bad_chain) {
- ok = check_policy(ctx);
- }
+ ok = 1;
end:
- if (sktmp != NULL) {
- sk_X509_free(sktmp);
- }
- if (chain_ss != NULL) {
- X509_free(chain_ss);
- }
+ sk_X509_free(sktmp);
+ X509_free(chain_ss);
// Safety net, error returns must set ctx->error
- if (ok <= 0 && ctx->error == X509_V_OK) {
+ if (!ok && ctx->error == X509_V_OK) {
ctx->error = X509_V_ERR_UNSPECIFIED;
}
return ok;
@@ -526,7 +496,7 @@
// purpose
static int check_chain_extensions(X509_STORE_CTX *ctx) {
- int ok = 0, plen = 0;
+ int plen = 0;
int purpose = ctx->param->purpose;
// Check all untrusted certificates
@@ -537,9 +507,8 @@
ctx->error = X509_V_ERR_UNHANDLED_CRITICAL_EXTENSION;
ctx->error_depth = i;
ctx->current_cert = x;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
- goto end;
+ if (!call_verify_cb(0, ctx)) {
+ return 0;
}
}
@@ -548,9 +517,8 @@
ctx->error = X509_V_ERR_INVALID_CA;
ctx->error_depth = i;
ctx->current_cert = x;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
- goto end;
+ if (!call_verify_cb(0, ctx)) {
+ return 0;
}
}
if (ctx->param->purpose > 0 &&
@@ -558,9 +526,8 @@
ctx->error = X509_V_ERR_INVALID_PURPOSE;
ctx->error_depth = i;
ctx->current_cert = x;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
- goto end;
+ if (!call_verify_cb(0, ctx)) {
+ return 0;
}
}
// Check pathlen if not self issued
@@ -569,9 +536,8 @@
ctx->error = X509_V_ERR_PATH_LENGTH_EXCEEDED;
ctx->error_depth = i;
ctx->current_cert = x;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
- goto end;
+ if (!call_verify_cb(0, ctx)) {
+ return 0;
}
}
// Increment path length if not self issued
@@ -579,9 +545,8 @@
plen++;
}
}
- ok = 1;
-end:
- return ok;
+
+ return 1;
}
static int reject_dns_name_in_common_name(X509 *x509) {
@@ -729,24 +694,22 @@
}
static int check_trust(X509_STORE_CTX *ctx) {
- int ok;
X509 *x = NULL;
// Check all trusted certificates in chain
for (size_t i = ctx->last_untrusted; i < sk_X509_num(ctx->chain); i++) {
x = sk_X509_value(ctx->chain, i);
- ok = X509_check_trust(x, ctx->param->trust, 0);
+ int trust = X509_check_trust(x, ctx->param->trust, 0);
// If explicitly trusted return trusted
- if (ok == X509_TRUST_TRUSTED) {
+ if (trust == X509_TRUST_TRUSTED) {
return X509_TRUST_TRUSTED;
}
// If explicitly rejected notify callback and reject if not
// overridden.
- if (ok == X509_TRUST_REJECTED) {
+ if (trust == X509_TRUST_REJECTED) {
ctx->error_depth = (int)i;
ctx->current_cert = x;
ctx->error = X509_V_ERR_CERT_REJECTED;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
+ if (!call_verify_cb(0, ctx)) {
return X509_TRUST_REJECTED;
}
}
@@ -785,9 +748,8 @@
}
for (int i = 0; i <= last; i++) {
ctx->error_depth = i;
- int ok = check_cert(ctx);
- if (!ok) {
- return ok;
+ if (!check_cert(ctx)) {
+ return 0;
}
}
return 1;
@@ -807,23 +769,19 @@
// TODO(davidben): Remove these callbacks. gRPC currently sets them, but
// implements them incorrectly. It is not actually possible to implement
// |get_crl| from outside the library.
- ok = ctx->get_crl(ctx, &crl, x);
- // If error looking up CRL, nothing we can do except notify callback
- if (!ok) {
+ if (!ctx->get_crl(ctx, &crl, x)) {
ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL;
ok = call_verify_cb(0, ctx);
goto err;
}
+
ctx->current_crl = crl;
- ok = ctx->check_crl(ctx, crl);
- if (!ok) {
+ if (!ctx->check_crl(ctx, crl) || //
+ !cert_crl(ctx, crl, x)) {
goto err;
}
- ok = cert_crl(ctx, crl, x);
- if (!ok) {
- goto err;
- }
+ ok = 1;
err:
X509_CRL_free(crl);
@@ -1133,19 +1091,16 @@
// Retrieve CRL corresponding to current certificate.
static int get_crl(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 *x) {
- int ok;
X509 *issuer = NULL;
int crl_score = 0;
X509_CRL *crl = NULL;
- STACK_OF(X509_CRL) *skcrl;
- X509_NAME *nm = X509_get_issuer_name(x);
- ok = get_crl_sk(ctx, &crl, &issuer, &crl_score, ctx->crls);
- if (ok) {
+ if (get_crl_sk(ctx, &crl, &issuer, &crl_score, ctx->crls)) {
goto done;
}
// Lookup CRLs from store
- skcrl = X509_STORE_CTX_get1_crls(ctx, nm);
+ STACK_OF(X509_CRL) *skcrl =
+ X509_STORE_CTX_get1_crls(ctx, X509_get_issuer_name(x));
// If no CRLs found and a near match from get_crl_sk use that
if (!skcrl && crl) {
@@ -1244,8 +1199,6 @@
// Check certificate against CRL
static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x) {
- int ok;
- X509_REVOKED *rev;
// The rules changed for this... previously if a CRL contained unhandled
// critical extensions it could still be used to indicate a certificate
// was revoked. This has since been changed since critical extension can
@@ -1253,16 +1206,15 @@
if (!(ctx->param->flags & X509_V_FLAG_IGNORE_CRITICAL) &&
(crl->flags & EXFLAG_CRITICAL)) {
ctx->error = X509_V_ERR_UNHANDLED_CRITICAL_CRL_EXTENSION;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
// Look for serial number of certificate in CRL.
+ X509_REVOKED *rev;
if (X509_CRL_get0_by_cert(crl, &rev, x)) {
ctx->error = X509_V_ERR_CERT_REVOKED;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -1336,14 +1288,19 @@
}
static int internal_verify(X509_STORE_CTX *ctx) {
- int ok = 0;
- X509 *xs, *xi;
-
+ // TODO(davidben): This logic is incredibly confusing. Rewrite this:
+ //
+ // First, don't allow the verify callback to suppress
+ // X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY, which will simplify the
+ // signature check. Then replace jumping into the middle of the loop. It's
+ // trying to ensure that all certificates see |check_cert_time|, then checking
+ // the root's self signature when requested, but not breaking partial chains
+ // in the process.
int n = (int)sk_X509_num(ctx->chain);
ctx->error_depth = n - 1;
n--;
- xi = sk_X509_value(ctx->chain, n);
-
+ X509 *xi = sk_X509_value(ctx->chain, n);
+ X509 *xs;
if (x509_check_issued_with_callback(ctx, xi, xi)) {
xs = xi;
} else {
@@ -1354,13 +1311,11 @@
if (n <= 0) {
ctx->error = X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE;
ctx->current_cert = xi;
- ok = call_verify_cb(0, ctx);
- goto end;
- } else {
- n--;
- ctx->error_depth = n;
- xs = sk_X509_value(ctx->chain, n);
+ return call_verify_cb(0, ctx);
}
+ n--;
+ ctx->error_depth = n;
+ xs = sk_X509_value(ctx->chain, n);
}
// ctx->error=0; not needed
@@ -1375,31 +1330,27 @@
if (pkey == NULL) {
ctx->error = X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY;
ctx->current_cert = xi;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
- goto end;
+ if (!call_verify_cb(0, ctx)) {
+ return 0;
}
} else if (X509_verify(xs, pkey) <= 0) {
ctx->error = X509_V_ERR_CERT_SIGNATURE_FAILURE;
ctx->current_cert = xs;
- ok = call_verify_cb(0, ctx);
- if (!ok) {
- goto end;
+ if (!call_verify_cb(0, ctx)) {
+ return 0;
}
}
}
check_cert:
- ok = check_cert_time(ctx, xs);
- if (!ok) {
- goto end;
+ if (!check_cert_time(ctx, xs)) {
+ return 0;
}
// The last error (if any) is still in the error value
ctx->current_cert = xs;
- ok = call_verify_cb(1, ctx);
- if (!ok) {
- goto end;
+ if (!call_verify_cb(1, ctx)) {
+ return 0;
}
n--;
@@ -1408,9 +1359,8 @@
xs = sk_X509_value(ctx->chain, n);
}
}
- ok = 1;
-end:
- return ok;
+
+ return 1;
}
int X509_cmp_current_time(const ASN1_TIME *ctm) {
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index d3f4493..0f5a222 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -3517,6 +3517,12 @@
OPENSSL_EXPORT int X509_CRL_match(const X509_CRL *a, const X509_CRL *b);
+// X509_verify_cert performs certifice verification with |ctx|, which must have
+// been initialized with |X509_STORE_CTX_init|. It returns one on success and
+// zero on error. On success, |X509_STORE_CTX_get0_chain| or
+// |X509_STORE_CTX_get1_chain| may be used to return the verified certificate
+// chain. On error, |X509_STORE_CTX_get_error| may be used to return additional
+// error information.
OPENSSL_EXPORT int X509_verify_cert(X509_STORE_CTX *ctx);
OPENSSL_EXPORT int X509_check_trust(X509 *x, int id, int flags);
@@ -3863,14 +3869,52 @@
OPENSSL_EXPORT int X509_STORE_load_locations(X509_STORE *ctx, const char *file,
const char *dir);
OPENSSL_EXPORT int X509_STORE_set_default_paths(X509_STORE *ctx);
+
+// X509_STORE_CTX_get_error, after |X509_verify_cert| returns, returns
+// |X509_V_OK| if verification succeeded or an |X509_V_ERR_*| describing why
+// verification failed. This will be consistent with |X509_verify_cert|'s return
+// value, unless the caller used the deprecated verification callback (see
+// |X509_STORE_CTX_set_verify_cb|) in a way that breaks |ctx|'s invariants.
+//
+// If called during the deprecated verification callback when |ok| is zero, it
+// returns the current error under consideration.
OPENSSL_EXPORT int X509_STORE_CTX_get_error(X509_STORE_CTX *ctx);
-OPENSSL_EXPORT void X509_STORE_CTX_set_error(X509_STORE_CTX *ctx, int s);
+
+// X509_STORE_CTX_set_error sets |ctx|'s error to |err|, which should be
+// |X509_V_OK| or an |X509_V_ERR_*| constant. It is not expected to be called in
+// typical |X509_STORE_CTX| usage, but may be used in callback APIs where
+// applications synthesize |X509_STORE_CTX| error conditions. See also
+// |X509_STORE_CTX_set_verify_cb| and |SSL_CTX_set_cert_verify_callback|.
+OPENSSL_EXPORT void X509_STORE_CTX_set_error(X509_STORE_CTX *ctx, int err);
+
+// X509_STORE_CTX_get_error_depth returns the depth at which the error returned
+// by |X509_STORE_CTX_get_error| occured. This is zero-indexed integer into the
+// certificate chain. Zero indicates the target certificate, one its issuer, and
+// so on.
OPENSSL_EXPORT int X509_STORE_CTX_get_error_depth(X509_STORE_CTX *ctx);
+
OPENSSL_EXPORT X509 *X509_STORE_CTX_get_current_cert(X509_STORE_CTX *ctx);
OPENSSL_EXPORT X509_CRL *X509_STORE_CTX_get0_current_crl(X509_STORE_CTX *ctx);
+
+// X509_STORE_CTX_get_chain is a legacy alias for |X509_STORE_CTX_get0_chain|.
OPENSSL_EXPORT STACK_OF(X509) *X509_STORE_CTX_get_chain(X509_STORE_CTX *ctx);
+
+// X509_STORE_CTX_get0_chain, after a successful |X509_verify_cert| call,
+// returns the verified certificate chain. The chain begins with the leaf and
+// ends with trust anchor.
+//
+// At other points, such as after a failed verification or during the deprecated
+// verification callback, it returns the partial chain built so far. Callers
+// should avoid relying on this as this exposes unstable library implementation
+// details.
OPENSSL_EXPORT STACK_OF(X509) *X509_STORE_CTX_get0_chain(X509_STORE_CTX *ctx);
+
+// X509_STORE_CTX_get1_chain behaves like |X509_STORE_CTX_get0_chain| but
+// returns a newly-allocated |STACK_OF(X509)| containing the completed chain,
+// with each certificate's reference count incremented. Callers must free the
+// result with |sk_X509_pop_free| and |X509_free| when done.
OPENSSL_EXPORT STACK_OF(X509) *X509_STORE_CTX_get1_chain(X509_STORE_CTX *ctx);
+
OPENSSL_EXPORT void X509_STORE_CTX_set_cert(X509_STORE_CTX *c, X509 *x);
OPENSSL_EXPORT void X509_STORE_CTX_set_chain(X509_STORE_CTX *c,
STACK_OF(X509) *sk);
@@ -3907,7 +3951,8 @@
// preserves the default behavior. Returning one when |ok| is zero will proceed
// past some error. The callback may inspect |ctx| and the error queue to
// attempt to determine the current stage of certificate verification, but this
-// is often unreliable.
+// is often unreliable. When synthesizing an error, callbacks should use
+// |X509_STORE_CTX_set_error| to set a corresponding error.
//
// WARNING: Do not use this function. It is extremely fragile and unpredictable.
// This callback exposes implementation details of certificate verification,
