Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added an interface for revocation checks to DeviceAttestationVerifier #31222

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/ERROR_CODES.md
Original file line number Diff line number Diff line change
@@ -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` |
9 changes: 9 additions & 0 deletions src/controller/AutoCommissioner.cpp
Original file line number Diff line number Diff line change
@@ -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>())
{
73 changes: 69 additions & 4 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
@@ -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();
10 changes: 10 additions & 0 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
@@ -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
3 changes: 3 additions & 0 deletions src/controller/CommissioningDelegate.cpp
Original file line number Diff line number Diff line change
@@ -70,6 +70,9 @@ const char * StageToString(CommissioningStage stage)
case kAttestationVerification:
return "AttestationVerification";

case kAttestationRevocationCheck:
return "AttestationRevocationCheck";

case kSendOpCertSigningRequest:
return "SendOpCertSigningRequest";

2 changes: 2 additions & 0 deletions src/controller/CommissioningDelegate.h
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions src/controller/python/OpCredsBinding.cpp
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -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),
Original file line number Diff line number Diff line change
@@ -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 });
Original file line number Diff line number Diff line change
@@ -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:
Original file line number Diff line number Diff line change
@@ -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
10 changes: 10 additions & 0 deletions src/credentials/attestation_verifier/DeviceAttestationVerifier.h
Original file line number Diff line number Diff line change
@@ -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.
*
3 changes: 3 additions & 0 deletions src/lib/core/CHIPError.cpp
Original file line number Diff line number Diff line change
@@ -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;
9 changes: 8 additions & 1 deletion src/lib/core/CHIPError.h
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions src/lib/core/tests/TestCHIPErrorStr.cpp
Original file line number Diff line number Diff line change
@@ -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,
6 changes: 3 additions & 3 deletions src/python_testing/TC_CGEN_2_4.py
Original file line number Diff line number Diff line change
@@ -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):