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.

(Imported from upstream's 692f07c3e0c04180b56febc2feb57cd94395a7a2.)

Change-Id: I971f1a305f12bbf9f4ae955313d5557368f0d374
Reviewed-on: https://boringssl-review.googlesource.com/6760
Reviewed-by: Adam Langley <agl@google.com>

1 files changed, 13 insertions, 9 deletions

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index ba5885c5..c62a6f58 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -204,22 +204,26 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
 		OPENSSL_PUT_ERROR(X509, 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. */
+		OPENSSL_PUT_ERROR(X509, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+		return -1;
+		}
 
 	cb=ctx->verify_cb;
 
 	/* 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)
+	ctx->chain = sk_X509_new_null();
+	if (ctx->chain == NULL || !sk_X509_push(ctx->chain, ctx->cert))
 		{
-		if (	((ctx->chain=sk_X509_new_null()) == NULL) ||
-			(!sk_X509_push(ctx->chain,ctx->cert)))
-			{
-			OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE);
-			goto end;
-			}
-		X509_up_ref(ctx->cert);
-		ctx->last_untrusted=1;
+		OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE);
+		goto end;
 		}
+	X509_up_ref(ctx->cert);
+	ctx->last_untrusted = 1;
 
 	/* We use a temporary STACK so we can chop and hack at it */
 	if (ctx->untrusted != NULL