From aae41f8c54257d9fa6904d3a9aa09c5db6cefd0d Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 25 Jun 2015 09:47:15 +0100 Subject: [PATCH] Reject calls to X509_verify_cert that have not been reinitialised The function X509_verify_cert checks the value of |ctx->chain| at the beginning, and if it is NULL then it initialises it, along with the value of ctx->untrusted. The normal way to use X509_verify_cert() is to first call X509_STORE_CTX_init(); then set up various parameters etc; then call X509_verify_cert(); then check the results; and finally call X509_STORE_CTX_cleanup(). The initial call to X509_STORE_CTX_init() sets |ctx->chain| to NULL. The only place in the OpenSSL codebase where |ctx->chain| is set to anything other than a non NULL value is in X509_verify_cert itself. Therefore the only ways that |ctx->chain| could be non NULL on entry to X509_verify_cert is if one of the following occurs: 1) An application calls X509_verify_cert() twice without re-initialising in between. 2) An application reaches inside the X509_STORE_CTX structure and changes the value of |ctx->chain| directly. With regards to the second of these, we should discount this - it should not be supported to allow this. With regards to the first of these, the documentation is not exactly crystal clear, but the implication is that you must call X509_STORE_CTX_init() before each call to X509_verify_cert(). If you fail to do this then, at best, the results would be undefined. Calling X509_verify_cert() with |ctx->chain| set to a non NULL value is likely to have unexpected results, and could be dangerous. This commit changes the behaviour of X509_verify_cert() so that it causes an error if |ctx->chain| is anything other than NULL (because this indicates that we have not been initialised properly). It also clarifies the associated documentation. This is a follow up commit to CVE-2015-1793. Reviewed-by: Stephen Henson --- crypto/x509/x509_vfy.c | 22 ++++++++++++++-------- doc/crypto/X509_STORE_CTX_new.pod | 13 +++++++++---- doc/crypto/X509_verify_cert.pod | 3 ++- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 7a7fc59e77..7222113c68 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -193,6 +193,14 @@ int X509_verify_cert(X509_STORE_CTX *ctx) X509err(X509_F_X509_VERIFY_CERT, X509_R_NO_CERT_SET_FOR_US_TO_VERIFY); return -1; } + if (ctx->chain != NULL) { + /* + * This X509_STORE_CTX has already been used to verify a cert. We + * cannot do another one. + */ + X509err(X509_F_X509_VERIFY_CERT, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + return -1; + } cb = ctx->verify_cb; @@ -200,15 +208,13 @@ int X509_verify_cert(X509_STORE_CTX *ctx) * first we make sure the chain we are going to build is present and that * the first entry is in place */ - if (ctx->chain == NULL) { - 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); - goto end; - } - CRYPTO_add(&ctx->cert->references, 1, CRYPTO_LOCK_X509); - ctx->last_untrusted = 1; + 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); + goto end; } + CRYPTO_add(&ctx->cert->references, 1, CRYPTO_LOCK_X509); + ctx->last_untrusted = 1; /* We use a temporary STACK so we can chop and hack at it */ if (ctx->untrusted != NULL diff --git a/doc/crypto/X509_STORE_CTX_new.pod b/doc/crypto/X509_STORE_CTX_new.pod index d8d3346632..7c154572ec 100644 --- a/doc/crypto/X509_STORE_CTX_new.pod +++ b/doc/crypto/X509_STORE_CTX_new.pod @@ -40,10 +40,15 @@ is no longer valid. If B is NULL nothing is done. X509_STORE_CTX_init() sets up B for a subsequent verification operation. -The trusted certificate store is set to B, the end entity certificate -to be verified is set to B and a set of additional certificates (which -will be untrusted but may be used to build the chain) in B. Any or -all of the B, B and B parameters can be B. +It must be called before each call to X509_verify_cert(), i.e. a B is only +good for one call to X509_verify_cert(); if you want to verify a second +certificate with the same B then you must call X509_XTORE_CTX_cleanup() +and then X509_STORE_CTX_init() again before the second call to +X509_verify_cert(). The trusted certificate store is set to B, the end +entity certificate to be verified is set to B and a set of additional +certificates (which will be untrusted but may be used to build the chain) in +B. Any or all of the B, B and B parameters can be +B. X509_STORE_CTX_trusted_stack() sets the set of trusted certificates of B to B. This is an alternative way of specifying trusted certificates diff --git a/doc/crypto/X509_verify_cert.pod b/doc/crypto/X509_verify_cert.pod index e5cfc6f813..48055b0f96 100644 --- a/doc/crypto/X509_verify_cert.pod +++ b/doc/crypto/X509_verify_cert.pod @@ -32,7 +32,8 @@ OpenSSL internally for certificate validation, in both the S/MIME and SSL/TLS code. The negative return value from X509_verify_cert() can only occur if no -certificate is set in B (due to a programming error) or if a retry +certificate is set in B (due to a programming error); if X509_verify_cert() +twice without reinitialising B in between; or if a retry operation is requested during internal lookups (which never happens with standard lookup methods). It is however recommended that application check for <= 0 return value on error. -- 2.25.1