From f3e235ed6faa82170d857fdec3287558eb906c58 Mon Sep 17 00:00:00 2001 From: Viktor Dukhovni Date: Tue, 17 May 2016 13:40:57 -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. 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. Reviewed-by: Richard Levitte --- crypto/x509/x509_txt.c | 4 +++ crypto/x509/x509_vfy.c | 47 ++++++++++++++++++++++++++------- crypto/x509v3/v3_addr.c | 6 +++++ doc/crypto/X509_verify_cert.pod | 2 +- include/openssl/x509_vfy.h | 5 ++++ ssl/statem/statem_lib.c | 10 +++++++ 6 files changed, 64 insertions(+), 10 deletions(-) diff --git a/crypto/x509/x509_txt.c b/crypto/x509/x509_txt.c index 293efcfb8e..5341e79669 100644 --- a/crypto/x509/x509_txt.c +++ b/crypto/x509/x509_txt.c @@ -161,6 +161,10 @@ const char *X509_verify_cert_error_string(long n) return ("CA certificate key too weak"); case X509_V_ERR_CA_MD_TOO_WEAK: return ("CA signature digest algorithm too weak"); + case X509_V_ERR_INVALID_CALL: + return ("Invalid certificate verification context"); + case X509_V_ERR_STORE_LOOKUP: + return ("Issuer certificate lookup error"); default: /* Printing an error number into a static buffer is not thread-safe */ diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 866aa3992e..a5e77896f8 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -251,9 +251,11 @@ static int verify_chain(X509_STORE_CTX *ctx) int X509_verify_cert(X509_STORE_CTX *ctx) { SSL_DANE *dane = ctx->dane; + int ret; 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; } @@ -263,6 +265,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; } @@ -273,6 +276,7 @@ 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; return -1; } X509_up_ref(ctx->cert); @@ -283,15 +287,19 @@ int X509_verify_cert(X509_STORE_CTX *ctx) !verify_cb_cert(ctx, ctx->cert, 0, X509_V_ERR_EE_KEY_TOO_SMALL)) return 0; + if (DANETLS_ENABLED(dane)) + ret = dane_verify(ctx); + else + ret = verify_chain(ctx); + /* - * If dane->trecs is an empty stack, we'll fail, since the user enabled - * DANE. If none of the TLSA records were usable, and it makes sense to - * keep going with an unauthenticated handshake, they can handle that in - * the verify callback, or not set SSL_VERIFY_PEER. + * Safety-net. If we are returning an error, we must also set ctx->error, + * so that the chain is not considered verified should the error be ignored + * (e.g. TLS with SSL_VERIFY_NONE). */ - if (DANETLS_ENABLED(dane)) - return dane_verify(ctx); - return verify_chain(ctx); + if (ret <= 0 && ctx->error == X509_V_OK) + ctx->error = X509_V_ERR_UNSPECIFIED; + return ret; } /* @@ -562,8 +570,16 @@ static int check_name_constraints(X509_STORE_CTX *ctx) if (nc) { int rv = NAME_CONSTRAINTS_check(x, nc); - if (rv != X509_V_OK && !verify_cb_cert(ctx, x, i, rv)) + switch (rv) { + case X509_V_OK: + break; + case X509_V_ERR_OUT_OF_MEM: return 0; + default: + if (!verify_cb_cert(ctx, x, i, rv)) + return 0; + break; + } } } } @@ -1457,6 +1473,7 @@ static int check_policy(X509_STORE_CTX *ctx) */ if (ctx->bare_ta_signed && !sk_X509_push(ctx->chain, NULL)) { X509err(X509_F_CHECK_POLICY, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; return 0; } ret = X509_policy_check(&ctx->tree, &ctx->explicit_policy, ctx->chain, @@ -1466,6 +1483,7 @@ static int check_policy(X509_STORE_CTX *ctx) if (ret == X509_PCY_TREE_INTERNAL) { X509err(X509_F_CHECK_POLICY, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; return 0; } /* Invalid or inconsistent extensions */ @@ -1496,7 +1514,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; } @@ -2742,6 +2765,7 @@ static int build_chain(X509_STORE_CTX *ctx) */ if (ctx->untrusted && (sktmp = sk_X509_dup(ctx->untrusted)) == NULL) { X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; return 0; } @@ -2758,12 +2782,14 @@ static int build_chain(X509_STORE_CTX *ctx) if (DANETLS_ENABLED(dane) && dane->certs != NULL) { if (sktmp == NULL && (sktmp = sk_X509_new_null()) == NULL) { X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; return 0; } for (i = 0; i < sk_X509_num(dane->certs); ++i) { if (!sk_X509_push(sktmp, sk_X509_value(dane->certs, i))) { sk_X509_free(sktmp); X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; return 0; } } @@ -2827,6 +2853,7 @@ static int build_chain(X509_STORE_CTX *ctx) if (ok < 0) { trust = X509_TRUST_REJECTED; + ctx->error = X509_V_ERR_STORE_LOOKUP; search = 0; continue; } @@ -2873,6 +2900,7 @@ static int build_chain(X509_STORE_CTX *ctx) X509_free(xtmp); X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE); trust = X509_TRUST_REJECTED; + ctx->error = X509_V_ERR_OUT_OF_MEM; search = 0; continue; } @@ -2969,6 +2997,7 @@ static int build_chain(X509_STORE_CTX *ctx) if (!sk_X509_push(ctx->chain, xtmp)) { X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE); trust = X509_TRUST_REJECTED; + ctx->error = X509_V_ERR_OUT_OF_MEM; search = 0; continue; } diff --git a/crypto/x509v3/v3_addr.c b/crypto/x509v3/v3_addr.c index 53583fb754..be8ca5dcbb 100644 --- a/crypto/x509v3/v3_addr.c +++ b/crypto/x509v3/v3_addr.c @@ -1166,6 +1166,11 @@ int X509v3_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 addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain, @@ -1200,6 +1205,7 @@ static int addr_validate_path_internal(X509_STORE_CTX *ctx, if ((child = sk_IPAddressFamily_dup(ext)) == NULL) { X509V3err(X509V3_F_ADDR_VALIDATE_PATH_INTERNAL, ERR_R_MALLOC_FAILURE); + ctx->error = X509_V_ERR_OUT_OF_MEM; ret = 0; goto done; } diff --git a/doc/crypto/X509_verify_cert.pod b/doc/crypto/X509_verify_cert.pod index 6b4ff7e742..c7a7bb4b12 100644 --- a/doc/crypto/X509_verify_cert.pod +++ b/doc/crypto/X509_verify_cert.pod @@ -32,7 +32,7 @@ OpenSSL internally for certificate validation, in both the S/MIME and SSL/TLS code. A negative return value from X509_verify_cert() can occur if it is invoked -incurrectly, such as with no certificate set in B, or when it is called +incorrectly, such as with no certificate set in B, or when it is called twice in succession without reinitialising B for the second call. A negative return value can also happen due to internal resource problems or if a retry operation is requested during internal lookups (which never happens diff --git a/include/openssl/x509_vfy.h b/include/openssl/x509_vfy.h index 3826c75b1c..44f1f16991 100644 --- a/include/openssl/x509_vfy.h +++ b/include/openssl/x509_vfy.h @@ -159,6 +159,11 @@ void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); # define X509_V_ERR_CA_KEY_TOO_SMALL 67 # define X509_V_ERR_CA_MD_TOO_WEAK 68 +/* Caller error */ +# define X509_V_ERR_INVALID_CALL 69 +/* Issuer lookup error */ +# define X509_V_ERR_STORE_LOOKUP 70 + /* Certificate verify flags */ # if OPENSSL_API_COMPAT < 0x10100000L diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index a9e1a2496d..6ceb9ec39e 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -548,6 +548,13 @@ int ssl_verify_alarm_type(long type) case X509_V_ERR_CRL_NOT_YET_VALID: case X509_V_ERR_CERT_UNTRUSTED: case X509_V_ERR_CERT_REJECTED: + case X509_V_ERR_HOSTNAME_MISMATCH: + case X509_V_ERR_EMAIL_MISMATCH: + case X509_V_ERR_IP_ADDRESS_MISMATCH: + case X509_V_ERR_DANE_NO_MATCH: + case X509_V_ERR_EE_KEY_TOO_SMALL: + case X509_V_ERR_CA_KEY_TOO_SMALL: + case X509_V_ERR_CA_MD_TOO_WEAK: al = SSL_AD_BAD_CERTIFICATE; break; case X509_V_ERR_CERT_SIGNATURE_FAILURE: @@ -561,7 +568,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: -- GitLab