From 7def8d303e3cf0f1a27ee8ebcb8ae5137261a361 Mon Sep 17 00:00:00 2001 From: lloyd Date: Sat, 8 Feb 2014 15:50:01 +0000 Subject: 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. --- src/lib/cert/x509/cert_status.h | 1 + src/lib/cert/x509/certstor.cpp | 4 +- src/lib/cert/x509/certstor.h | 4 +- src/lib/cert/x509/x509path.cpp | 17 +++--- src/tests/nist_x509.cpp | 129 ++++++++++++++++------------------------ src/tests/tests.cpp | 3 +- 6 files changed, 65 insertions(+), 93 deletions(-) (limited to 'src') 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& 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& 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& 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 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& certstores) +const X509_CRL* find_crls_for(const X509_Certificate& cert, + const std::vector& certstores) { - const X509_DN issuer_dn = cert.subject_dn(); - const std::vector 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& 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& 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 #include #include +#include -#include +namespace fs = boost::filesystem; using namespace Botan; -std::vector dir_listing(const std::string&); +namespace { + +std::vector dir_listing(const std::string& dir_name) + { + std::vector 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 test_dirs = dir_listing(root_test_dir); - std::sort(test_dirs.begin(), test_dirs.end()); + const std::vector 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 all_files = dir_listing(test_dir); + const std::string test_dir = test_dirs[i]; + const std::vector all_files = dir_listing(test_dir); std::vector 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 - << " " << 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 certs, std::vector 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_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 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 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 #include -#include #include +#include 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()) -- cgit v1.2.3