Skip to content

Commit

Permalink
Fix OpenSSL Implementation of ValidateCertificateChain().
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@emargolis
emargolis committed May 6, 2022
1 parent 8faa780 commit daf6d4f
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 8 deletions.
9 changes: 7 additions & 2 deletions src/crypto/CHIPCryptoPALOpenSSL.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1524,6 +1524,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;
Expand All @@ -1543,6 +1544,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<long>(rootCertificateLen));
VerifyOrExit(x509RootCertificate != nullptr,
(result = CertificateChainValidationResult::kRootFormatInvalid, err = CHIP_ERROR_INTERNAL));
Expand All @@ -1554,14 +1558,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<long>(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);
Expand All @@ -1574,6 +1578,7 @@ CHIP_ERROR ValidateCertificateChain(const uint8_t * rootCertificate, size_t root
X509_free(x509LeafCertificate);
X509_free(x509CACertificate);
X509_free(x509RootCertificate);
sk_X509_pop_free(chain, X509_free);
X509_STORE_CTX_free(verifyCtx);
X509_STORE_free(store);

Expand Down
23 changes: 17 additions & 6 deletions src/crypto/tests/CHIPCryptoPALTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit daf6d4f

Please sign in to comment.