Skip to content

Commit

Permalink
Remove unused OpenSSL-only PKCS7 operations (#18089)
Browse files Browse the repository at this point in the history 
- There are no callsites for `LoadCertsFromPKCS7` and associated
  PKCS7 primitives
- These deal with DER certificates, in contexts where common Matter
  code would never make use of them. At best it would be used inside
  NOC issuing logic if, internally, a commissioner received chains
  in PKCS7 format
- The code is only available on OpenSSL backend, and this is the
  only feature not available on mbedTLS as well.
- The code makes use of unsafe casts and omits error checking, e.g.

```
bytes_written = static_cast<size_t>(i2d_X509(sk_X509_value(certs, static_cast<int>(i)), pX509ListAux));
```
  which ignores errors from i2d_X509 that could return -1, which could
  cause invalid offsets or other semantic errors.

Overall, given that the code has risks in its usage, and is not
available on mbedTLS backend, and is never used by common code,
it's just easier to remove it from the SDK and let the organizations
who need it make use of it.

If the code is reinstated, the unsafe use of `static_cast` and the ignoring
of error values would need to be fixed in the PKCS7 code.

Testing done:
- Unit tests still pass after removal
  • Loading branch information
@tcarmelveilleux @pull
tcarmelveilleux authored and pull[bot] committed Oct 10, 2023
1 parent 693454e commit 2304656
Show file tree
Hide file tree
Showing 5 changed files with 0 additions and 343 deletions.
6 changes: 0 additions & 6 deletions src/crypto/CHIPCryptoPAL.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1352,12 +1352,6 @@ CHIP_ERROR GenerateCompressedFabricId(const Crypto::P256PublicKey & rootPublicKe

typedef CapacityBoundBuffer<kMax_x509_Certificate_Length> X509DerCertificate;

CHIP_ERROR LoadCertsFromPKCS7(const char * pkcs7, X509DerCertificate * x509list, uint32_t * max_certs);

CHIP_ERROR LoadCertFromPKCS7(const char * pkcs7, X509DerCertificate * x509list, uint32_t n_cert);

CHIP_ERROR GetNumberOfCertsFromPKCS7(const char * pkcs7, uint32_t * n_certs);

enum class CertificateChainValidationResult
{
kSuccess = 0,
Expand Down
148 changes: 0 additions & 148 deletions src/crypto/CHIPCryptoPALOpenSSL.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1515,154 +1515,6 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::PointIsValid(void * R)
return CHIP_NO_ERROR;
}

static void security_free_cert_list(X509_LIST * certs)
{
if (certs)
{
sk_X509_pop_free(certs, X509_free);
}
}

CHIP_ERROR LoadCertsFromPKCS7(const char * pkcs7, X509DerCertificate * x509list, uint32_t * max_certs)
{
CHIP_ERROR err = CHIP_NO_ERROR;
X509_LIST * certs = nullptr;
BIO * bio_cert = nullptr;
PKCS7 * p7 = nullptr;
int p7_type = 0;

VerifyOrExit(x509list != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrExit(max_certs != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT);

bio_cert = BIO_new_mem_buf(pkcs7, -1);

p7 = PEM_read_bio_PKCS7(bio_cert, nullptr, nullptr, nullptr);
VerifyOrExit(p7 != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE);

p7_type = OBJ_obj2nid(p7->type);
if (p7_type == NID_pkcs7_signed)
{
certs = p7->d.sign->cert;
}
else if (p7_type == NID_pkcs7_signedAndEnveloped)
{
certs = p7->d.signed_and_enveloped->cert;
}

VerifyOrExit(certs != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE);
VerifyOrExit(static_cast<uint32_t>(sk_X509_num(certs)) <= *max_certs, err = CHIP_ERROR_WRONG_CERT_TYPE);

*max_certs = static_cast<uint32_t>(sk_X509_num(certs));

certs = X509_chain_up_ref(certs);

for (uint32_t i = 0; i < *max_certs; ++i)
{
size_t bytes_written = 0;
unsigned char * pX509ListEnd = x509list[i];
unsigned char ** pX509ListAux = &pX509ListEnd;

bytes_written = static_cast<size_t>(i2d_X509(sk_X509_value(certs, static_cast<int>(i)), pX509ListAux));

VerifyOrExit(bytes_written <= x509list[i].Capacity(), err = CHIP_ERROR_NO_MEMORY);

x509list[i].SetLength(bytes_written);
}

exit:
BIO_free_all(bio_cert);
PKCS7_free(p7);
security_free_cert_list(certs);

return err;
}

CHIP_ERROR LoadCertFromPKCS7(const char * pkcs7, X509DerCertificate * x509list, uint32_t n_cert)
{
CHIP_ERROR err = CHIP_NO_ERROR;
X509_LIST * certs = nullptr;
BIO * bio_cert = nullptr;
PKCS7 * p7 = nullptr;
int p7_type = 0;

VerifyOrExit(x509list != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT);

bio_cert = BIO_new_mem_buf(pkcs7, -1);

p7 = PEM_read_bio_PKCS7(bio_cert, nullptr, nullptr, nullptr);
VerifyOrExit(p7 != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE);

p7_type = OBJ_obj2nid(p7->type);
if (p7_type == NID_pkcs7_signed)
{
certs = p7->d.sign->cert;
}
else if (p7_type == NID_pkcs7_signedAndEnveloped)
{
certs = p7->d.signed_and_enveloped->cert;
}

VerifyOrExit(certs != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE);
VerifyOrExit(n_cert < static_cast<uint32_t>(sk_X509_num(certs)), err = CHIP_ERROR_INVALID_ARGUMENT);

certs = X509_chain_up_ref(certs);

{
size_t bytes_written = 0;
unsigned char * pX509ListEnd = reinterpret_cast<unsigned char *>(x509list);
unsigned char ** pX509ListAux = &pX509ListEnd;

bytes_written = static_cast<size_t>(i2d_X509(sk_X509_value(certs, static_cast<int>(n_cert)), pX509ListAux));

VerifyOrExit(bytes_written <= x509list->Capacity(), err = CHIP_ERROR_NO_MEMORY);

x509list->SetLength(bytes_written);
}

exit:
BIO_free_all(bio_cert);
PKCS7_free(p7);
security_free_cert_list(certs);

return err;
}

CHIP_ERROR GetNumberOfCertsFromPKCS7(const char * pkcs7, uint32_t * n_certs)
{
CHIP_ERROR err = CHIP_NO_ERROR;
X509_LIST * certs = nullptr;
BIO * bio_cert = nullptr;
PKCS7 * p7 = nullptr;
int p7_type = 0;

VerifyOrExit(n_certs != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT);

bio_cert = BIO_new_mem_buf(pkcs7, -1);

p7 = PEM_read_bio_PKCS7(bio_cert, nullptr, nullptr, nullptr);
VerifyOrExit(p7 != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE);

p7_type = OBJ_obj2nid(p7->type);
if (p7_type == NID_pkcs7_signed)
{
certs = p7->d.sign->cert;
}
else if (p7_type == NID_pkcs7_signedAndEnveloped)
{
certs = p7->d.signed_and_enveloped->cert;
}

VerifyOrExit(certs != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE);

*n_certs = static_cast<uint32_t>(sk_X509_num(certs));

exit:
BIO_free_all(bio_cert);
PKCS7_free(p7);

return err;
}

CHIP_ERROR ValidateCertificateChain(const uint8_t * rootCertificate, size_t rootCertificateLen, const uint8_t * caCertificate,
size_t caCertificateLen, const uint8_t * leafCertificate, size_t leafCertificateLen,
CertificateChainValidationResult & result)
Expand Down
15 changes: 0 additions & 15 deletions src/crypto/CHIPCryptoPALmbedTLS.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1263,21 +1263,6 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::PointIsValid(void * R)
return CHIP_NO_ERROR;
}

CHIP_ERROR LoadCertsFromPKCS7(const char * pkcs7, X509DerCertificate * x509list, uint32_t * max_certs)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}

CHIP_ERROR LoadCertFromPKCS7(const char * pkcs7, X509DerCertificate * x509list, uint32_t n_cert)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}

