From 7a813621dac6878ab53b6ed7392939a8982226e8 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 3 Jan 2024 16:57:00 -0500 Subject: [PATCH] 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 Reviewed-by: Bob Beck --- crypto/x509/x509_vfy.c | 78 ++++++++++++++++++++++++------------------ include/openssl/x509.h | 10 +++--- 2 files changed, 49 insertions(+), 39 deletions(-) diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 7e9aca418..35fa3e1c3 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -134,8 +134,18 @@ static int cert_self_signed(X509 *x, int *out_is_self_signed) { 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 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { 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 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { 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 @@ int x509_check_issued_with_callback(X509_STORE_CTX *ctx, X509 *x, 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 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) { 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 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) { 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 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) { 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 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) { 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 @@ static int check_name_constraints(X509_STORE_CTX *ctx) { 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 @@ static int check_name_constraints(X509_STORE_CTX *ctx) { 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 @@ static int check_id_error(X509_STORE_CTX *ctx, int errcode) { 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 @@ static int check_trust(X509_STORE_CTX *ctx) { 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 @@ static int check_cert(X509_STORE_CTX *ctx) { // 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 @@ static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) { 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 @@ static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) { 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 @@ static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) { 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 @@ static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) { 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 @@ static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) { // 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 @@ static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) { 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 @@ static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) { 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 @@ static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x) { 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 @@ static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x) { // 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 @@ static int check_policy(X509_STORE_CTX *ctx) { 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 @@ static int check_cert_time(X509_STORE_CTX *ctx, X509 *x) { 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 @@ static int check_cert_time(X509_STORE_CTX *ctx, X509 *x) { 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 @@ static int check_cert_time(X509_STORE_CTX *ctx, X509 *x) { 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 @@ static int check_cert_time(X509_STORE_CTX *ctx, X509 *x) { 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 @@ static int internal_verify(X509_STORE_CTX *ctx) { 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 @@ static int internal_verify(X509_STORE_CTX *ctx) { 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 @@ static int internal_verify(X509_STORE_CTX *ctx) { // 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 893bf4220..d3f4493d8 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -3903,11 +3903,11 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_time_posix(X509_STORE_CTX *ctx, // 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,