Merge pull request #313 from randombit/path-validation-fixes

Fix cert validation bugs found by x509test.

7 files changed, 148 insertions, 96 deletions

diff --git a/src/lib/cert/x509/cert_status.h b/src/lib/cert/x509/cert_status.h
index 56777cae8..5028af384 100644
--- a/src/lib/cert/x509/cert_status.h
+++ b/src/lib/cert/x509/cert_status.h
@@ -31,6 +31,8 @@ enum class Certificate_Status_Code {
    CERT_ISSUER_NOT_FOUND = 3000,
    CANNOT_ESTABLISH_TRUST,
 
+   CERT_CHAIN_LOOP,
+
    // Validation errors
    POLICY_ERROR = 4000,
    INVALID_USAGE,
@@ -42,6 +44,8 @@ enum class Certificate_Status_Code {
    OCSP_CERT_NOT_LISTED,
    OCSP_BAD_STATUS,
 
+   CERT_NAME_NOMATCH,
+
    // Hard failures
    CERT_IS_REVOKED = 5000,
    CRL_BAD_SIGNATURE,
diff --git a/src/lib/cert/x509/info.txt b/src/lib/cert/x509/info.txt
index 291be3114..be1e879c3 100644
--- a/src/lib/cert/x509/info.txt
+++ b/src/lib/cert/x509/info.txt
@@ -1,4 +1,4 @@
-define X509_CERTIFICATES 20131128
+define X509_CERTIFICATES 20151023
 define OCSP 20131128
 
 <requires>
diff --git a/src/lib/cert/x509/key_constraint.h b/src/lib/cert/x509/key_constraint.h
index 3509b6868..179e413b5 100644
--- a/src/lib/cert/x509/key_constraint.h
+++ b/src/lib/cert/x509/key_constraint.h
@@ -18,15 +18,15 @@ namespace Botan {
 */
 enum Key_Constraints {
    NO_CONSTRAINTS     = 0,
-   DIGITAL_SIGNATURE  = 32768,
-   NON_REPUDIATION    = 16384,
-   KEY_ENCIPHERMENT   = 8192,
-   DATA_ENCIPHERMENT  = 4096,
-   KEY_AGREEMENT      = 2048,
-   KEY_CERT_SIGN      = 1024,
-   CRL_SIGN           = 512,
-   ENCIPHER_ONLY      = 256,
-   DECIPHER_ONLY      = 128
+   DIGITAL_SIGNATURE  = 1 << 15,
+   NON_REPUDIATION    = 1 << 14,
+   KEY_ENCIPHERMENT   = 1 << 13,
+   DATA_ENCIPHERMENT  = 1 << 12,
+   KEY_AGREEMENT      = 1 << 11,
+   KEY_CERT_SIGN      = 1 << 10,
+   CRL_SIGN           = 1 << 9,
+   ENCIPHER_ONLY      = 1 << 8,
+   DECIPHER_ONLY      = 1 << 7
 };
 
 class Public_Key;
diff --git a/src/lib/cert/x509/x509cert.cpp b/src/lib/cert/x509/x509cert.cpp
index 43bf1099d..3d1ebbbad 100644
--- a/src/lib/cert/x509/x509cert.cpp
+++ b/src/lib/cert/x509/x509cert.cpp
@@ -1,6 +1,6 @@
 /*
 * X.509 Certificates
-* (C) 1999-2010 Jack Lloyd
+* (C) 1999-2010,2015 Jack Lloyd
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
@@ -224,7 +224,7 @@ bool X509_Certificate::is_CA_cert() const
    if(!subject.get1_u32bit("X509v3.BasicConstraints.is_ca"))
       return false;
 
-   return allowed_usage(KEY_CERT_SIGN);
+   return allowed_usage(Key_Constraints(KEY_CERT_SIGN));
    }
 
 bool X509_Certificate::allowed_usage(Key_Constraints usage) const
@@ -236,10 +236,37 @@ bool X509_Certificate::allowed_usage(Key_Constraints usage) const
 
 bool X509_Certificate::allowed_usage(const std::string& usage) const
    {
-   for(auto constraint : ex_constraints())
-      if(constraint == usage)
+   const std::vector<std::string> ex = ex_constraints();
+
+   if(ex.empty())
+      return true;
+
+   if(std::find(ex.begin(), ex.end(), usage) != ex.end())
+      return true;
+
+   return false;
+   }
+
+bool X509_Certificate::allowed_usage(Usage_Type usage) const
+   {
+   switch(usage)
+      {
+      case Usage_Type::UNSPECIFIED:
          return true;
 
+      case Usage_Type::TLS_SERVER_AUTH:
+         return allowed_usage(Key_Constraints(DATA_ENCIPHERMENT | KEY_ENCIPHERMENT | DIGITAL_SIGNATURE)) && allowed_usage("PKIX.ServerAuth");
+
+      case Usage_Type::TLS_CLIENT_AUTH:
+         return allowed_usage(Key_Constraints(DIGITAL_SIGNATURE | NON_REPUDIATION)) && allowed_usage("PKIX.ClientAuth");
+
+      case Usage_Type::OCSP_RESPONDER:
+         return allowed_usage(Key_Constraints(DIGITAL_SIGNATURE | NON_REPUDIATION)) && allowed_usage("PKIX.OCSPSigning");
+
+      case Usage_Type::CERTIFICATE_AUTHORITY:
+         return is_CA_cert();
+      }
+
    return false;
    }
 
@@ -330,36 +357,6 @@ std::vector<byte> X509_Certificate::raw_subject_dn() const
    return subject.get1_memvec("X509.Certificate.dn_bits");
    }
 
-namespace {
-
-bool cert_subject_dns_match(const std::string& name,
-                            const std::vector<std::string>& cert_names)
-   {
-   for(size_t i = 0; i != cert_names.size(); ++i)
-      {
-      const std::string cn = cert_names[i];
-
-      if(cn == name)
-         return true;
-
-      /*
-      * Possible wildcard match. We only support the most basic form of
-      * cert wildcarding ala RFC 2595
-      */
-      if(cn.size() > 2 && cn[0] == '*' && cn[1] == '.' && name.size() > cn.size())
-         {
-         const std::string base = cn.substr(1, std::string::npos);
-
-         if(name.compare(name.size() - base.size(), base.size(), base) == 0)
-            return true;
-         }
-      }
-
-   return false;
-   }
-
-}
-
 std::string X509_Certificate::fingerprint(const std::string& hash_name) const
    {
    std::unique_ptr<HashFunction> hash(HashFunction::create(hash_name));
@@ -385,11 +382,17 @@ bool X509_Certificate::matches_dns_name(const std::string& name) const
    if(name == "")
       return false;
 
-   if(cert_subject_dns_match(name, subject_info("DNS")))
-      return true;
+   std::vector<std::string> issued_names = subject_info("DNS");
 
-   if(cert_subject_dns_match(name, subject_info("Name")))
-      return true;
+   // Fall back to CN only if no DNS names are set (RFC 6125 sec 6.4.4)
+   if(issued_names.empty())
+      issued_names = subject_info("Name");
+
+   for(size_t i = 0; i != issued_names.size(); ++i)
+      {
+      if(host_wildcard_match(issued_names[i], name))
+         return true;
+      }
 
    return false;
    }
