Fix a bug introduced in 1.11.6 where we tried to check CRL signatures

against the wrong key, causing any check to fail.

Clean up the NIST X.509 path validation tests and run them by default.

9 files changed, 69 insertions, 96 deletions

diff --git a/doc/relnotes/1_11_8.rst b/doc/relnotes/1_11_8.rst
index 2e8ac1b02..a5bc38a05 100644
--- a/doc/relnotes/1_11_8.rst
+++ b/doc/relnotes/1_11_8.rst
@@ -4,6 +4,9 @@ Version 1.11.8, Not Yet Released
 * The `botan` command line application introduced in 1.11.7 is now
   installed along with the library.
 
+* A bug in certificate path validation introduced in 1.11.6 which
+  caused all CRL signature checks to fail has been corrected.
+
 * The ChaCha20 stream cipher has been added.
 
 * CMAC now supports 256 and 512 bit block ciphers, which also allows
diff --git a/doc/relnotes/contents.rst b/doc/relnotes/contents.rst
index 73a3d270d..406fc4133 100644
--- a/doc/relnotes/contents.rst
+++ b/doc/relnotes/contents.rst
@@ -3,5 +3,6 @@ Release Notes
 ========================================
 
 .. toctree::
+   :maxdepth: 2
 
    index
diff --git a/doc/relnotes/index.rst b/doc/relnotes/index.rst
index 9cc15689d..aef046d02 100644
--- a/doc/relnotes/index.rst
+++ b/doc/relnotes/index.rst
@@ -1,7 +1,4 @@
 
-Release Notes
-========================================
-
 Series 1.11
 ----------------------------------------
 
