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,