Skip to content

Commit

Permalink
Fix OpenSSL Implementation of ValidateCertificateChain(). (#18125)
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 authored May 10, 2022
1 parent 395375d commit f503636
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 @@ -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;
Expand All @@ -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<long>(rootCertificateLen));
VerifyOrExit(x509RootCertificate != nullptr,
(result = CertificateChainValidationResult::kRootFormatInvalid, err = CHIP_ERROR_INTERNAL));
Expand All @@ -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<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 @@ -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);

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 f503636

Please sign in to comment.