@@ -430,45 +433,36 @@ bool operator!=(const X509_Certificate& cert1, const X509_Certificate& cert2)
 
 std::string X509_Certificate::to_string() const
    {
-   const char* dn_fields[] = { "Name",
-                               "Email",
-                               "Organization",
-                               "Organizational Unit",
-                               "Locality",
-                               "State",
-                               "Country",
-                               "IP",
-                               "DNS",
-                               "URI",
-                               "PKIX.XMPPAddr",
-                               nullptr };
+   const std::vector<std::string> dn_fields{
+      "Name",
+      "Email",
+      "Organization",
+      "Organizational Unit",
+      "Locality",
+      "State",
+      "Country",
+      "IP",
+      "DNS",
+      "URI",
+      "PKIX.XMPPAddr"
+      };
 
    std::ostringstream out;
 
-   for(size_t i = 0; dn_fields[i]; ++i)
+   for(auto&& field : dn_fields)
       {
-      const std::vector<std::string> vals = this->subject_info(dn_fields[i]);
-
-      if(vals.empty())
-         continue;
-
-      out << "Subject " << dn_fields[i] << ":";
-      for(size_t j = 0; j != vals.size(); ++j)
-         out << " " << vals[j];
-      out << "\n";
+      for(auto&& val : subject_info(field))
+         {
+         out << "Subject " << field << ": " << val << "\n";
+         }
       }
 
-   for(size_t i = 0; dn_fields[i]; ++i)
+   for(auto&& field : dn_fields)
       {
-      const std::vector<std::string> vals = this->issuer_info(dn_fields[i]);
-
-      if(vals.empty())
-         continue;
-
-      out << "Issuer " << dn_fields[i] << ":";
-      for(size_t j = 0; j != vals.size(); ++j)
-         out << " " << vals[j];
-      out << "\n";
+      for(auto&& val : issuer_info(field))
+         {
+         out << "Issuer " << field << ": " << val << "\n";
+         }
       }
 
    out << "Version: " << this->x509_version() << "\n";
diff --git a/src/lib/cert/x509/x509cert.h b/src/lib/cert/x509/x509cert.h
index 1a46d290f..578360a80 100644
--- a/src/lib/cert/x509/x509cert.h
+++ b/src/lib/cert/x509/x509cert.h
@@ -1,6 +1,6 @@
 /*
 * X.509 Certificates
-* (C) 1999-2007 Jack Lloyd
+* (C) 1999-2007,2015 Jack Lloyd
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
@@ -18,6 +18,15 @@
 
 namespace Botan {
 
+enum class Usage_Type
+   {
+   UNSPECIFIED, // no restrictions
+   TLS_SERVER_AUTH,
+   TLS_CLIENT_AUTH,
+   CERTIFICATE_AUTHORITY,
+   OCSP_RESPONDER
+   };
+
 /**
 * This class represents X.509 Certificate
 */
@@ -137,6 +146,8 @@ class BOTAN_DLL X509_Certificate : public X509_Object
       */
       bool allowed_usage(const std::string& usage) const;
 
+      bool allowed_usage(Usage_Type usage) const;
+
       /**
       * Get the path limit as defined in the BasicConstraints extension of
       * this certificate.
diff --git a/src/lib/cert/x509/x509path.cpp b/src/lib/cert/x509/x509path.cpp
index 09cabcb65..a6c3ce6e9 100644
--- a/src/lib/cert/x509/x509path.cpp
+++ b/src/lib/cert/x509/x509path.cpp
@@ -214,7 +214,9 @@ check_chain(const std::vector<X509_Certificate>& cert_path,
 Path_Validation_Result x509_path_validate(
    const std::vector<X509_Certificate>& end_certs,
    const Path_Validation_Restrictions& restrictions,
-   const std::vector<Certificate_Store*>& certstores)
+   const std::vector<Certificate_Store*>& certstores,
+   const std::string& hostname,
+   Usage_Type usage)
    {
    if(end_certs.empty())
       throw std::invalid_argument("x509_path_validate called with no subjects");
@@ -222,6 +224,13 @@ Path_Validation_Result x509_path_validate(
    std::vector<X509_Certificate> cert_path;
    cert_path.push_back(end_certs[0]);
 
+   /*
+   * This is an inelegant but functional way of preventing path loops
+   * (where C1 -> C2 -> C3 -> C1). We store a set of all the certificate
+   * fingerprints in the path. If there is a duplicate, we error out.
+   */
+   std::set<std::string> certs_seen;
+
    Certificate_Store_Overlay extra(end_certs);
 
    // iterate until we reach a root or cannot find the issuer
@@ -231,38 +240,55 @@ Path_Validation_Result x509_path_validate(
       if(!cert)
          return Path_Validation_Result(Certificate_Status_Code::CERT_ISSUER_NOT_FOUND);
 
+      const std::string fprint = cert->fingerprint("SHA-256");
+      if(certs_seen.count(fprint) > 0)
+         return Path_Validation_Result(Certificate_Status_Code::CERT_CHAIN_LOOP);
+      certs_seen.insert(fprint);
       cert_path.push_back(*cert);
       }
 
-   return Path_Validation_Result(check_chain(cert_path, restrictions, certstores),
-                                 std::move(cert_path));
+   std::vector<std::set<Certificate_Status_Code>> res = check_chain(cert_path, restrictions, certstores);
+
+   if(hostname != "" && !cert_path[0].matches_dns_name(hostname))
+      res[0].insert(Certificate_Status_Code::CERT_NAME_NOMATCH);
+
+   if(!cert_path[0].allowed_usage(usage))
+      res[0].insert(Certificate_Status_Code::INVALID_USAGE);
+
+   return Path_Validation_Result(res, std::move(cert_path));
    }
 
 Path_Validation_Result x509_path_validate(
    const X509_Certificate& end_cert,
    const Path_Validation_Restrictions& restrictions,
-   const std::vector<Certificate_Store*>& certstores)
+   const std::vector<Certificate_Store*>& certstores,
+   const std::string& hostname,
+   Usage_Type usage)
    {
    std::vector<X509_Certificate> certs;
    certs.push_back(end_cert);
-   return x509_path_validate(certs, restrictions, certstores);
+   return x509_path_validate(certs, restrictions, certstores, hostname, usage);
    }
 
 Path_Validation_Result x509_path_validate(
    const std::vector<X509_Certificate>& end_certs,
    const Path_Validation_Restrictions& restrictions,
-   const Certificate_Store& store)
+   const Certificate_Store& store,
+   const std::string& hostname,
+   Usage_Type usage)
    {
    std::vector<Certificate_Store*> certstores;
    certstores.push_back(const_cast<Certificate_Store*>(&store));
 
-   return x509_path_validate(end_certs, restrictions, certstores);
+   return x509_path_validate(end_certs, restrictions, certstores, hostname, usage);
    }
 
 Path_Validation_Result x509_path_validate(
    const X509_Certificate& end_cert,
    const Path_Validation_Restrictions& restrictions,
-   const Certificate_Store& store)
+   const Certificate_Store& store,
+   const std::string& hostname,
+   Usage_Type usage)
    {
    std::vector<X509_Certificate> certs;
    certs.push_back(end_cert);
@@ -270,7 +296,7 @@ Path_Validation_Result x509_path_validate(
    std::vector<Certificate_Store*> certstores;
    certstores.push_back(const_cast<Certificate_Store*>(&store));
 
-   return x509_path_validate(certs, restrictions, certstores);
+   return x509_path_validate(certs, restrictions, certstores, hostname, usage);
    }
 
 Path_Validation_Restrictions::Path_Validation_Restrictions(bool require_rev,
@@ -310,6 +336,9 @@ Path_Validation_Result::Path_Validation_Result(std::vector<std::set<Certificate_
 
 const X509_Certificate& Path_Validation_Result::trust_root() const
    {
+   if(m_cert_path.empty())
+      throw std::runtime_error("Path_Validation_Result::trust_root no path set");
+
    return m_cert_path[m_cert_path.size()-1];
    }
 
@@ -366,6 +395,8 @@ const char* Path_Validation_Result::status_string(Certificate_Status_Code code)
          return "Certificate issuer not found";
       case Certificate_Status_Code::CANNOT_ESTABLISH_TRUST:
          return "Cannot establish trust";
+      case Certificate_Status_Code::CERT_CHAIN_LOOP:
+         return "Loop in certificate chain";
 
       case Certificate_Status_Code::POLICY_ERROR:
          return "Policy error";
@@ -381,6 +412,8 @@ const char* Path_Validation_Result::status_string(Certificate_Status_Code code)
          return "OCSP cert not listed";
       case Certificate_Status_Code::OCSP_BAD_STATUS:
          return "OCSP bad status";
+      case Certificate_Status_Code::CERT_NAME_NOMATCH:
+         return "Certificate does not match provided name";
 
       case Certificate_Status_Code::CERT_IS_REVOKED:
          return "Certificate is revoked";
@@ -388,9 +421,10 @@ const char* Path_Validation_Result::status_string(Certificate_Status_Code code)
          return "CRL bad signature";
       case Certificate_Status_Code::SIGNATURE_ERROR:
          return "Signature error";
-      default:
-         return "Unknown error";
+         // intentionally no default so we are warned
       }
+
+   return "Unknown error";
    }
 
 }
