Skip to content

Commit

Permalink
Fix PAA pathlen check to match spec (#27207)
Browse files Browse the repository at this point in the history 
* Fix PAA pathlen check

Problem:
- Since Matter 1.1, PAAs are allowed to omit the pathlen optional
  field to basic constraints extension. The code did not do that
- Fixes #27194

Changes in this PR:

- Update all CryptoPAL backends to fix the path length checks
- Added unit tests covering valid and invalid basic constraints
  around path lengths.

Testing done:

- Unit tests pass on both mbedTLS and BoringSSL/OpenSSL CryptoPAL

* Restyled by clang-format

* Address review comments

* Address review comments

* Improve logic with help from @bzbarsky-apple

* Restyled by clang-format

---------

Co-authored-by: [email protected] <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
  • Loading branch information
@tcarmelveilleux @tennessee-google
@restyled-commits @pull
3 people authored and pull[bot] committed Oct 20, 2023
1 parent a634a4c commit 1b2b211
Show file tree
Hide file tree
Showing 10 changed files with 268 additions and 37 deletions.
3 changes: 2 additions & 1 deletion src/crypto/CHIPCryptoPALOpenSSL.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1653,7 +1653,8 @@ CHIP_ERROR VerifyAttestationCertificateFormat(const ByteSpan & cert, Attestation
}
else
{
VerifyOrExit(isCA && (pathLen == -1 || pathLen == 0 || pathLen == 1), err = CHIP_ERROR_INTERNAL);
// For PAA, pathlen must be absent or equal to 1 (see Matter 1.1 spec 6.2.2.5)
VerifyOrExit(isCA && (pathLen == -1 || pathLen == 1), err = CHIP_ERROR_INTERNAL);
}
}
break;
Expand Down
18 changes: 12 additions & 6 deletions src/crypto/CHIPCryptoPALPSA.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1295,9 +1295,8 @@ CHIP_ERROR VerifyAttestationCertificateFormat(const ByteSpan & cert, Attestation

if (OID_CMP(sOID_Extension_BasicConstraints, extOID))
{
int isCA = 0;
int pathLen = -1;
unsigned char * seqStart = p;
int isCA = 0;
int pathLen = -1;

VerifyOrExit(extCritical, error = CHIP_ERROR_INTERNAL);
extBasicPresent = true;
Expand All @@ -1306,11 +1305,17 @@ CHIP_ERROR VerifyAttestationCertificateFormat(const ByteSpan & cert, Attestation
VerifyOrExit(result == 0, error = CHIP_ERROR_INTERNAL);
if (len > 0)
{
result = mbedtls_asn1_get_bool(&p, end, &isCA);
unsigned char * seqStart = p;
result = mbedtls_asn1_get_bool(&p, end, &isCA);
VerifyOrExit(result == 0 || result == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG, error = CHIP_ERROR_INTERNAL);

if (p != seqStart + len)
// Check if pathLen is there by validating if the cursor didn't get to the end of
// of the internal SEQUENCE for the basic constraints encapsulation.
// Missing pathLen optional tag will leave pathLen == -1 for following checks.
bool hasPathLen = (p != (seqStart + len));
if (hasPathLen)
{
// Extract pathLen value, making sure it's a valid format.
result = mbedtls_asn1_get_int(&p, end, &pathLen);
VerifyOrExit(result == 0, error = CHIP_ERROR_INTERNAL);
}
Expand All @@ -1326,7 +1331,8 @@ CHIP_ERROR VerifyAttestationCertificateFormat(const ByteSpan & cert, Attestation
}
else
{
VerifyOrExit(isCA && (pathLen == -1 || pathLen == 0 || pathLen == 1), error = CHIP_ERROR_INTERNAL);
// For PAA, pathlen must be absent or equal to 1 (see Matter 1.1 spec 6.2.2.5)
VerifyOrExit(isCA && (pathLen == -1 || pathLen == 1), error = CHIP_ERROR_INTERNAL);
}
}
else if (OID_CMP(sOID_Extension_KeyUsage, extOID))
Expand Down
18 changes: 12 additions & 6 deletions src/crypto/CHIPCryptoPALmbedTLS.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1386,9 +1386,8 @@ CHIP_ERROR VerifyAttestationCertificateFormat(const ByteSpan & cert, Attestation

if (OID_CMP(sOID_Extension_BasicConstraints, extOID))
{
int isCA = 0;
int pathLen = -1;
unsigned char * seqStart = p;
int isCA = 0;
int pathLen = -1;

VerifyOrExit(extCritical, error = CHIP_ERROR_INTERNAL);
extBasicPresent = true;
Expand All @@ -1397,11 +1396,17 @@ CHIP_ERROR VerifyAttestationCertificateFormat(const ByteSpan & cert, Attestation
VerifyOrExit(result == 0, error = CHIP_ERROR_INTERNAL);
if (len > 0)
{
result = mbedtls_asn1_get_bool(&p, end, &isCA);
unsigned char * seqStart = p;
result = mbedtls_asn1_get_bool(&p, end, &isCA);
VerifyOrExit(result == 0 || result == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG, error = CHIP_ERROR_INTERNAL);

if (p != seqStart + len)
// Check if pathLen is there by validating if the cursor didn't get to the end of
// of the internal SEQUENCE for the basic constraints encapsulation.
// Missing pathLen optional tag will leave pathLen == -1 for following checks.
bool hasPathLen = (p != (seqStart + len));
if (hasPathLen)
{
// Extract pathLen value, making sure it's a valid format.
result = mbedtls_asn1_get_int(&p, end, &pathLen);
VerifyOrExit(result == 0, error = CHIP_ERROR_INTERNAL);
}
Expand All @@ -1417,7 +1422,8 @@ CHIP_ERROR VerifyAttestationCertificateFormat(const ByteSpan & cert, Attestation
}
else
{
VerifyOrExit(isCA && (pathLen == -1 || pathLen == 0 || pathLen == 1), error = CHIP_ERROR_INTERNAL);
// For PAA, pathlen must be absent or equal to 1 (see Matter 1.1 spec 6.2.2.5)
VerifyOrExit(isCA && (pathLen == -1 || pathLen == 1), error = CHIP_ERROR_INTERNAL);
}
}
else if (OID_CMP(sOID_Extension_KeyUsage, extOID))
Expand Down
1 change: 1 addition & 0 deletions src/crypto/tests/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ chip_test_suite("tests") {
sources = [
"AES_CCM_128_test_vectors.h",
"CHIPCryptoPALTest.cpp",
"DacValidationExplicitVectors.h",
"DerSigConversion_test_vectors.h",
"ECDH_P256_test_vectors.h",
"HKDF_SHA256_test_vectors.h",
Expand Down
13 changes: 13 additions & 0 deletions src/crypto/tests/CHIPCryptoPALTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ class HeapChecker
};
#endif

#include "DacValidationExplicitVectors.h"

} // namespace

static uint32_t gs_test_entropy_source_called = 0;
Expand Down Expand Up @@ -1956,14 +1958,25 @@ static void TestX509_VerifyAttestationCertificateFormat(nlTestSuite * inSuite, v
{ ByteSpan(), Crypto::AttestationCertType::kDAC, CHIP_ERROR_INVALID_ARGUMENT },
{ sTestCert_PAI_FFF2_NoPID_FB_Cert, Crypto::AttestationCertType::kDAC, CHIP_ERROR_INTERNAL },
{ sTestCert_DAC_FFF2_8006_0025_ValInFuture_Cert, Crypto::AttestationCertType::kPAA, CHIP_ERROR_INTERNAL },
{ ByteSpan{kPaaWithNoPathlen}, Crypto::AttestationCertType::kPAA, CHIP_NO_ERROR },
{ ByteSpan{kPaiPathLenMissing}, Crypto::AttestationCertType::kPAI, CHIP_ERROR_INTERNAL },
{ ByteSpan{kPaiPathLen1}, Crypto::AttestationCertType::kPAI, CHIP_ERROR_INTERNAL },
{ ByteSpan{kPaaPathLen2}, Crypto::AttestationCertType::kPAA, CHIP_ERROR_INTERNAL },
};
// clang-format on

int case_idx = 0;
for (auto & testCase : sValidationTestCases)
{
ByteSpan cert = testCase.cert;
CHIP_ERROR err = VerifyAttestationCertificateFormat(cert, testCase.type);
if (err != testCase.expectedError)
{
ChipLogError(Crypto, "Failed TestX509_VerifyAttestationCertificateFormat sub-case %d, err: %" CHIP_ERROR_FORMAT,
case_idx, err.Format());
}
NL_TEST_ASSERT(inSuite, err == testCase.expectedError);
++case_idx;
}
}

Expand Down
Loading

0 comments on commit 1b2b211

Please sign in to comment.