Forbid unusual return values out of verify_cb
If a verify_cb consistently returns -1, it would broadly get treated as
success, except the final call would leak into ok and come out of
X509_verify_cert and read as failure. Prevent this ambiguity by
requiring the return value be 0 or 1, and aborting otherwise.
Update-Note: If the verify callback returns anything other than 0 or 1,
X509_verify_cert will now crash in BSSL_CHECK. If this happens, fix the
callback to use the correct return value.
Change-Id: I0394e68febe9f22a42bcd5de8ea4f3a82b07c862
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65107
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 7e9aca4..35fa3e1 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -134,8 +134,18 @@
return 1;
}
-// Given a certificate try and find an exact match in the store
+static int call_verify_cb(int ok, X509_STORE_CTX *ctx) {
+ ok = ctx->verify_cb(ok, ctx);
+ // Historically, callbacks returning values like -1 would be treated as a mix
+ // of success or failure. Insert that callers check correctly.
+ //
+ // TODO(davidben): Also use this wrapper to constrain which errors may be
+ // suppressed, and ensure all |verify_cb| calls remember to fill in an error.
+ BSSL_CHECK(ok == 0 || ok == 1);
+ return ok;
+}
+// Given a certificate try and find an exact match in the store
static X509 *lookup_cert_match(X509_STORE_CTX *ctx, X509 *x) {
STACK_OF(X509) *certs;
X509 *xtmp = NULL;
@@ -293,7 +303,7 @@
ctx->current_cert = x;
ctx->error_depth = i - 1;
bad_chain = 1;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
goto end;
}
@@ -404,7 +414,7 @@
ctx->error_depth = num - 1;
bad_chain = 1;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
goto end;
}
@@ -492,7 +502,7 @@
ctx->error = ret;
ctx->current_cert = x;
- return ctx->verify_cb(0, ctx);
+ return call_verify_cb(0, ctx);
}
static X509 *get_trusted_issuer(X509_STORE_CTX *ctx, X509 *x) {
@@ -527,7 +537,7 @@
ctx->error = X509_V_ERR_UNHANDLED_CRITICAL_EXTENSION;
ctx->error_depth = i;
ctx->current_cert = x;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
goto end;
}
@@ -538,7 +548,7 @@
ctx->error = X509_V_ERR_INVALID_CA;
ctx->error_depth = i;
ctx->current_cert = x;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
goto end;
}
@@ -548,7 +558,7 @@
ctx->error = X509_V_ERR_INVALID_PURPOSE;
ctx->error_depth = i;
ctx->current_cert = x;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
goto end;
}
@@ -559,7 +569,7 @@
ctx->error = X509_V_ERR_PATH_LENGTH_EXCEEDED;
ctx->error_depth = i;
ctx->current_cert = x;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
goto end;
}
@@ -629,7 +639,7 @@
ctx->error = rv;
ctx->error_depth = i;
ctx->current_cert = x;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
break;
@@ -661,7 +671,7 @@
ctx->error = rv;
ctx->error_depth = i;
ctx->current_cert = leaf;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
break;
@@ -675,7 +685,7 @@
ctx->error = errcode;
ctx->current_cert = ctx->cert;
ctx->error_depth = 0;
- return ctx->verify_cb(0, ctx);
+ return call_verify_cb(0, ctx);
}
static int check_hosts(X509 *x, X509_VERIFY_PARAM *param) {
@@ -735,7 +745,7 @@
ctx->error_depth = (int)i;
ctx->current_cert = x;
ctx->error = X509_V_ERR_CERT_REJECTED;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
return X509_TRUST_REJECTED;
}
@@ -801,7 +811,7 @@
// If error looking up CRL, nothing we can do except notify callback
if (!ok) {
ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
goto err;
}
ctx->current_crl = crl;
@@ -843,7 +853,7 @@
return 0;
}
ctx->error = X509_V_ERR_ERROR_IN_CRL_LAST_UPDATE_FIELD;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -853,7 +863,7 @@
return 0;
}
ctx->error = X509_V_ERR_CRL_NOT_YET_VALID;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -866,7 +876,7 @@
return 0;
}
ctx->error = X509_V_ERR_ERROR_IN_CRL_NEXT_UPDATE_FIELD;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -875,7 +885,7 @@
return 0;
}
ctx->error = X509_V_ERR_CRL_HAS_EXPIRED;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -1175,7 +1185,7 @@
// If not self signed, can't check signature
if (!x509_check_issued_with_callback(ctx, issuer, issuer)) {
ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL_ISSUER;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -1186,21 +1196,21 @@
if ((issuer->ex_flags & EXFLAG_KUSAGE) &&
!(issuer->ex_kusage & X509v3_KU_CRL_SIGN)) {
ctx->error = X509_V_ERR_KEYUSAGE_NO_CRL_SIGN;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
if (!(ctx->current_crl_score & CRL_SCORE_SCOPE)) {
ctx->error = X509_V_ERR_DIFFERENT_CRL_SCOPE;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
if (crl->idp_flags & IDP_INVALID) {
ctx->error = X509_V_ERR_INVALID_EXTENSION;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -1215,14 +1225,14 @@
EVP_PKEY *ikey = X509_get0_pubkey(issuer);
if (!ikey) {
ctx->error = X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
} else {
// Verify CRL signature
if (X509_CRL_verify(crl, ikey) <= 0) {
ctx->error = X509_V_ERR_CRL_SIGNATURE_FAILURE;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -1243,7 +1253,7 @@
if (!(ctx->param->flags & X509_V_FLAG_IGNORE_CRITICAL) &&
(crl->flags & EXFLAG_CRITICAL)) {
ctx->error = X509_V_ERR_UNHANDLED_CRITICAL_CRL_EXTENSION;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
return 0;
}
@@ -1251,7 +1261,7 @@
// Look for serial number of certificate in CRL.
if (X509_CRL_get0_by_cert(crl, &rev, x)) {
ctx->error = X509_V_ERR_CERT_REVOKED;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
return 0;
}
@@ -1270,7 +1280,7 @@
if (ret == X509_V_ERR_OUT_OF_MEM) {
return 0;
}
- return ctx->verify_cb(0, ctx);
+ return call_verify_cb(0, ctx);
}
return 1;
@@ -1292,7 +1302,7 @@
if (i == 0) {
ctx->error = X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD;
ctx->current_cert = x;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -1300,7 +1310,7 @@
if (i > 0) {
ctx->error = X509_V_ERR_CERT_NOT_YET_VALID;
ctx->current_cert = x;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -1309,7 +1319,7 @@
if (i == 0) {
ctx->error = X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD;
ctx->current_cert = x;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -1317,7 +1327,7 @@
if (i < 0) {
ctx->error = X509_V_ERR_CERT_HAS_EXPIRED;
ctx->current_cert = x;
- if (!ctx->verify_cb(0, ctx)) {
+ if (!call_verify_cb(0, ctx)) {
return 0;
}
}
@@ -1344,7 +1354,7 @@
if (n <= 0) {
ctx->error = X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE;
ctx->current_cert = xi;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
goto end;
} else {
n--;
@@ -1365,14 +1375,14 @@
if (pkey == NULL) {
ctx->error = X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY;
ctx->current_cert = xi;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
goto end;
}
} else if (X509_verify(xs, pkey) <= 0) {
ctx->error = X509_V_ERR_CERT_SIGNATURE_FAILURE;
ctx->current_cert = xs;
- ok = ctx->verify_cb(0, ctx);
+ ok = call_verify_cb(0, ctx);
if (!ok) {
goto end;
}
@@ -1387,7 +1397,7 @@
// The last error (if any) is still in the error value
ctx->current_cert = xs;
- ok = ctx->verify_cb(1, ctx);
+ ok = call_verify_cb(1, ctx);
if (!ok) {
goto end;
}
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 893bf42..d3f4493 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -3903,11 +3903,11 @@
// X509_STORE_CTX_set_verify_cb configures a callback function for |ctx| that is
// called multiple times during |X509_verify_cert|. The callback returns zero to
-// fail verification and non-zero to proceed. Typically, it will return |ok|,
-// which 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.
+// fail verification and one to proceed. Typically, it will return |ok|, which
+// 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.
//
// WARNING: Do not use this function. It is extremely fragile and unpredictable.
// This callback exposes implementation details of certificate verification,
