X.509 path validation now performs all possible tests and returns a

set of error codes, instead of failing immediately on first error.
This prevents a 'weak' error like an expired certificate from hiding a
major error such as signature validation failure or hard revocation.

4 files changed, 153 insertions, 137 deletions

diff --git a/src/lib/cert/x509/cert_status.h b/src/lib/cert/x509/cert_status.h
index d343d2e58..3b80253dc 100644
--- a/src/lib/cert/x509/cert_status.h
+++ b/src/lib/cert/x509/cert_status.h
@@ -8,50 +8,46 @@
 #ifndef BOTAN_X509_PATH_RESULT_H__
 #define BOTAN_X509_PATH_RESULT_H__
 
-#include <string>
-
 namespace Botan {
 
-enum Certificate_Status_Code {
-   VERIFIED,
-   UNKNOWN_X509_ERROR,
-   CANNOT_ESTABLISH_TRUST,
-   CERT_CHAIN_TOO_LONG,
-   SIGNATURE_ERROR,
-   POLICY_ERROR,
-   INVALID_USAGE,
+enum class Certificate_Status_Code {
+   VERIFIED = 0x00000000,
+   OCSP_RESPONSE_GOOD,
+   NO_REVOCATION_DATA,
 
-   SIGNATURE_METHOD_TOO_WEAK,
+   // Local policy failures
+   SIGNATURE_METHOD_TOO_WEAK = 1000,
    UNTRUSTED_HASH,
 
-   CERT_MULTIPLE_ISSUERS_FOUND,
-
-   CERT_FORMAT_ERROR,
-   CERT_ISSUER_NOT_FOUND,
-   CERT_NOT_YET_VALID,
+   // Time problems
+   CERT_NOT_YET_VALID = 2000,
    CERT_HAS_EXPIRED,
-   CERT_IS_REVOKED,
-
-   NO_REVOCATION_DATA,
-
-   CRL_FORMAT_ERROR,
+   OCSP_NOT_YET_VALID,
+   OCSP_HAS_EXPIRED,
    CRL_NOT_YET_VALID,
    CRL_HAS_EXPIRED,
-   CRL_NOT_FOUND,
-   CRL_BAD_SIGNATURE,
 
+   // Chain generation problems
+   CERT_ISSUER_NOT_FOUND = 3000,
+   CANNOT_ESTABLISH_TRUST,
+
+   // Validation errors
+   POLICY_ERROR = 4000,
+   INVALID_USAGE,
+   CERT_CHAIN_TOO_LONG,
+   CA_CERT_NOT_FOR_CERT_ISSUER,
+
+   // Revocation errors
+   CA_CERT_NOT_FOR_CRL_ISSUER,
    OCSP_CERT_NOT_LISTED,
-   OCSP_NOT_YET_VALID,
-   OCSP_EXPIRED,
    OCSP_BAD_STATUS,
-   OCSP_RESPONSE_GOOD,
 
-   CA_CERT_CANNOT_SIGN,
-   CA_CERT_NOT_FOR_CERT_ISSUER,
-   CA_CERT_NOT_FOR_CRL_ISSUER
+   // Hard failures
+   CERT_IS_REVOKED = 5000,
+   CRL_BAD_SIGNATURE,
+   SIGNATURE_ERROR,
 };
 
-
 }
 
 #endif
diff --git a/src/lib/cert/x509/ocsp.cpp b/src/lib/cert/x509/ocsp.cpp
index fe14588e9..fab14fc84 100644
--- a/src/lib/cert/x509/ocsp.cpp
+++ b/src/lib/cert/x509/ocsp.cpp
@@ -210,7 +210,7 @@ Certificate_Status_Code Response::status_for(const X509_Certificate& issuer,
             return Certificate_Status_Code::OCSP_NOT_YET_VALID;
 
          if(response.next_update().time_is_set() && current_time > response.next_update())
-            return Certificate_Status_Code::OCSP_EXPIRED;
+            return Certificate_Status_Code::OCSP_HAS_EXPIRED;
 
          if(response.cert_status() == 0)
             return Certificate_Status_Code::OCSP_RESPONSE_GOOD;
diff --git a/src/lib/cert/x509/x509path.cpp b/src/lib/cert/x509/x509path.cpp
index 317c34718..c1b68e72d 100644
--- a/src/lib/cert/x509/x509path.cpp
+++ b/src/lib/cert/x509/x509path.cpp
@@ -13,6 +13,9 @@
 #include <botan/oids.h>
 #include <algorithm>
 #include <chrono>
+#include <vector>
+#include <set>
+
 #include <iostream>
 
 namespace Botan {
@@ -69,9 +72,10 @@ const X509_CRL* find_crls_for(const X509_Certificate& cert,
    return nullptr;
    }
 
-Certificate_Status_Code check_chain(const std::vector<X509_Certificate>& cert_path,
-                                    const Path_Validation_Restrictions& restrictions,
-                                    const std::vector<Certificate_Store*>& certstores)
+std::vector<std::set<Certificate_Status_Code>>
+check_chain(const std::vector<X509_Certificate>& cert_path,
+            const Path_Validation_Restrictions& restrictions,
+            const std::vector<Certificate_Store*>& certstores)
    {
    const std::set<std::string>& trusted_hashes = restrictions.trusted_hashes();
 
@@ -81,8 +85,12 @@ Certificate_Status_Code check_chain(const std::vector<X509_Certificate>& cert_pa
 
    std::vector<std::future<OCSP::Response>> ocsp_responses;
 
+   std::vector<std::set<Certificate_Status_Code>> cert_status(cert_path.size());
+
    for(size_t i = 0; i != cert_path.size(); ++i)
       {
+      std::set<Certificate_Status_Code>& status = cert_status.at(i);
+
       const bool at_self_signed_root = (i == cert_path.size() - 1);
 
       const X509_Certificate& subject = cert_path[i];
@@ -98,37 +106,42 @@ Certificate_Status_Code check_chain(const std::vector<X509_Certificate>& cert_pa
 
       // Check all certs for valid time range
       if(current_time < X509_Time(subject.start_time()))
-         return Certificate_Status_Code::CERT_NOT_YET_VALID;
+         status.insert(Certificate_Status_Code::CERT_NOT_YET_VALID);
 
       if(current_time > X509_Time(subject.end_time()))
-         return Certificate_Status_Code::CERT_HAS_EXPIRED;
+         status.insert(Certificate_Status_Code::CERT_HAS_EXPIRED);
 
       // Check issuer constraints
 
       // Don't require CA bit set on self-signed end entity cert
       if(!issuer.is_CA_cert() && !self_signed_ee_cert)
-         return Certificate_Status_Code::CA_CERT_NOT_FOR_CERT_ISSUER;
+         status.insert(Certificate_Status_Code::CA_CERT_NOT_FOR_CERT_ISSUER);
 
       if(issuer.path_limit() < i)
-         return Certificate_Status_Code::CERT_CHAIN_TOO_LONG;
+         status.insert(Certificate_Status_Code::CERT_CHAIN_TOO_LONG);
 
       std::unique_ptr<Public_Key> issuer_key(issuer.subject_public_key());
 
       if(subject.check_signature(*issuer_key) == false)
-         return Certificate_Status_Code::SIGNATURE_ERROR;
+         status.insert(Certificate_Status_Code::SIGNATURE_ERROR);
 
       if(issuer_key->estimated_strength() < restrictions.minimum_key_strength())
-         return Certificate_Status_Code::SIGNATURE_METHOD_TOO_WEAK;
+         status.insert(Certificate_Status_Code::SIGNATURE_METHOD_TOO_WEAK);
 
+      // Allow untrusted hashes on self-signed roots
       if(!trusted_hashes.empty() && !at_self_signed_root)
+         {
          if(!trusted_hashes.count(subject.hash_used_for_signature()))
-            return Certificate_Status_Code::UNTRUSTED_HASH;
+            status.insert(Certificate_Status_Code::UNTRUSTED_HASH);
+         }
       }
 
    for(size_t i = 0; i != cert_path.size() - 1; ++i)
       {
-      const X509_Certificate& subject = cert_path[i];
-      const X509_Certificate& ca = cert_path[i+1];
+      std::set<Certificate_Status_Code>& status = cert_status.at(i);
+
+      const X509_Certificate& subject = cert_path.at(i);
+      const X509_Certificate& ca = cert_path.at(i+1);
 
       if(i < ocsp_responses.size())
          {
@@ -136,21 +149,21 @@ Certificate_Status_Code check_chain(const std::vector<X509_Certificate>& cert_pa
             {
             OCSP::Response ocsp = ocsp_responses[i].get();
 
-            auto status = ocsp.status_for(ca, subject);
+            auto ocsp_status = ocsp.status_for(ca, subject);
+
+            status.insert(ocsp_status);
 
-            if(status == CERT_IS_REVOKED)
-               return status;
+            //std::cout << "OCSP status: " << Path_Validation_Result::status_string(ocsp_status) << "\n";
 
-            if(status == OCSP_RESPONSE_GOOD)
-               {
-               if(i == 0 && !restrictions.ocsp_all_intermediates())
-                  return status; // return immediately to just OCSP end cert
-               else
-                  continue;
-               }
+            // Either way we have a definitive answer, no need to check CRLs
+            if(ocsp_status == Certificate_Status_Code::CERT_IS_REVOKED)
+               return cert_status;
+            else if(ocsp_status == Certificate_Status_Code::OCSP_RESPONSE_GOOD)
+               continue;
             }
          catch(std::exception& e)
             {
+            //std::cout << "OCSP error: " << e.what() << "\n";
             }
          }
 
@@ -158,33 +171,32 @@ Certificate_Status_Code check_chain(const std::vector<X509_Certificate>& cert_pa
 
       if(!crl_p)
          {
-         if(restrictions.require_revocation_information())
-            return Certificate_Status_Code::CRL_NOT_FOUND;
+         status.insert(Certificate_Status_Code::NO_REVOCATION_DATA);
          continue;
          }
 
       const X509_CRL& crl = *crl_p;
 
       if(!ca.allowed_usage(CRL_SIGN))
-         return Certificate_Status_Code::CA_CERT_NOT_FOR_CRL_ISSUER;
+         status.insert(Certificate_Status_Code::CA_CERT_NOT_FOR_CRL_ISSUER);
 
       if(current_time < X509_Time(crl.this_update()))
-         return Certificate_Status_Code::CRL_NOT_YET_VALID;
+         status.insert(Certificate_Status_Code::CRL_NOT_YET_VALID);
 
       if(current_time > X509_Time(crl.next_update()))
-         return Certificate_Status_Code::CRL_HAS_EXPIRED;
+         status.insert(Certificate_Status_Code::CRL_HAS_EXPIRED);
 
       if(crl.check_signature(ca.subject_public_key()) == false)
-         return Certificate_Status_Code::CRL_BAD_SIGNATURE;
+         status.insert(Certificate_Status_Code::CRL_BAD_SIGNATURE);
 
       if(crl.is_revoked(subject))
-         return Certificate_Status_Code::CERT_IS_REVOKED;
+         status.insert(Certificate_Status_Code::CERT_IS_REVOKED);
       }
 
    if(self_signed_ee_cert)
-      return Certificate_Status_Code::CANNOT_ESTABLISH_TRUST;
+      cert_status.back().insert(Certificate_Status_Code::CANNOT_ESTABLISH_TRUST);
 
-   return Certificate_Status_Code::VERIFIED;
+   return cert_status;
    }
 
 }
@@ -209,15 +221,11 @@ Path_Validation_Result x509_path_validate(
       if(!cert)
          return Path_Validation_Result(Certificate_Status_Code::CERT_ISSUER_NOT_FOUND);
 
-      if(cert->path_limit() && (cert->path_limit() < cert_path.size()-1))
-         return Path_Validation_Result(Certificate_Status_Code::CERT_CHAIN_TOO_LONG);
-
       cert_path.push_back(*cert);
       }
 
-   Certificate_Status_Code res = check_chain(cert_path, restrictions, certstores);
-
-   return Path_Validation_Result(res, std::move(cert_path));
+   return Path_Validation_Result(check_chain(cert_path, restrictions, certstores),
+                                 std::move(cert_path));
    }
 
 Path_Validation_Result x509_path_validate(
@@ -271,6 +279,25 @@ Path_Validation_Restrictions::Path_Validation_Restrictions(bool require_rev,
    m_trusted_hashes.insert("SHA-512");
    }
 
+Path_Validation_Result::Path_Validation_Result(std::vector<std::set<Certificate_Status_Code>> status,
+                                               std::vector<X509_Certificate>&& cert_chain) :
+   m_overall(Certificate_Status_Code::VERIFIED),
+   m_all_status(status),
+   m_cert_path(cert_chain)
+   {
+   // take the "worst" error as overall
+   for(const auto& s : m_all_status)
+      {
+      if(!s.empty())
+         {
+         auto worst = *s.rbegin();
+         // Leave OCSP confirmations on cert-level status only
+         if(worst != Certificate_Status_Code::OCSP_RESPONSE_GOOD)
+            m_overall = worst;
+         }
+      }
+   }
+
 const X509_Certificate& Path_Validation_Result::trust_root() const
    {
    return m_cert_path[m_cert_path.size()-1];
@@ -286,85 +313,74 @@ std::set<std::string> Path_Validation_Result::trusted_hashes() const
 
 bool Path_Validation_Result::successful_validation() const
    {
-   if(status() == VERIFIED || status() == OCSP_RESPONSE_GOOD)
+   if(result() == Certificate_Status_Code::VERIFIED ||
+      result() == Certificate_Status_Code::OCSP_RESPONSE_GOOD)
       return true;
    return false;
    }
 
 std::string Path_Validation_Result::result_string() const
    {
-   return status_string(m_status);
+   return status_string(result());
    }
 
-std::string Path_Validation_Result::status_string(Certificate_Status_Code code)
+const char* Path_Validation_Result::status_string(Certificate_Status_Code code)
    {
    switch(code)
       {
-      case VERIFIED:
-         return "verified";
-      case UNKNOWN_X509_ERROR:
-         return "unknown error";
-      case CANNOT_ESTABLISH_TRUST:
-         return "cannot establish trust";
-      case CERT_CHAIN_TOO_LONG:
-         return "certificate chain too long";
-      case SIGNATURE_ERROR:
-         return "signature error";
-      case SIGNATURE_METHOD_TOO_WEAK:
-         return "signature method too weak";
-
-      case POLICY_ERROR:
-         return "policy error";
-      case INVALID_USAGE:
-         return "invalid usage";
-      case UNTRUSTED_HASH:
-         return "untrusted hash function";
-
-      case CERT_MULTIPLE_ISSUERS_FOUND:
-         return "Multiple certificate issuers found";
-      case CERT_FORMAT_ERROR:
-         return "Certificate format error";
-      case CERT_ISSUER_NOT_FOUND:
-         return "Certificate issuer not found";
-      case CERT_NOT_YET_VALID:
+      case Certificate_Status_Code::VERIFIED:
+         return "Verified";
+      case Certificate_Status_Code::OCSP_RESPONSE_GOOD:
+         return "OCSP response good";
+      case Certificate_Status_Code::NO_REVOCATION_DATA:
+         return "No revocation data";
+      case Certificate_Status_Code::SIGNATURE_METHOD_TOO_WEAK:
+         return "Signature method too weak";
+      case Certificate_Status_Code::UNTRUSTED_HASH:
+         return "Untrusted hash";
+
+      case Certificate_Status_Code::CERT_NOT_YET_VALID:
          return "Certificate is not yet valid";
-      case CERT_HAS_EXPIRED:
+      case Certificate_Status_Code::CERT_HAS_EXPIRED:
          return "Certificate has expired";
-      case CERT_IS_REVOKED:
-         return "Certificate is revoked";
-      case NO_REVOCATION_DATA:
-         return "No revocation data available";
-      case CRL_FORMAT_ERROR:
-         return "CRL format error";
-      case CRL_NOT_YET_VALID:
+      case Certificate_Status_Code::OCSP_NOT_YET_VALID:
+         return "OCSP is not yet valid";
+      case Certificate_Status_Code::OCSP_HAS_EXPIRED:
+         return "OCSP has expired";
+      case Certificate_Status_Code::CRL_NOT_YET_VALID:
          return "CRL is not yet valid";
-      case CRL_HAS_EXPIRED:
+      case Certificate_Status_Code::CRL_HAS_EXPIRED:
          return "CRL has expired";
-      case CRL_NOT_FOUND:
-         return "CRL not found";
-      case CRL_BAD_SIGNATURE:
-         return "CRL has invalid signature";
-      case CA_CERT_CANNOT_SIGN:
-         return "CA certificate cannot sign";
-      case CA_CERT_NOT_FOR_CERT_ISSUER:
+
+      case Certificate_Status_Code::CERT_ISSUER_NOT_FOUND:
+         return "Certificate issuer not found";
+      case Certificate_Status_Code::CANNOT_ESTABLISH_TRUST:
+         return "Cannot establish trust";
+
+      case Certificate_Status_Code::POLICY_ERROR:
+         return "Policy error";
+      case Certificate_Status_Code::INVALID_USAGE:
+         return "Invalid usage";
+      case Certificate_Status_Code::CERT_CHAIN_TOO_LONG:
+         return "Certificate chain too long";
+      case Certificate_Status_Code::CA_CERT_NOT_FOR_CERT_ISSUER:
          return "CA certificate not allowed to issue certs";
-      case CA_CERT_NOT_FOR_CRL_ISSUER:
+      case Certificate_Status_Code::CA_CERT_NOT_FOR_CRL_ISSUER:
          return "CA certificate not allowed to issue CRLs";
+      case Certificate_Status_Code::OCSP_CERT_NOT_LISTED:
+         return "OCSP cert not listed";
+      case Certificate_Status_Code::OCSP_BAD_STATUS:
+         return "OCSP bad status";
 
-      case OCSP_CERT_NOT_LISTED:
-         return "OCSP response does not included requested cert";
-      case OCSP_NOT_YET_VALID:
-         return "OCSP response is from the future";
-      case OCSP_EXPIRED:
-         return "OCSP response is expired";
-      case OCSP_BAD_STATUS:
-         return "OCSP response had unknown/bad status";
-      case OCSP_RESPONSE_GOOD:
-         return "OCSP response had good status";
+      case Certificate_Status_Code::CERT_IS_REVOKED:
+         return "Certificate is revoked";
+      case Certificate_Status_Code::CRL_BAD_SIGNATURE:
+         return "CRL bad signature";
+      case Certificate_Status_Code::SIGNATURE_ERROR:
+         return "Signature error";
+      default:
+         return "Unknown error";
       }
-
-   // default case
-   return "Unknown code " + std::to_string(code);
    }
 
 }
diff --git a/src/lib/cert/x509/x509path.h b/src/lib/cert/x509/x509path.h
index d6a41a8f8..f7e57759e 100644
--- a/src/lib/cert/x509/x509path.h
+++ b/src/lib/cert/x509/x509path.h
@@ -99,24 +99,27 @@ class BOTAN_DLL Path_Validation_Result
       bool successful_validation() const;
 
       /**
-      * @return validation result code
+      * @return overall validation result code
       */
-      Certificate_Status_Code result() const { return m_status; }
+      Certificate_Status_Code result() const { return m_overall; }
 
-      Certificate_Status_Code status() const { return m_status; }
+      /**
+      * Return a set of status codes for each certificate in the chain
+      */
+      const std::vector<std::set<Certificate_Status_Code>>& all_statuses() const
+         { return m_all_status; }
 
       /**
       * @return string representation of the validation result
       */
       std::string result_string() const;
 
-      static std::string status_string(Certificate_Status_Code code);
+      static const char* status_string(Certificate_Status_Code code);
 
-      Path_Validation_Result(Certificate_Status_Code status,
-                             std::vector<X509_Certificate>&& cert_chain) :
-         m_status(status), m_cert_path(cert_chain) {}
+      Path_Validation_Result(std::vector<std::set<Certificate_Status_Code>> status,
+                             std::vector<X509_Certificate>&& cert_chain);
 
-      Path_Validation_Result(Certificate_Status_Code status) : m_status(status) {}
+      Path_Validation_Result(Certificate_Status_Code status) : m_overall(status) {}
 
    private:
       friend Path_Validation_Result x509_path_validate(
@@ -124,7 +127,8 @@ class BOTAN_DLL Path_Validation_Result
          const Path_Validation_Restrictions& restrictions,
          const std::vector<Certificate_Store*>& certstores);
 
-      Certificate_Status_Code m_status;
+      Certificate_Status_Code m_overall;
+      std::vector<std::set<Certificate_Status_Code>> m_all_status;
       std::vector<X509_Certificate> m_cert_path;
    };