CPPXT-105 - Fix reuse of X509_STORE_CTX during CRL checking
[shibboleth/cpp-xmltooling.git] / xmltooling / security / impl / PKIXPathValidator.cpp
index 602d001..9a11070 100644 (file)
@@ -113,8 +113,8 @@ namespace xmltooling {
     {
     public:
         PKIXPathValidator(const xercesc::DOMElement* e)
-            : m_log(Category::getInstance(XMLTOOLING_LOGCAT".PathValidator.PKIX")),
-              m_lock(XMLToolingConfig::getConfig().getNamedMutex(XMLTOOLING_LOGCAT".PathValidator.PKIX")),
+            : m_log(Category::getInstance(XMLTOOLING_LOGCAT ".PathValidator.PKIX")),
+              m_lock(XMLToolingConfig::getConfig().getNamedMutex(XMLTOOLING_LOGCAT ".PathValidator.PKIX")),
               m_minRefreshDelay(XMLHelper::getAttrInt(e, 60, minRefreshDelay)),
               m_minSecondsRemaining(XMLHelper::getAttrInt(e, 86400, minSecondsRemaining)),
               m_minPercentRemaining(XMLHelper::getAttrInt(e, 10, minPercentRemaining)) {
@@ -291,7 +291,7 @@ bool PKIXPathValidator::validate(X509* EE, STACK_OF(X509)* untrusted, const Path
 
     // AFAICT, EE and untrusted are passed in but not owned by the ctx.
 #if (OPENSSL_VERSION_NUMBER >= 0x00907000L)
-    if (X509_STORE_CTX_init(&ctx,store,EE,untrusted)!=1) {
+    if (X509_STORE_CTX_init(&ctx,store,EE,untrusted) != 1) {
         log_openssl();
         m_log.error("unable to initialize X509_STORE_CTX");
         X509_STORE_free(store);
@@ -317,8 +317,8 @@ bool PKIXPathValidator::validate(X509* EE, STACK_OF(X509)* untrusted, const Path
     X509_STORE_CTX_set_verify_cb(&ctx,error_callback);
 
     // Do a first pass verify. If CRLs aren't used, this is the only pass.
-    int ret=X509_verify_cert(&ctx);
-    if (ret==1) {
+    int ret = X509_verify_cert(&ctx);
+    if (ret == 1) {
         // Now see if the depth was acceptable by counting the number of intermediates.
         int depth=sk_X509_num(ctx.chain)-2;
         if (pkixParams->getVerificationDepth() < depth) {
@@ -327,32 +327,25 @@ bool PKIXPathValidator::validate(X509* EE, STACK_OF(X509)* untrusted, const Path
                 (depth==-1) ? 0 : depth,
                 pkixParams->getVerificationDepth()
                 );
-            ret=0;
+            ret = 0;
         }
     }
 
-    if (pkixParams->getRevocationChecking() != PKIXPathValidatorParams::REVOCATION_OFF) {
+    // If the first pass succeeded, check to see if we need a second with CRLs.
+    if (ret == 1 && pkixParams->getRevocationChecking() != PKIXPathValidatorParams::REVOCATION_OFF) {
 #if (OPENSSL_VERSION_NUMBER >= 0x00907000L)
-        // When we add CRLs, we have to be sure the nextUpdate hasn't passed, because OpenSSL won't accept
+        // After the first X509_verify_cert call, the ctx can no longer be used
+        // (subsequent calls will fail with OpenSSL 1.0.1p / 1.0.2d or later).
+        X509_STORE_CTX_cleanup(&ctx);
+
+       // When we add CRLs, we have to be sure the nextUpdate hasn't passed, because OpenSSL won't accept
         // the CRL in that case. If we end up not adding a CRL for a particular link in the chain, the
         // validation will fail (if the fullChain option was set).
         set<string> crlissuers;
         time_t now = time(nullptr);
 
-        const vector<XSECCryptoX509CRL*>& crls = pkixParams->getCRLs();
-        for (vector<XSECCryptoX509CRL*>::const_iterator j=crls.begin(); j!=crls.end(); ++j) {
-            if ((*j)->getProviderName()==DSIGConstants::s_unicodeStrPROVOpenSSL &&
-                (X509_cmp_time(X509_CRL_get_nextUpdate(static_cast<OpenSSLCryptoX509CRL*>(*j)->getOpenSSLX509CRL()), &now) > 0)) {
-                // owned by store
-                X509_STORE_add_crl(store, X509_CRL_dup(static_cast<OpenSSLCryptoX509CRL*>(*j)->getOpenSSLX509CRL()));
-                string crlissuer(X509_NAME_to_string(X509_CRL_get_issuer(static_cast<OpenSSLCryptoX509CRL*>(*j)->getOpenSSLX509CRL())));
-                if (!crlissuer.empty()) {
-                    m_log.debug("added CRL issued by (%s)", crlissuer.c_str());
-                    crlissuers.insert(crlissuer);
-                }
-            }
-        }
-
+        // Pull CRLs from external CDP first, since an attacker is likely to stick an old but valid CRL into
+        // the signature.
         for (int i = 0; i < sk_X509_num(untrusted); ++i) {
             X509 *cert = sk_X509_value(untrusted, i);
             string crlissuer(X509_NAME_to_string(X509_get_issuer_name(cert)));
@@ -360,7 +353,6 @@ bool PKIXPathValidator::validate(X509* EE, STACK_OF(X509)* untrusted, const Path
                // We already have a CRL for this cert, so skip CRLDP processing for this one.
                continue;
             }
-
             bool foundUsableCDP = false;
             STACK_OF(DIST_POINT)* dps = (STACK_OF(DIST_POINT)*)X509_get_ext_d2i(cert, NID_crl_distribution_points, nullptr, nullptr);
             for (int ii = 0; !foundUsableCDP && ii < sk_DIST_POINT_num(dps); ++ii) {
@@ -369,12 +361,8 @@ bool PKIXPathValidator::validate(X509* EE, STACK_OF(X509)* untrusted, const Path
                     continue;
                 for (int iii = 0; !foundUsableCDP && iii < sk_GENERAL_NAME_num(dp->distpoint->name.fullname); ++iii) {
                     GENERAL_NAME* gen = sk_GENERAL_NAME_value(dp->distpoint->name.fullname, iii);
-                    // Only consider HTTP URIs, and stop after the first one we find.
-#ifdef HAVE_STRCASECMP
-                    if (gen->type == GEN_URI && (!strncasecmp((const char*)gen->d.ia5->data, "http:", 5))) {
-#else
-                    if (gen->type == GEN_URI && (!strnicmp((const char*)gen->d.ia5->data, "http:", 5))) {
-#endif
+                    // Only consider URIs, and stop after the first one we find.
+                    if (gen->type == GEN_URI) {
                         const char* cdpuri = (const char*)gen->d.ia5->data;
                         auto_ptr<XSECCryptoX509CRL> crl(getRemoteCRLs(cdpuri));
                         if (crl.get() && crl->getProviderName()==DSIGConstants::s_unicodeStrPROVOpenSSL &&
@@ -391,12 +379,44 @@ bool PKIXPathValidator::validate(X509* EE, STACK_OF(X509)* untrusted, const Path
             sk_DIST_POINT_free(dps);
         }
 
-        // Do a second pass verify with CRLs in place.
-        if (pkixParams->getRevocationChecking() == PKIXPathValidatorParams::REVOCATION_FULLCHAIN)
-            X509_STORE_CTX_set_flags(&ctx, X509_V_FLAG_CRL_CHECK|X509_V_FLAG_CRL_CHECK_ALL);
-        else
-            X509_STORE_CTX_set_flags(&ctx, X509_V_FLAG_CRL_CHECK);
-        ret=X509_verify_cert(&ctx);
+        // Pick up any valid CRLs inline.
+        const vector<XSECCryptoX509CRL*>& crls = pkixParams->getCRLs();
+        for (vector<XSECCryptoX509CRL*>::const_iterator j=crls.begin(); j!=crls.end(); ++j) {
+            if ((*j)->getProviderName()==DSIGConstants::s_unicodeStrPROVOpenSSL &&
+                (X509_cmp_time(X509_CRL_get_nextUpdate(static_cast<OpenSSLCryptoX509CRL*>(*j)->getOpenSSLX509CRL()), &now) > 0)) {
+                string crlissuer(X509_NAME_to_string(X509_CRL_get_issuer(static_cast<OpenSSLCryptoX509CRL*>(*j)->getOpenSSLX509CRL())));
+                if (crlissuer.empty() || crlissuers.count(crlissuer)) {
+                   // We already have a CRL for this cert, so skip this one.
+                   continue;
+                }
+                m_log.debug("added CRL issued by (%s)", crlissuer.c_str());
+                crlissuers.insert(crlissuer);
+                // owned by store
+                X509_STORE_add_crl(store, X509_CRL_dup(static_cast<OpenSSLCryptoX509CRL*>(*j)->getOpenSSLX509CRL()));
+            }
+        }
+
+        // Do a second pass verify with CRLs in place. Reinitialize ctx, see
+        // https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=aae41f8c54257d9fa6904d3a9aa09c5db6cefd0d
+#if (OPENSSL_VERSION_NUMBER >= 0x00907000L)
+               if (X509_STORE_CTX_init(&ctx,store,EE,untrusted) != 1) {
+                       log_openssl();
+                       m_log.error("unable to initialize X509_STORE_CTX");
+                       ret = 0;
+               }
+#else
+       X509_STORE_CTX_init(&ctx,store,EE,untrusted);
+#endif
+       if (ret != 0) {
+                       X509_STORE_CTX_trusted_stack(&ctx,CAstack);
+                       X509_STORE_CTX_set_depth(&ctx,100);  // already checked above
+                       X509_STORE_CTX_set_verify_cb(&ctx,error_callback);
+                       if (pkixParams->getRevocationChecking() == PKIXPathValidatorParams::REVOCATION_FULLCHAIN)
+                               X509_STORE_CTX_set_flags(&ctx, X509_V_FLAG_CRL_CHECK|X509_V_FLAG_CRL_CHECK_ALL);
+                       else
+                               X509_STORE_CTX_set_flags(&ctx, X509_V_FLAG_CRL_CHECK);
+                       ret = X509_verify_cert(&ctx);
+       }
 #else
         m_log.warn("CRL checking is enabled, but OpenSSL version is too old");
         ret = 0;
@@ -406,7 +426,7 @@ bool PKIXPathValidator::validate(X509* EE, STACK_OF(X509)* untrusted, const Path
     if (ret == 1) {
         m_log.debug("successfully validated certificate chain");
     }
-#if (OPENSSL_VERSION_NUMBER < 0x10000000L)
+#if defined(X509_V_ERR_NO_EXPLICIT_POLICY) && (OPENSSL_VERSION_NUMBER < 0x10000000L)
     else if (X509_STORE_CTX_get_error(&ctx) == X509_V_ERR_NO_EXPLICIT_POLICY && !pkixParams->isPolicyMappingInhibited()) {
         m_log.warn("policy mapping requires OpenSSL 1.0.0 or later");
     }
@@ -427,7 +447,7 @@ XSECCryptoX509CRL* PKIXPathValidator::getRemoteCRLs(const char* cdpuri) const
 
     // The filenames for the CRL cache are based on a hash of the CRL location.
     string cdpfile = SecurityHelper::doHash("SHA1", cdpuri, strlen(cdpuri)) + ".crl";
-    XMLToolingConfig::getConfig().getPathResolver()->resolve(cdpfile, PathResolver::XMLTOOLING_RUN_FILE);
+    XMLToolingConfig::getConfig().getPathResolver()->resolve(cdpfile, PathResolver::XMLTOOLING_CACHE_FILE);
     string cdpstaging = cdpfile + ".tmp";
 
     time_t now = time(nullptr);