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 <steve@openssl.org>

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index a0083b552d3a53402ab6c751b95f8d45eeb0183e..2e4c54b8161c0cd0e7d657a01abed695d92ae5b8 100644 (file)
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -162,6 +162,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;
 
@@ -169,15 +177,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 b17888f149e9d346e4486c856285c8d9e36b18ef..eb38b0a109051237f4bb0c8579bb291b30969bfd 100644 (file)
--- a/doc/crypto/X509_STORE_CTX_new.pod
+++ b/doc/crypto/X509_STORE_CTX_new.pod
@@ -39,10 +39,15 @@ X509_STORE_CTX_free() completely frees up B<ctx>. After this call B<ctx>
 is no longer valid.
 
 X509_STORE_CTX_init() sets up B<ctx> for a subsequent verification operation.
-The trusted certificate store is set to B<store>, the end entity certificate
-to be verified is set to B<x509> and a set of additional certificates (which
-will be untrusted but may be used to build the chain) in B<chain>. Any or
-all of the B<store>, B<x509> and B<chain> parameters can be B<NULL>.
+It must be called before each call to X509_verify_cert(), i.e. a B<ctx> is only
+good for one call to X509_verify_cert(); if you want to verify a second
+certificate with the same B<ctx> 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<store>, the end
+entity certificate to be verified is set to B<x509> and a set of additional
+certificates (which will be untrusted but may be used to build the chain) in
+B<chain>. Any or all of the B<store>, B<x509> and B<chain> parameters can be
+B<NULL>.
 
 X509_STORE_CTX_trusted_stack() sets the set of trusted certificates of B<ctx>
 to B<sk>. 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 5253bdcd70722d0642093e16dd400ffd4e13cbe5..a22e44118cfc07b0d75578feaff4047e64133f6e 100644 (file)
--- 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<ctx> (due to a programming error) or if a retry
+certificate is set in B<ctx> (due to a programming error); if X509_verify_cert()
+twice without reinitialising B<ctx> 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.