From 3d4f83a5c4c0278ae136e70cdf0799d25f01cde3 Mon Sep 17 00:00:00 2001 From: Viktor Dukhovni Date: Tue, 17 May 2016 18:25:40 -0400 Subject: [PATCH] Ensure verify error is set when X509_verify_cert() fails Set ctx->error = X509_V_ERR_OUT_OF_MEM when verificaiton cannot continue due to malloc failure. Similarly for issuer lookup failures and caller errors (bad parameters or invalid state). Also, when X509_verify_cert() returns <= 0 make sure that the verification status does not remain X509_V_OK, as a last resort set it it to X509_V_ERR_UNSPECIFIED, just in case some code path returns an error without setting an appropriate value of ctx->error. Add new and some missing error codes to X509 error -> SSL alert switch. Reviewed-by: Rich Salz --- crypto/x509/x509_txt.c | 4 ++++ crypto/x509/x509_vfy.c | 43 +++++++++++++++++++++++++++++++++++------ crypto/x509/x509_vfy.h | 22 +++++++++++++++++++-- crypto/x509v3/v3_addr.c | 6 ++++++ ssl/s3_both.c | 3 +++ 5 files changed, 70 insertions(+), 8 deletions(-) diff --git a/crypto/x509/x509_txt.c b/crypto/x509/x509_txt.c index d834180ec4..0952813bd2 100644 --- a/crypto/x509/x509_txt.c +++ b/crypto/x509/x509_txt.c @@ -183,6 +183,10 @@ const char *X509_verify_cert_error_string(long n) return ("unsupported or invalid name syntax"); case X509_V_ERR_CRL_PATH_VALIDATION_ERROR: return ("CRL path validation error"); + case X509_V_ERR_INVALID_CALL: + return ("Invalid certificate verification context"); + case X509_V_ERR_STORE_LOOKUP: + return ("Issuer certificate lookup error"); default: BIO_snprintf(buf, sizeof buf, "error number %ld", n); diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 3bad523f71..2e2287e6c9 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -160,6 +160,7 @@ int X509_verify_cert(X509_STORE_CTX *ctx) STACK_OF(X509) *sktmp = NULL; if (ctx->cert == NULL) { X509err(X509_F_X509_VERIFY_CERT, X509_R_NO_CERT_SET_FOR_US_TO_VERIFY); + ctx->error = X509_V_ERR_INVALID_CALL; return -1; } if (ctx->chain != NULL) { @@ -168,6 +169,7 @@ int X509_verify_cert(X509_STORE_CTX *ctx) * cannot do another one. */ X509err(X509_F_X509_VERIFY_CERT, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + ctx->error = X509_V_ERR_INVALID_CALL; return -1; } @@ -180,6 +182,8 @@ int X509_verify_cert(X509_STORE_CTX *ctx) if (((ctx->chain = sk_X509_new_null()) == NULL) || (!sk_X509_push(ctx->chain, ctx->cert))) { X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; + ok = -1; goto end; } CRYPTO_add(&ctx->cert->references, 1, CRYPTO_LOCK_X509); @@ -189,6 +193,8 @@ int X509_verify_cert(X509_STORE_CTX *ctx) if (ctx->untrusted != NULL && (sktmp = sk_X509_dup(ctx->untrusted)) == NULL) { X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; + ok = -1; goto end; } @@ -214,6 +220,8 @@ int X509_verify_cert(X509_STORE_CTX *ctx) if (xtmp != NULL) { if (!sk_X509_push(ctx->chain, xtmp)) { X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; + ok = -1; goto end; } CRYPTO_add(&xtmp->references, 1, CRYPTO_LOCK_X509); @@ -293,15 +301,19 @@ int X509_verify_cert(X509_STORE_CTX *ctx) if (ctx->check_issued(ctx, x, x)) break; ok = ctx->get_issuer(&xtmp, ctx, x); - if (ok < 0) - return ok; + if (ok < 0) { + ctx->error = X509_V_ERR_STORE_LOOKUP; + goto end; + } if (ok == 0) break; x = xtmp; if (!sk_X509_push(ctx->chain, x)) { X509_free(xtmp); X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE); - return 0; + ctx->error = X509_V_ERR_OUT_OF_MEM; + ok = -1; + goto end; } num++; } @@ -317,8 +329,10 @@ int X509_verify_cert(X509_STORE_CTX *ctx) while (j-- > 1) { xtmp2 = sk_X509_value(ctx->chain, j - 1); ok = ctx->get_issuer(&xtmp, ctx, xtmp2); - if (ok < 0) + if (ok < 0) { + ctx->error = X509_V_ERR_STORE_LOOKUP; goto end; + } /* Check if we found an alternate chain */ if (ok > 0) { /* @@ -432,6 +446,10 @@ int X509_verify_cert(X509_STORE_CTX *ctx) sk_X509_free(sktmp); if (chain_ss != NULL) X509_free(chain_ss); + + /* Safety net, error returns must set ctx->error */ + if (ok <= 0 && ctx->error == X509_V_OK) + ctx->error = X509_V_ERR_UNSPECIFIED; return ok; } @@ -654,12 +672,19 @@ static int check_name_constraints(X509_STORE_CTX *ctx) NAME_CONSTRAINTS *nc = sk_X509_value(ctx->chain, j)->nc; if (nc) { rv = NAME_CONSTRAINTS_check(x, nc); - if (rv != X509_V_OK) { + switch (rv) { + case X509_V_OK: + continue; + case X509_V_ERR_OUT_OF_MEM: + ctx->error = rv; + return 0; + default: ctx->error = rv; ctx->error_depth = i; ctx->current_cert = x; if (!ctx->verify_cb(0, ctx)) return 0; + break; } } } @@ -1469,6 +1494,7 @@ static int check_policy(X509_STORE_CTX *ctx) ctx->param->policies, ctx->param->flags); if (ret == 0) { X509err(X509_F_CHECK_POLICY, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; return 0; } /* Invalid or inconsistent extensions */ @@ -1497,7 +1523,12 @@ static int check_policy(X509_STORE_CTX *ctx) if (ctx->param->flags & X509_V_FLAG_NOTIFY_POLICY) { ctx->current_cert = NULL; - ctx->error = X509_V_OK; + /* + * Verification errors need to be "sticky", a callback may have allowed + * an SSL handshake to continue despite an error, and we must then + * remain in an error state. Therefore, we MUST NOT clear earlier + * verification errors by setting the error to X509_V_OK. + */ if (!ctx->verify_cb(2, ctx)) return 0; } diff --git a/crypto/x509/x509_vfy.h b/crypto/x509/x509_vfy.h index b7d8b2472e..eefca75183 100644 --- a/crypto/x509/x509_vfy.h +++ b/crypto/x509/x509_vfy.h @@ -365,13 +365,31 @@ void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); # define X509_V_ERR_PERMITTED_VIOLATION 47 # define X509_V_ERR_EXCLUDED_VIOLATION 48 # define X509_V_ERR_SUBTREE_MINMAX 49 +# define X509_V_ERR_APPLICATION_VERIFICATION 50 # define X509_V_ERR_UNSUPPORTED_CONSTRAINT_TYPE 51 # define X509_V_ERR_UNSUPPORTED_CONSTRAINT_SYNTAX 52 # define X509_V_ERR_UNSUPPORTED_NAME_SYNTAX 53 # define X509_V_ERR_CRL_PATH_VALIDATION_ERROR 54 -/* The application is not happy */ -# define X509_V_ERR_APPLICATION_VERIFICATION 50 +# if 0 /* Reserved for compatibility 1.0.2 */ +/* Suite B mode algorithm violation */ +# define X509_V_ERR_SUITE_B_INVALID_VERSION 56 +# define X509_V_ERR_SUITE_B_INVALID_ALGORITHM 57 +# define X509_V_ERR_SUITE_B_INVALID_CURVE 58 +# define X509_V_ERR_SUITE_B_INVALID_SIGNATURE_ALGORITHM 59 +# define X509_V_ERR_SUITE_B_LOS_NOT_ALLOWED 60 +# define X509_V_ERR_SUITE_B_CANNOT_SIGN_P_384_WITH_P_256 61 + +/* Host, email and IP check errors */ +# define X509_V_ERR_HOSTNAME_MISMATCH 62 +# define X509_V_ERR_EMAIL_MISMATCH 63 +# define X509_V_ERR_IP_ADDRESS_MISMATCH 64 +# endif + +/* Caller error */ +# define X509_V_ERR_INVALID_CALL 65 +/* Issuer lookup error */ +# define X509_V_ERR_STORE_LOOKUP 66 /* Certificate verify flags */ diff --git a/crypto/x509v3/v3_addr.c b/crypto/x509v3/v3_addr.c index 94cfed0509..1290dec9bb 100644 --- a/crypto/x509v3/v3_addr.c +++ b/crypto/x509v3/v3_addr.c @@ -1211,6 +1211,11 @@ int v3_addr_subset(IPAddrBlocks *a, IPAddrBlocks *b) /* * Core code for RFC 3779 2.3 path validation. + * + * Returns 1 for success, 0 on error. + * + * When returning 0, ctx->error MUST be set to an appropriate value other than + * X509_V_OK. */ static int v3_addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain, @@ -1245,6 +1250,7 @@ static int v3_addr_validate_path_internal(X509_STORE_CTX *ctx, if ((child = sk_IPAddressFamily_dup(ext)) == NULL) { X509V3err(X509V3_F_V3_ADDR_VALIDATE_PATH_INTERNAL, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; ret = 0; goto done; } diff --git a/ssl/s3_both.c b/ssl/s3_both.c index 107b460f27..c51fc6ea01 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c @@ -619,7 +619,10 @@ int ssl_verify_alarm_type(long type) case X509_V_ERR_CERT_REVOKED: al = SSL_AD_CERTIFICATE_REVOKED; break; + case X509_V_ERR_UNSPECIFIED: case X509_V_ERR_OUT_OF_MEM: + case X509_V_ERR_INVALID_CALL: + case X509_V_ERR_STORE_LOOKUP: al = SSL_AD_INTERNAL_ERROR; break; case X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT: -- 2.25.1