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

Author: vijs

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`                   |

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<CommissioningErrorInfo>())
         {

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<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 @@ 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();

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<OnDeviceConnected> mOnDeviceConnectedCallback;
     chip::Callback::Callback<OnDeviceConnectionFailure> mOnDeviceConnectionFailureCallback;
 #if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES

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";
 

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

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;

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),

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<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 });

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<OnAttestationInformationVerification> * onCompletion) override;
+
     CsaCdKeysTrustStore * GetCertificationDeclarationTrustStore() override { return &mCdKeysTrustStore; }
 
 protected:

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<OnAttestationInformationVerification> * onCompletion) override
+    {
+        (void) info;
+        (void) onCompletion;
+        VerifyOrDie(false);
+    }
 };
 
 // Default to avoid nullptr on getter and cleanly handle new products/clients before

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<OnAttestationInformationVerification> * onCompletion) = 0;
+
     /**
      * @brief Get the trust store used for the attestation verifier.
      *

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;

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

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,

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):