From be9389ced25be6aa51398a9e3697dff113c5992e Mon Sep 17 00:00:00 2001 From: Vijay Selvaraj Date: Thu, 9 May 2024 15:05:38 -0400 Subject: [PATCH] Added an interface for revocation checks to DeviceAttestationVerifier (#31222) * Added an interface for revocation checks to DeviceAttestationVerifier * Addressed review comments * Removed the changes to add revocation set support to chip-tool and it will be submitted as a followup PR * Implemented PR #31222 review comments * Fixed issues in revocation check and code cleanup --- docs/ERROR_CODES.md | 1 + src/controller/AutoCommissioner.cpp | 9 +++ src/controller/CHIPDeviceController.cpp | 73 ++++++++++++++++++- src/controller/CHIPDeviceController.h | 10 +++ src/controller/CommissioningDelegate.cpp | 3 + src/controller/CommissioningDelegate.h | 2 + src/controller/python/OpCredsBinding.cpp | 2 + .../commissioning_failure_test.py | 4 +- .../DefaultDeviceAttestationVerifier.cpp | 10 +++ .../DefaultDeviceAttestationVerifier.h | 3 + .../DeviceAttestationVerifier.cpp | 8 ++ .../DeviceAttestationVerifier.h | 10 +++ src/lib/core/CHIPError.cpp | 3 + src/lib/core/CHIPError.h | 9 ++- src/lib/core/tests/TestCHIPErrorStr.cpp | 1 + src/python_testing/TC_CGEN_2_4.py | 6 +- 16 files changed, 144 insertions(+), 10 deletions(-) diff --git a/docs/ERROR_CODES.md b/docs/ERROR_CODES.md index ecfda40d943983..5799c7a3f1b885 100644 --- a/docs/ERROR_CODES.md +++ b/docs/ERROR_CODES.md @@ -47,6 +47,7 @@ This file was **AUTOMATICALLY** generated by | 29 | 0x1D | `CHIP_ERROR_INVALID_IPK` | | 30 | 0x1E | `CHIP_ERROR_INVALID_STRING_LENGTH` | | 31 | 0x1F | `CHIP_ERROR_INVALID_LIST_LENGTH` | +| 32 | 0x20 | `CHIP_ERROR_FAILED_DEVICE_ATTESTATION` | | 33 | 0x21 | `CHIP_ERROR_END_OF_TLV` | | 34 | 0x22 | `CHIP_ERROR_TLV_UNDERRUN` | | 35 | 0x23 | `CHIP_ERROR_INVALID_TLV_ELEMENT` | diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index a59ff82a551f1d..5b37988a491260 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -389,6 +389,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio case CommissioningStage::kSendAttestationRequest: return CommissioningStage::kAttestationVerification; case CommissioningStage::kAttestationVerification: + return CommissioningStage::kAttestationRevocationCheck; + case CommissioningStage::kAttestationRevocationCheck: return CommissioningStage::kSendOpCertSigningRequest; case CommissioningStage::kSendOpCertSigningRequest: return CommissioningStage::kValidateCSR; @@ -693,6 +695,13 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio "Failed device attestation. Device vendor and/or product ID do not match the IDs expected. " "Verify DAC certificate chain and certification declaration to ensure spec rules followed."); } + + if (report.stageCompleted == CommissioningStage::kAttestationVerification) + { + ChipLogError(Controller, "Failed verifying attestation information. Now checking DAC chain revoked status."); + // don't error out until we check for DAC chain revocation status + err = CHIP_NO_ERROR; + } } else if (report.Is()) { diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index bb1143265945c2..910c0b21040c96 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -947,7 +947,7 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de return CHIP_ERROR_INCORRECT_STATE; } - if (mCommissioningStage != CommissioningStage::kAttestationVerification) + if (mCommissioningStage != CommissioningStage::kAttestationRevocationCheck) { ChipLogError(Controller, "Commissioning is not attestation verification phase"); return CHIP_ERROR_INCORRECT_STATE; @@ -1175,6 +1175,17 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification( MATTER_TRACE_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner"); DeviceCommissioner * commissioner = reinterpret_cast(context); + if (commissioner->mCommissioningStage == CommissioningStage::kAttestationVerification) + { + // Check for revoked DAC Chain before calling delegate. Enter next stage. + + CommissioningDelegate::CommissioningReport report; + report.Set(result); + + return commissioner->CommissioningStageComplete( + result == AttestationVerificationResult::kSuccess ? CHIP_NO_ERROR : CHIP_ERROR_INTERNAL, report); + } + if (!commissioner->mDeviceBeingCommissioned) { ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device"); @@ -1184,6 +1195,15 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification( auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters(); Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate(); + if (params.GetCompletionStatus().attestationResult.HasValue()) + { + auto previousResult = params.GetCompletionStatus().attestationResult.Value(); + if (previousResult != AttestationVerificationResult::kSuccess) + { + result = previousResult; + } + } + if (result != AttestationVerificationResult::kSuccess) { CommissioningDelegate::CommissioningReport report; @@ -1398,6 +1418,18 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const Credentials::Device return CHIP_NO_ERROR; } +CHIP_ERROR +DeviceCommissioner::CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info) +{ + MATTER_TRACE_SCOPE("CheckForRevokedDACChain", "DeviceCommissioner"); + VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mDeviceAttestationVerifier != nullptr, CHIP_ERROR_INCORRECT_STATE); + + mDeviceAttestationVerifier->CheckForRevokedDACChain(info, &mDeviceAttestationInformationVerificationCallback); + + return CHIP_NO_ERROR; +} + CHIP_ERROR DeviceCommissioner::ValidateCSR(DeviceProxy * proxy, const ByteSpan & NOCSRElements, const ByteSpan & AttestationSignature, const ByteSpan & dac, const ByteSpan & csrNonce) { @@ -3037,9 +3069,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } case CommissioningStage::kAttestationVerification: { ChipLogProgress(Controller, "Verifying attestation"); - if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() || - !params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() || - !params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue()) + if (IsAttestationInformationMissing(params)) { ChipLogError(Controller, "Missing attestation information"); CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); @@ -3055,9 +3085,32 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio if (ValidateAttestationInfo(info) != CHIP_NO_ERROR) { ChipLogError(Controller, "Error validating attestation information"); + CommissioningStageComplete(CHIP_ERROR_FAILED_DEVICE_ATTESTATION); + return; + } + } + break; + case CommissioningStage::kAttestationRevocationCheck: { + ChipLogProgress(Controller, "Verifying device's DAC chain revocation status"); + if (IsAttestationInformationMissing(params)) + { + ChipLogError(Controller, "Missing attestation information"); CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); return; } + + DeviceAttestationVerifier::AttestationInfo info( + params.GetAttestationElements().Value(), + proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge(), + params.GetAttestationSignature().Value(), params.GetPAI().Value(), params.GetDAC().Value(), + params.GetAttestationNonce().Value(), params.GetRemoteVendorId().Value(), params.GetRemoteProductId().Value()); + + if (CheckForRevokedDACChain(info) != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Error validating device's DAC chain revocation status"); + CommissioningStageComplete(CHIP_ERROR_FAILED_DEVICE_ATTESTATION); + return; + } } break; case CommissioningStage::kSendOpCertSigningRequest: { @@ -3424,6 +3477,18 @@ void DeviceCommissioner::ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, } } +bool DeviceCommissioner::IsAttestationInformationMissing(const CommissioningParameters & params) +{ + if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() || + !params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() || + !params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue()) + { + return true; + } + + return false; +} + CHIP_ERROR DeviceController::GetCompressedFabricIdBytes(MutableByteSpan & outBytes) const { const auto * fabricInfo = GetFabricInfo(); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 42b8a7e2481541..8bef525e9cd773 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -1000,6 +1000,14 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, */ CHIP_ERROR ProcessCertificateChain(const ByteSpan & certificate); + /** + * @brief + * This function validates the revocation status of the DAC Chain sent by the device. + * + * @param[in] info Structure contatining all the required information for validating the device attestation. + */ + CHIP_ERROR CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info); + void HandleAttestationResult(CHIP_ERROR err); CommissioneeDeviceProxy * FindCommissioneeDevice(NodeId id); @@ -1053,6 +1061,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, // extend it). void ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, CommissioningParameters & params, CommissioningStage step); + bool IsAttestationInformationMissing(const CommissioningParameters & params); + chip::Callback::Callback mOnDeviceConnectedCallback; chip::Callback::Callback mOnDeviceConnectionFailureCallback; #if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES diff --git a/src/controller/CommissioningDelegate.cpp b/src/controller/CommissioningDelegate.cpp index bbe82de18080d3..b3e3b6ca2ac754 100644 --- a/src/controller/CommissioningDelegate.cpp +++ b/src/controller/CommissioningDelegate.cpp @@ -70,6 +70,9 @@ const char * StageToString(CommissioningStage stage) case kAttestationVerification: return "AttestationVerification"; + case kAttestationRevocationCheck: + return "AttestationRevocationCheck"; + case kSendOpCertSigningRequest: return "SendOpCertSigningRequest"; diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index fae510b6b17325..e79ad1d67c4bcf 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -47,6 +47,7 @@ enum CommissioningStage : uint8_t kSendDACCertificateRequest, ///< Send DAC CertificateChainRequest (0x3E:2) command to the device kSendAttestationRequest, ///< Send AttestationRequest (0x3E:0) command to the device kAttestationVerification, ///< Verify AttestationResponse (0x3E:1) validity + kAttestationRevocationCheck, ///< Verify Revocation Status of device's DAC chain kSendOpCertSigningRequest, ///< Send CSRRequest (0x3E:4) command to the device kValidateCSR, ///< Verify CSRResponse (0x3E:5) validity kGenerateNOCChain, ///< TLV encode Node Operational Credentials (NOC) chain certs @@ -782,6 +783,7 @@ class CommissioningDelegate * kSendDACCertificateRequest: RequestedCertificate * kSendAttestationRequest: AttestationResponse * kAttestationVerification: AttestationErrorInfo if there is an error + * kAttestationRevocationCheck: AttestationErrorInfo if there is an error * kSendOpCertSigningRequest: CSRResponse * kGenerateNOCChain: NocChain * kSendTrustedRootCert: None diff --git a/src/controller/python/OpCredsBinding.cpp b/src/controller/python/OpCredsBinding.cpp index 66a6c841843cde..5fd4205d4c7ce7 100644 --- a/src/controller/python/OpCredsBinding.cpp +++ b/src/controller/python/OpCredsBinding.cpp @@ -331,6 +331,8 @@ class TestCommissioner : public chip::Controller::AutoCommissioner return mNeedsDST && mParams.GetDSTOffsets().HasValue(); case chip::Controller::CommissioningStage::kError: case chip::Controller::CommissioningStage::kSecurePairing: + // "not valid" because attestation verification always fails after entering revocation check step + case chip::Controller::CommissioningStage::kAttestationVerification: return false; default: return true; diff --git a/src/controller/python/test/test_scripts/commissioning_failure_test.py b/src/controller/python/test/test_scripts/commissioning_failure_test.py index 4681dd108d758a..eca170601c7f29 100755 --- a/src/controller/python/test/test_scripts/commissioning_failure_test.py +++ b/src/controller/python/test/test_scripts/commissioning_failure_test.py @@ -96,7 +96,7 @@ def main(): # TODO: Start at stage 2 once handling for arming failsafe on pase is done. if options.report: - for testFailureStage in range(3, 20): + for testFailureStage in range(3, 21): FailIfNot(test.TestPaseOnly(ip=options.deviceAddress1, setuppin=20202021, nodeid=1), @@ -105,7 +105,7 @@ def main(): "Commissioning failure tests failed for simulated report failure on stage {}".format(testFailureStage)) else: - for testFailureStage in range(3, 20): + for testFailureStage in range(3, 21): FailIfNot(test.TestPaseOnly(ip=options.deviceAddress1, setuppin=20202021, nodeid=1), diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp index 0d7f67ff82b0f6..f3444b0c303940 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp @@ -607,6 +607,16 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa return CHIP_NO_ERROR; } +void DefaultDACVerifier::CheckForRevokedDACChain(const AttestationInfo & info, + Callback::Callback * onCompletion) +{ + AttestationVerificationResult attestationError = AttestationVerificationResult::kSuccess; + + // TODO(#33124): Implement default version of CheckForRevokedDACChain + + onCompletion->mCall(onCompletion->mContext, info, attestationError); +} + bool CsaCdKeysTrustStore::IsCdTestKey(const ByteSpan & kid) const { return kid.data_equal(ByteSpan{ gTestCdPubkeyKid }); diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h index bdf7f705dbd0f1..346d098a58a337 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h @@ -74,6 +74,9 @@ class DefaultDACVerifier : public DeviceAttestationVerifier const ByteSpan & attestationSignatureBuffer, const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) override; + void CheckForRevokedDACChain(const AttestationInfo & info, + Callback::Callback * onCompletion) override; + CsaCdKeysTrustStore * GetCertificationDeclarationTrustStore() override { return &mCdKeysTrustStore; } protected: diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp index cc5d9d3030de80..5bdbb9a8d82792 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp @@ -69,6 +69,14 @@ class UnimplementedDACVerifier : public DeviceAttestationVerifier (void) csrNonce; return CHIP_ERROR_NOT_IMPLEMENTED; } + + void CheckForRevokedDACChain(const AttestationInfo & info, + Callback::Callback * onCompletion) override + { + (void) info; + (void) onCompletion; + VerifyOrDie(false); + } }; // Default to avoid nullptr on getter and cleanly handle new products/clients before diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h index 9465d934207d1d..f45ceae06c23fe 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h @@ -386,6 +386,16 @@ class DeviceAttestationVerifier const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) = 0; + /** + * @brief Verify whether or not the given DAC chain is revoked. + * + * @param[in] info All of the information required to check for revoked DAC chain. + * @param[in] onCompletion Callback handler to provide Attestation Information Verification result to the caller of + * CheckForRevokedDACChain() + */ + virtual void CheckForRevokedDACChain(const AttestationInfo & info, + Callback::Callback * onCompletion) = 0; + /** * @brief Get the trust store used for the attestation verifier. * diff --git a/src/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp index 68a44d8c39a7ea..2b2461ca79d09c 100644 --- a/src/lib/core/CHIPError.cpp +++ b/src/lib/core/CHIPError.cpp @@ -146,6 +146,9 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err) case CHIP_ERROR_INVALID_LIST_LENGTH.AsInteger(): desc = "Invalid list length"; break; + case CHIP_ERROR_FAILED_DEVICE_ATTESTATION.AsInteger(): + desc = "Failed Device Attestation"; + break; case CHIP_END_OF_TLV.AsInteger(): desc = "End of TLV"; break; diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index 37001c8c6971a2..370a7d37d096ef 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -743,7 +743,14 @@ using CHIP_ERROR = ::chip::ChipError; */ #define CHIP_ERROR_INVALID_LIST_LENGTH CHIP_CORE_ERROR(0x1f) -// AVAILABLE: 0x20 +/** + * @def CHIP_ERROR_FAILED_DEVICE_ATTESTATION + * + * @brief + * Device Attestation failed. + * + */ +#define CHIP_ERROR_FAILED_DEVICE_ATTESTATION CHIP_CORE_ERROR(0x20) /** * @def CHIP_END_OF_TLV diff --git a/src/lib/core/tests/TestCHIPErrorStr.cpp b/src/lib/core/tests/TestCHIPErrorStr.cpp index 730a4843e14a45..54e187c72763fd 100644 --- a/src/lib/core/tests/TestCHIPErrorStr.cpp +++ b/src/lib/core/tests/TestCHIPErrorStr.cpp @@ -69,6 +69,7 @@ static const CHIP_ERROR kTestElements[] = CHIP_ERROR_UNINITIALIZED, CHIP_ERROR_INVALID_STRING_LENGTH, CHIP_ERROR_INVALID_LIST_LENGTH, + CHIP_ERROR_FAILED_DEVICE_ATTESTATION, CHIP_END_OF_TLV, CHIP_ERROR_TLV_UNDERRUN, CHIP_ERROR_INVALID_TLV_ELEMENT, diff --git a/src/python_testing/TC_CGEN_2_4.py b/src/python_testing/TC_CGEN_2_4.py index 23ab28ef09fad9..7bb2e6d21d3d73 100644 --- a/src/python_testing/TC_CGEN_2_4.py +++ b/src/python_testing/TC_CGEN_2_4.py @@ -34,9 +34,9 @@ kSendPAICertificateRequest = 10 kSendDACCertificateRequest = 11 kSendAttestationRequest = 12 -kSendOpCertSigningRequest = 14 -kSendTrustedRootCert = 17 -kSendNOC = 18 +kSendOpCertSigningRequest = 15 +kSendTrustedRootCert = 18 +kSendNOC = 19 class TC_CGEN_2_4(MatterBaseTest):