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
diff --git a/docs/ERROR_CODES.md b/docs/ERROR_CODES.md
index ecfda40..5799c7a 100644
--- a/docs/ERROR_CODES.md
+++ b/docs/ERROR_CODES.md
@@ -47,6 +47,7 @@
| 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 a59ff82..5b37988 100644
--- a/src/controller/AutoCommissioner.cpp
+++ b/src/controller/AutoCommissioner.cpp
@@ -389,6 +389,8 @@
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 @@
"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<CommissioningErrorInfo>())
{
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
index bb11432..910c0b2 100644
--- a/src/controller/CHIPDeviceController.cpp
+++ b/src/controller/CHIPDeviceController.cpp
@@ -947,7 +947,7 @@
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 @@
MATTER_TRACE_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner");
DeviceCommissioner * commissioner = reinterpret_cast<DeviceCommissioner *>(context);
+ if (commissioner->mCommissioningStage == CommissioningStage::kAttestationVerification)
+ {
+ // Check for revoked DAC Chain before calling delegate. Enter next stage.
+
+ CommissioningDelegate::CommissioningReport report;
+ report.Set<AttestationErrorInfo>(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 @@
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 @@
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 @@
}
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 @@
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 @@
}
}
+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 42b8a7e..8bef525 100644
--- a/src/controller/CHIPDeviceController.h
+++ b/src/controller/CHIPDeviceController.h
@@ -1000,6 +1000,14 @@
*/
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 @@
// extend it).
void ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, CommissioningParameters & params, CommissioningStage step);
+ bool IsAttestationInformationMissing(const CommissioningParameters & params);
+
chip::Callback::Callback<OnDeviceConnected> mOnDeviceConnectedCallback;
chip::Callback::Callback<OnDeviceConnectionFailure> mOnDeviceConnectionFailureCallback;
#if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
diff --git a/src/controller/CommissioningDelegate.cpp b/src/controller/CommissioningDelegate.cpp
index bbe82de..b3e3b6c 100644
--- a/src/controller/CommissioningDelegate.cpp
+++ b/src/controller/CommissioningDelegate.cpp
@@ -70,6 +70,9 @@
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 fae510b..e79ad1d 100644
--- a/src/controller/CommissioningDelegate.h
+++ b/src/controller/CommissioningDelegate.h
@@ -47,6 +47,7 @@
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 @@
* 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 66a6c84..5fd4205 100644
--- a/src/controller/python/OpCredsBinding.cpp
+++ b/src/controller/python/OpCredsBinding.cpp
@@ -331,6 +331,8 @@
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 4681dd1..eca1706 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 @@
# 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 @@
"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 0d7f67f..f3444b0 100644
--- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp
+++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp
@@ -607,6 +607,16 @@
return CHIP_NO_ERROR;
}
+void DefaultDACVerifier::CheckForRevokedDACChain(const AttestationInfo & info,
+ Callback::Callback<OnAttestationInformationVerification> * 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 bdf7f70..346d098 100644
--- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h
+++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h
@@ -74,6 +74,9 @@
const ByteSpan & attestationSignatureBuffer,
const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) override;
+ void CheckForRevokedDACChain(const AttestationInfo & info,
+ Callback::Callback<OnAttestationInformationVerification> * 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 cc5d9d3..5bdbb9a 100644
--- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp
+++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp
@@ -69,6 +69,14 @@
(void) csrNonce;
return CHIP_ERROR_NOT_IMPLEMENTED;
}
+
+ void CheckForRevokedDACChain(const AttestationInfo & info,
+ Callback::Callback<OnAttestationInformationVerification> * 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 9465d93..f45ceae 100644
--- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h
+++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h
@@ -387,6 +387,16 @@
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<OnAttestationInformationVerification> * onCompletion) = 0;
+
+ /**
* @brief Get the trust store used for the attestation verifier.
*
* Returns nullptr if not supported. Be careful not to hold-on to the trust store
diff --git a/src/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp
index 68a44d8..2b2461c 100644
--- a/src/lib/core/CHIPError.cpp
+++ b/src/lib/core/CHIPError.cpp
@@ -146,6 +146,9 @@
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 37001c8..370a7d3 100644
--- a/src/lib/core/CHIPError.h
+++ b/src/lib/core/CHIPError.h
@@ -743,7 +743,14 @@
*/
#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 730a484..54e187c 100644
--- a/src/lib/core/tests/TestCHIPErrorStr.cpp
+++ b/src/lib/core/tests/TestCHIPErrorStr.cpp
@@ -69,6 +69,7 @@
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 23ab28e..7bb2e6d 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):
