From 44972629766c2b3a5bc268c26e0491b02dd9fb7c Mon Sep 17 00:00:00 2001 From: Evgeny Margolis Date: Tue, 10 May 2022 11:25:00 -0700 Subject: [PATCH] Fix OpenSSL Implementation of ValidateCertificateChain(). (#18125) In the OpenSSL Implementation of ValidateCertificateChain() function the intermediate ceritificate is loaded as a trusted certificate, which opens door to various security attacks. Updated implementation: now loading intermediate certificate as untrusted certificate part of cert chain. Added test case that would identify this issue in the previous implementation. --- src/crypto/CHIPCryptoPALOpenSSL.cpp | 9 +++++++-- src/crypto/tests/CHIPCryptoPALTest.cpp | 23 +++++++++++++++++------ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/crypto/CHIPCryptoPALOpenSSL.cpp b/src/crypto/CHIPCryptoPALOpenSSL.cpp index 8c59ffeae96ca7..b2ea8c3ef25274 100644 --- a/src/crypto/CHIPCryptoPALOpenSSL.cpp +++ b/src/crypto/CHIPCryptoPALOpenSSL.cpp @@ -1530,6 +1530,7 @@ CHIP_ERROR ValidateCertificateChain(const uint8_t * rootCertificate, size_t root int status = 0; X509_STORE_CTX * verifyCtx = nullptr; X509_STORE * store = nullptr; + STACK_OF(X509) * chain = nullptr; X509 * x509RootCertificate = nullptr; X509 * x509CACertificate = nullptr; X509 * x509LeafCertificate = nullptr; @@ -1549,6 +1550,9 @@ CHIP_ERROR ValidateCertificateChain(const uint8_t * rootCertificate, size_t root verifyCtx = X509_STORE_CTX_new(); VerifyOrExit(verifyCtx != nullptr, (result = CertificateChainValidationResult::kNoMemory, err = CHIP_ERROR_NO_MEMORY)); + chain = sk_X509_new_null(); + VerifyOrExit(chain != nullptr, (result = CertificateChainValidationResult::kNoMemory, err = CHIP_ERROR_NO_MEMORY)); + x509RootCertificate = d2i_X509(nullptr, &rootCertificate, static_cast(rootCertificateLen)); VerifyOrExit(x509RootCertificate != nullptr, (result = CertificateChainValidationResult::kRootFormatInvalid, err = CHIP_ERROR_INTERNAL)); @@ -1560,14 +1564,14 @@ CHIP_ERROR ValidateCertificateChain(const uint8_t * rootCertificate, size_t root VerifyOrExit(x509CACertificate != nullptr, (result = CertificateChainValidationResult::kICAFormatInvalid, err = CHIP_ERROR_INTERNAL)); - status = X509_STORE_add_cert(store, x509CACertificate); + status = sk_X509_push(chain, x509CACertificate); VerifyOrExit(status == 1, (result = CertificateChainValidationResult::kInternalFrameworkError, err = CHIP_ERROR_INTERNAL)); x509LeafCertificate = d2i_X509(nullptr, &leafCertificate, static_cast(leafCertificateLen)); VerifyOrExit(x509LeafCertificate != nullptr, (result = CertificateChainValidationResult::kLeafFormatInvalid, err = CHIP_ERROR_INTERNAL)); - status = X509_STORE_CTX_init(verifyCtx, store, x509LeafCertificate, nullptr); + status = X509_STORE_CTX_init(verifyCtx, store, x509LeafCertificate, chain); VerifyOrExit(status == 1, (result = CertificateChainValidationResult::kInternalFrameworkError, err = CHIP_ERROR_INTERNAL)); status = X509_verify_cert(verifyCtx); @@ -1580,6 +1584,7 @@ CHIP_ERROR ValidateCertificateChain(const uint8_t * rootCertificate, size_t root X509_free(x509LeafCertificate); X509_free(x509CACertificate); X509_free(x509RootCertificate); + sk_X509_free(chain); X509_STORE_CTX_free(verifyCtx); X509_STORE_free(store); diff --git a/src/crypto/tests/CHIPCryptoPALTest.cpp b/src/crypto/tests/CHIPCryptoPALTest.cpp index de70d7644f7cdb..588af82a6d545d 100644 --- a/src/crypto/tests/CHIPCryptoPALTest.cpp +++ b/src/crypto/tests/CHIPCryptoPALTest.cpp @@ -1901,6 +1901,23 @@ static void TestX509_CertChainValidation(nlTestSuite * inSuite, void * inContext leaf_cert.data(), leaf_cert.size(), chainValidationResult); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_CERT_NOT_TRUSTED); NL_TEST_ASSERT(inSuite, chainValidationResult == CertificateChainValidationResult::kChainInvalid); + + // Now test with a Root certificate that does not correspond to the chain + ByteSpan wrong_root_cert; + err = GetTestCert(TestCert::kRoot02, TestCertLoadFlags::kDERForm, wrong_root_cert); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = ValidateCertificateChain(wrong_root_cert.data(), wrong_root_cert.size(), ica_cert.data(), ica_cert.size(), + leaf_cert.data(), leaf_cert.size(), chainValidationResult); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_CERT_NOT_TRUSTED); + NL_TEST_ASSERT(inSuite, chainValidationResult == CertificateChainValidationResult::kChainInvalid); + + // Now test with a Root certificate that does not correspond to the chain. + // Trying to validate ICA (as a leaf) which chains to Root - should fail bacause Root is loaded as untrusted intermediate cert. + err = ValidateCertificateChain(wrong_root_cert.data(), wrong_root_cert.size(), root_cert.data(), root_cert.size(), + ica_cert.data(), ica_cert.size(), chainValidationResult); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_CERT_NOT_TRUSTED); + NL_TEST_ASSERT(inSuite, chainValidationResult == CertificateChainValidationResult::kChainInvalid); } static void TestX509_IssuingTimestampValidation(nlTestSuite * inSuite, void * inContext) @@ -2138,15 +2155,12 @@ static void TestVIDPID_StringExtraction(nlTestSuite * inSuite, void * inContext) }; // clang-format on - int i = 1; for (const auto & testCase : kTestCases) { - fprintf(stderr, "DEBUG 01 i = %d\n", i); AttestationCertVidPid vidpid; AttestationCertVidPid vidpidFromCN; AttestationCertVidPid vidpidToCheck; CHIP_ERROR result = ExtractVIDPIDFromAttributeString(testCase.attrType, testCase.attr, vidpid, vidpidFromCN); - fprintf(stderr, "DEBUG 02 i = %d equal = %d\n", i++, (result == testCase.expectedResult)); NL_TEST_ASSERT(inSuite, result == testCase.expectedResult); if (testCase.attrType == DNAttrType::kMatterVID || testCase.attrType == DNAttrType::kMatterPID) @@ -2214,13 +2228,10 @@ static void TestVIDPID_x509Extraction(nlTestSuite * inSuite, void * inContext) { kOpCertNoVID, false, false, chip::VendorId::NotSpecified, 0x0000, CHIP_NO_ERROR }, }; - int i = 1; for (const auto & testCase : kTestCases) { - fprintf(stderr, "DEBUG 01 i = %d\n", i); AttestationCertVidPid vidpid; CHIP_ERROR result = ExtractVIDPIDFromX509Cert(testCase.cert, vidpid); - fprintf(stderr, "DEBUG 02 i = %d equal = %d\n", i++, (result == testCase.expectedResult)); NL_TEST_ASSERT(inSuite, result == testCase.expectedResult); NL_TEST_ASSERT(inSuite, vidpid.mVendorId.HasValue() == testCase.expectedVidPresent); NL_TEST_ASSERT(inSuite, vidpid.mProductId.HasValue() == testCase.expectedPidPresent);