CHIP_ERROR GetNumberOfCertsFromPKCS7(const char * pkcs7, uint32_t * n_certs)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}

CHIP_ERROR ValidateCertificateChain(const uint8_t * rootCertificate, size_t rootCertificateLen, const uint8_t * caCertificate,
size_t caCertificateLen, const uint8_t * leafCertificate, size_t leafCertificateLen,
CertificateChainValidationResult & result)
Expand Down
29 changes: 0 additions & 29 deletions src/crypto/tests/CHIPCryptoPALTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,6 @@

#include <lib/support/BytesToHex.h>

#if CHIP_CRYPTO_OPENSSL
#include "X509_PKCS7Extraction_test_vectors.h"
#endif

#if CHIP_CRYPTO_MBEDTLS
#include <mbedtls/memory_buffer_alloc.h>
#endif
Expand Down Expand Up @@ -1828,28 +1824,6 @@ static void TestCompressedFabricIdentifier(nlTestSuite * inSuite, void * inConte
NL_TEST_ASSERT(inSuite, error == CHIP_ERROR_INVALID_ARGUMENT);
}

#if CHIP_CRYPTO_OPENSSL
static void TestX509_PKCS7Extraction(nlTestSuite * inSuite, void * inContext)
{
CHIP_ERROR err = CHIP_NO_ERROR;
int status = 0;
X509DerCertificate x509list[3];
uint32_t max_certs = sizeof(x509list) / sizeof(X509DerCertificate);

err = LoadCertsFromPKCS7(pem_pkcs7_blob, x509list, &max_certs);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

status = memcmp(certificate_blob_leaf, x509list[0], x509list[0].Length());
NL_TEST_ASSERT(inSuite, status == 0);

status = memcmp(certificate_blob_intermediate, x509list[1], x509list[1].Length());
NL_TEST_ASSERT(inSuite, status == 0);

status = memcmp(certificate_blob_root, x509list[2], x509list[2].Length());
NL_TEST_ASSERT(inSuite, status == 0);
}
#endif // CHIP_CRYPTO_OPENSSL

static void TestPubkey_x509Extraction(nlTestSuite * inSuite, void * inContext)
{
using namespace TestCerts;
Expand Down Expand Up @@ -2391,9 +2365,6 @@ static const nlTest sTests[] = {
NL_TEST_DEF("Test Spake2+ object reuse", TestSPAKE2P_Reuse),
NL_TEST_DEF("Test compressed fabric identifier", TestCompressedFabricIdentifier),
NL_TEST_DEF("Test Pubkey Extraction from x509 Certificate", TestPubkey_x509Extraction),
#if CHIP_CRYPTO_OPENSSL
NL_TEST_DEF("Test x509 Certificate Extraction from PKCS7", TestX509_PKCS7Extraction),
#endif // CHIP_CRYPTO_OPENSSL
NL_TEST_DEF("Test x509 Certificate Chain Validation", TestX509_CertChainValidation),
NL_TEST_DEF("Test x509 Certificate Timestamp Validation", TestX509_IssuingTimestampValidation),
NL_TEST_DEF("Test Subject Key Id Extraction from x509 Certificate", TestSKID_x509Extraction),
Expand Down
Loading

0 comments on commit 2304656

Please sign in to comment.