diff --git a/src/lib/cert/x509/cert_status.h b/src/lib/cert/x509/cert_status.h
index 0ff5ad5f0..d343d2e58 100644
--- a/src/lib/cert/x509/cert_status.h
+++ b/src/lib/cert/x509/cert_status.h
@@ -38,6 +38,7 @@ enum Certificate_Status_Code {
    CRL_NOT_YET_VALID,
    CRL_HAS_EXPIRED,
    CRL_NOT_FOUND,
+   CRL_BAD_SIGNATURE,
 
    OCSP_CERT_NOT_LISTED,
    OCSP_NOT_YET_VALID,
diff --git a/src/lib/cert/x509/certstor.cpp b/src/lib/cert/x509/certstor.cpp
index e8b3a0718..7d708edd9 100644
--- a/src/lib/cert/x509/certstor.cpp
+++ b/src/lib/cert/x509/certstor.cpp
@@ -10,7 +10,7 @@
 
 namespace Botan {
 
-const X509_CRL* Certificate_Store::find_crl(const X509_Certificate&) const
+const X509_CRL* Certificate_Store::find_crl_for(const X509_Certificate&) const
    {
    return nullptr;
    }
@@ -86,7 +86,7 @@ void Certificate_Store_In_Memory::add_crl(const X509_CRL& crl)
    m_crls.push_back(crl);
    }
 
-const X509_CRL* Certificate_Store_In_Memory::find_crl(const X509_Certificate& subject) const
+const X509_CRL* Certificate_Store_In_Memory::find_crl_for(const X509_Certificate& subject) const
    {
    const std::vector<byte>& key_id = subject.authority_key_id();
 
diff --git a/src/lib/cert/x509/certstor.h b/src/lib/cert/x509/certstor.h
index fc37d8327..8c9fd9610 100644
--- a/src/lib/cert/x509/certstor.h
+++ b/src/lib/cert/x509/certstor.h
@@ -27,7 +27,7 @@ class BOTAN_DLL Certificate_Store
       virtual const X509_Certificate*
          find_cert(const X509_DN& subject_dn, const std::vector<byte>& key_id) const = 0;
 
-      virtual const X509_CRL* find_crl(const X509_Certificate& subject) const;
+      virtual const X509_CRL* find_crl_for(const X509_Certificate& subject) const;
 
       bool certificate_known(const X509_Certificate& cert) const
          {
@@ -62,7 +62,7 @@ class BOTAN_DLL Certificate_Store_In_Memory : public Certificate_Store
          const X509_DN& subject_dn,
          const std::vector<byte>& key_id) const override;
 
-      const X509_CRL* find_crl(const X509_Certificate& subject) const override;
+      const X509_CRL* find_crl_for(const X509_Certificate& subject) const override;
    private:
       // TODO: Add indexing on the DN and key id to avoid linear search
       std::vector<X509_Certificate> m_certs;
diff --git a/src/lib/cert/x509/x509path.cpp b/src/lib/cert/x509/x509path.cpp
index edbceaadd..4f1971311 100644
--- a/src/lib/cert/x509/x509path.cpp
+++ b/src/lib/cert/x509/x509path.cpp
@@ -34,15 +34,12 @@ const X509_Certificate* find_issuing_cert(const X509_Certificate& cert,
    return nullptr;
    }
 
-const X509_CRL* find_crls_from(const X509_Certificate& cert,
-                               const std::vector<Certificate_Store*>& certstores)
+const X509_CRL* find_crls_for(const X509_Certificate& cert,
+                              const std::vector<Certificate_Store*>& certstores)
    {
-   const X509_DN issuer_dn = cert.subject_dn();
-   const std::vector<byte> auth_key_id = cert.subject_key_id();
-
    for(size_t i = 0; i != certstores.size(); ++i)
       {
-      if(const X509_CRL* crl = certstores[i]->find_crl(cert))
+      if(const X509_CRL* crl = certstores[i]->find_crl_for(cert))
          return crl;
       }
 
@@ -152,12 +149,12 @@ Certificate_Status_Code check_chain(const std::vector<X509_Certificate>& cert_pa
             }
          }
 
-      const X509_CRL* crl_p = find_crls_from(ca, certstores);
+      const X509_CRL* crl_p = find_crls_for(subject, certstores);
 
       if(!crl_p)
          {
          if(restrictions.require_revocation_information())
-            return Certificate_Status_Code::NO_REVOCATION_DATA;
+            return Certificate_Status_Code::CRL_NOT_FOUND;
          continue;
          }
 
@@ -173,7 +170,7 @@ Certificate_Status_Code check_chain(const std::vector<X509_Certificate>& cert_pa
          return Certificate_Status_Code::CRL_HAS_EXPIRED;
 
       if(crl.check_signature(ca.subject_public_key()) == false)
-         return Certificate_Status_Code::SIGNATURE_ERROR;
+         return Certificate_Status_Code::CRL_BAD_SIGNATURE;
 
       if(crl.is_revoked(subject))
          return Certificate_Status_Code::CERT_IS_REVOKED;
@@ -333,6 +330,8 @@ std::string Path_Validation_Result::status_string(Certificate_Status_Code code)
          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:
diff --git a/src/tests/nist_x509.cpp b/src/tests/nist_x509.cpp
index ce2a587f7..f385ddddb 100644
--- a/src/tests/nist_x509.cpp
+++ b/src/tests/nist_x509.cpp
@@ -9,12 +9,6 @@ Policy extensions are not implemented, so we skip tests #34-#53.
 
 Tests #75 and #76 are skipped as they make use of relatively obscure CRL
 extensions which are not supported.
-
-In addition, please note that some of the tests have their results altered from
-what the test result should be according to NIST's documentation. The changes
-are clearly marked (see x509test.cpp; search for "CHANGE OF TEST RESULT") and
-there are comments explaining why the results where changed. Currently, tests
-#19, #65, and #67 have had their results changed from the official results.
 */
 
 #include "tests.h"
@@ -29,12 +23,32 @@ there are comments explaining why the results where changed. Currently, tests
 #include <vector>
 #include <map>
 #include <cstdlib>
+#include <boost/filesystem.hpp>
 
-#include <dirent.h>
+namespace fs = boost::filesystem;
 
 using namespace Botan;
 
-std::vector<std::string> dir_listing(const std::string&);
+namespace {
+
+std::vector<std::string> dir_listing(const std::string& dir_name)
+   {
+   std::vector<std::string> paths;
+
+   fs::directory_iterator dir(dir_name), end;
+
+   while(dir != end)
+      {
+      paths.push_back(dir->path().string());
+      ++dir;
+      }
+
+   std::sort(paths.begin(), paths.end());
+
+   return paths;
+   }
+
+}
 
 void run_one_test(u32bit, Path_Validation_Result::Code,
                   std::string, std::string,
@@ -48,20 +62,22 @@ void populate_expected_results();
 
 size_t test_nist_x509()
    {
+   const std::string root_test_dir = "src/tests/data/nist_x509/";
+
    unexp_failure = unexp_success = wrong_error = skipped = 0;
 
+   size_t ran = 0;
+
    try {
 
    populate_expected_results();
 
-   const std::string root_test_dir = "src/test-data/nist_x509/";
-   std::vector<std::string> test_dirs = dir_listing(root_test_dir);
-   std::sort(test_dirs.begin(), test_dirs.end());
+   const std::vector<std::string> test_dirs = dir_listing(root_test_dir);
 
-   for(size_t j = 0; j != test_dirs.size(); j++)
+   for(size_t i = 0; i != test_dirs.size(); i++)
       {
-      const std::string test_dir = root_test_dir + test_dirs[j] + "/";
-      std::vector<std::string> all_files = dir_listing(test_dir);
+      const std::string test_dir = test_dirs[i];
+      const std::vector<std::string> all_files = dir_listing(test_dir);
 
       std::vector<std::string> certs, crls;
       std::string root_cert, to_verify;
@@ -69,28 +85,26 @@ size_t test_nist_x509()
       for(size_t k = 0; k != all_files.size(); k++)
          {
          const std::string current = all_files[k];
+
          if(current.find("int") != std::string::npos &&
             current.find(".crt") != std::string::npos)
-            certs.push_back(test_dir + current);
+            certs.push_back(current);
          else if(current.find("root.crt") != std::string::npos)
-            root_cert = test_dir + current;
+            root_cert = current;
          else if(current.find("end.crt") != std::string::npos)
-            to_verify = test_dir + current;
+            to_verify = current;
          else if(current.find(".crl") != std::string::npos)
-            crls.push_back(test_dir + current);
+            crls.push_back(current);
          }
 
-      if(expected_results.find(j+1) == expected_results.end())
+      if(expected_results.find(i+1) == expected_results.end())
          {
-#if 0
-         std::cout << "Testing disabled for test #" << j+1
-                   << " <skipped>" << std::endl;
-#endif
          skipped++;
          continue;
          }
 
-      run_one_test(j+1, expected_results[j+1],
+      ++ran;
+      run_one_test(i+1, expected_results[i+1],
                    root_cert, to_verify, certs, crls);
       }
 
@@ -101,12 +115,11 @@ size_t test_nist_x509()
       return 1;
       }
 
-   std::cout << "Total unexpected failures: " << unexp_failure << std::endl;
-   std::cout << "Total unexpected successes: " << unexp_success << std::endl;
-   std::cout << "Total incorrect failures: " << wrong_error << std::endl;
-   std::cout << "Tests skipped: " << skipped << std::endl;
+   const size_t all_failures = unexp_failure + unexp_success + wrong_error;
 
-   return unexp_failure + unexp_success + wrong_error;
+   test_report("NIST X.509 path validation", ran, all_failures);
+
+   return all_failures;
    }
 
 void run_one_test(u32bit test_no, Path_Validation_Result::Code expected,
@@ -114,32 +127,19 @@ void run_one_test(u32bit test_no, Path_Validation_Result::Code expected,
                   std::vector<std::string> certs,
                   std::vector<std::string> crls)
    {
-   std::cout << "NIST X.509 test #" << test_no << "... ";
-
    Certificate_Store_In_Memory store;
 
    store.add_certificate(X509_Certificate(root_cert));
 
    X509_Certificate end_user(to_verify);
 
-   for(size_t j = 0; j != certs.size(); j++)
-      store.add_certificate(X509_Certificate(certs[j]));
+   for(size_t i = 0; i != certs.size(); i++)
+      store.add_certificate(X509_Certificate(certs[i]));
 
-   for(size_t j = 0; j != crls.size(); j++)
+   for(size_t i = 0; i != crls.size(); i++)
       {
-      DataSource_Stream in(crls[j]);
-
+      DataSource_Stream in(crls[i]);
       X509_CRL crl(in);
-      /*
-      std::vector<CRL_Entry> crl_entries = crl.get_revoked();
-      for(u32bit k = 0; k != crl_entries.size(); k++)
-         {
-         std::cout << "Revoked: " << std::flush;
-         for(u32bit l = 0; l != crl_entries[k].serial.size(); l++)
-            printf("%02X", crl_entries[k].serial[l]);
-         std::cout << std::endl;
-         }
-      */
       store.add_crl(crl);
       }
 
@@ -153,10 +153,9 @@ void run_one_test(u32bit test_no, Path_Validation_Result::Code expected,
    Path_Validation_Result::Code result = validation_result.result();
 
    if(result == expected)
-      {
-      std::cout << "passed" << std::endl;
       return;
-      }
+
+   std::cout << "NIST X.509 test #" << test_no << ": ";
 
    const std::string result_str = Path_Validation_Result::status_string(result);
    const std::string exp_str = Path_Validation_Result::status_string(expected);
@@ -173,39 +172,11 @@ void run_one_test(u32bit test_no, Path_Validation_Result::Code expected,
       }
    else
       {
-      std::cout << "wrong error: " << result_str << "/" << exp_str << std::endl;
+      std::cout << "wrong error, got '" << result_str << "' expected '" << exp_str << "'" << std::endl;
       wrong_error++;
       }
    }
 
-std::vector<std::string> dir_listing(const std::string& dir_name)
-   {
-   DIR* dir = opendir(dir_name.c_str());
-   if(!dir)
-      {
-      std::cout << "Error, couldn't open dir " << dir_name << std::endl;
-      std::exit(1);
-      }
-
-   std::vector<std::string> listing;
-
-   while(true)
-      {
-      struct dirent* dir_ent = readdir(dir);
-
-      if(!dir_ent)
-         break;
-      const std::string entry = dir_ent->d_name;
-      if(entry == "." || entry == "..")
-         continue;
-
-      listing.push_back(entry);
-      }
-   closedir(dir);
-
-   return listing;
-   }
-
 /*
   The expected results are essentially the error codes that best coorespond
   to the problem described in the testing documentation.
@@ -310,7 +281,7 @@ void populate_expected_results()
    expected_results[62] = Certificate_Status_Code::VERIFIED;
    expected_results[63] = Certificate_Status_Code::VERIFIED;
 
-   expected_results[64] = Certificate_Status_Code::SIGNATURE_ERROR;
+   expected_results[64] = Certificate_Status_Code::CRL_BAD_SIGNATURE;
 
    expected_results[65] = Certificate_Status_Code::CRL_NOT_FOUND;
    expected_results[66] = Certificate_Status_Code::CRL_NOT_FOUND;
diff --git a/src/tests/tests.cpp b/src/tests/tests.cpp
index 065931339..05fbebc89 100644
--- a/src/tests/tests.cpp
+++ b/src/tests/tests.cpp
@@ -1,8 +1,8 @@
 #include "tests.h"
 #include <botan/init.h>
 #include <iostream>
-#include <boost/filesystem.hpp>
 #include <fstream>
+#include <boost/filesystem.hpp>
 
 namespace fs = boost::filesystem;
 
@@ -266,6 +266,7 @@ int main(int argc, char* argv[])
    DEF_TEST(pk_keygen);
    DEF_TEST(cvc);
    DEF_TEST(x509);
+   DEF_TEST(nist_x509);
    DEF_TEST(tls);
 
    if(tests.empty())