diff --git a/src/lib/cert/x509/x509path.h b/src/lib/cert/x509/x509path.h
index f400641be..c56aef21f 100644
--- a/src/lib/cert/x509/x509path.h
+++ b/src/lib/cert/x509/x509path.h
@@ -132,13 +132,16 @@ class BOTAN_DLL Path_Validation_Result
       std::vector<X509_Certificate> m_cert_path;
    };
 
+
 /**
 * PKIX Path Validation
 */
 Path_Validation_Result BOTAN_DLL x509_path_validate(
    const std::vector<X509_Certificate>& end_certs,
    const Path_Validation_Restrictions& restrictions,
-   const std::vector<Certificate_Store*>& certstores);
+   const std::vector<Certificate_Store*>& certstores,
+   const std::string& hostname = "",
+   Usage_Type usage = Usage_Type::UNSPECIFIED);
 
 /**
 * PKIX Path Validation
@@ -146,7 +149,9 @@ Path_Validation_Result BOTAN_DLL x509_path_validate(
 Path_Validation_Result BOTAN_DLL x509_path_validate(
    const X509_Certificate& end_cert,
    const Path_Validation_Restrictions& restrictions,
-   const std::vector<Certificate_Store*>& certstores);
+   const std::vector<Certificate_Store*>& certstores,
+   const std::string& hostname = "",
+   Usage_Type usage = Usage_Type::UNSPECIFIED);
 
 /**
 * PKIX Path Validation
@@ -154,7 +159,9 @@ Path_Validation_Result BOTAN_DLL x509_path_validate(
 Path_Validation_Result BOTAN_DLL x509_path_validate(
    const X509_Certificate& end_cert,
    const Path_Validation_Restrictions& restrictions,
-   const Certificate_Store& store);
+   const Certificate_Store& store,
+   const std::string& hostname = "",
+   Usage_Type usage = Usage_Type::UNSPECIFIED);
 
 /**
 * PKIX Path Validation
@@ -162,7 +169,9 @@ Path_Validation_Result BOTAN_DLL x509_path_validate(
 Path_Validation_Result BOTAN_DLL x509_path_validate(
    const std::vector<X509_Certificate>& end_certs,
    const Path_Validation_Restrictions& restrictions,
-   const Certificate_Store& store);
+   const Certificate_Store& store,
+   const std::string& hostname = "",
+   Usage_Type usage = Usage_Type::UNSPECIFIED);
 
 }