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
Show file tree
Hide file tree
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
Expand Up @@ -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` |
Expand Down
9 changes: 9 additions & 0 deletions src/controller/AutoCommissioner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>())
{
Expand Down
73 changes: 69 additions & 4 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Expand All @@ -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;
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
Expand All @@ -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: {
Expand Down Expand Up @@ -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();
Expand Down
10 changes: 10 additions & 0 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/controller/CommissioningDelegate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ const char * StageToString(CommissioningStage stage)
case kAttestationVerification:
return "AttestationVerification";

case kAttestationRevocationCheck:
return "AttestationRevocationCheck";

case kSendOpCertSigningRequest:
return "SendOpCertSigningRequest";

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