From a7d7d8dcff0614a772a187ae29a17cda901c5ee0 Mon Sep 17 00:00:00 2001 From: C Freeman Date: Fri, 4 Feb 2022 02:00:39 -0500 Subject: [PATCH] Check VID/PID when doing device attestation (#14551) * Pass VID/PID to DAC. * Address nits from #14567 --- src/controller/AutoCommissioner.cpp | 5 ++--- src/controller/AutoCommissioner.h | 3 --- src/controller/CHIPDeviceController.cpp | 10 ++++++---- src/controller/CHIPDeviceController.h | 4 +++- src/controller/CommissioningDelegate.h | 14 ++++++++++++++ src/credentials/DeviceAttestationVerifier.cpp | 3 ++- src/credentials/DeviceAttestationVerifier.h | 3 ++- .../examples/DefaultDeviceAttestationVerifier.cpp | 8 +++++--- .../tests/TestDeviceAttestationCredentials.cpp | 3 ++- 9 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index e46f2772e26fca..04033fb622aca5 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -316,10 +316,10 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio switch (report.stageCompleted) { case CommissioningStage::kReadVendorId: - mVendorId = report.Get().vendorId; + mParams.SetRemoteVendorId(report.Get().vendorId); break; case CommissioningStage::kReadProductId: - mProductId = report.Get().productId; + mParams.SetRemoteProductId(report.Get().productId); break; case CommissioningStage::kReadSoftwareVersion: mSoftwareVersion = report.Get().softwareVersion; @@ -394,7 +394,6 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio } mParams.SetCompletionStatus(err); - // TODO: Get real endpoint mCommissioner->PerformCommissioningStep(proxy, nextStage, mParams, this, GetEndpoint(nextStage), GetCommandTimeout(nextStage)); return CHIP_NO_ERROR; } diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 8c262ad04370d9..b55fa7fbb772ff 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -57,10 +57,7 @@ class AutoCommissioner : public CommissioningDelegate OperationalDeviceProxy * mOperationalDeviceProxy = nullptr; OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr; CommissioningParameters mParams = CommissioningParameters(); - VendorId mVendorId; - uint16_t mProductId; uint32_t mSoftwareVersion; - // Memory space for the commisisoning parameters that come in as ByteSpans - the caller is not guaranteed to retain this memory uint8_t mSsid[CommissioningParameters::kMaxSsidLen]; uint8_t mCredentials[CommissioningParameters::kMaxCredentialsLen]; uint8_t mThreadOperationalDataset[CommissioningParameters::kMaxThreadDatasetLen]; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index b4f73dd0229d1b..50dc82a56c7e37 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1153,7 +1153,8 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * conte CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const ByteSpan & attestationElements, const ByteSpan & signature, const ByteSpan & attestationNonce, const ByteSpan & pai, - const ByteSpan & dac, DeviceProxy * proxy) + const ByteSpan & dac, VendorId remoteVendorId, uint16_t remoteProductId, + DeviceProxy * proxy) { VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(proxy != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -1165,7 +1166,7 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const ByteSpan & attestat proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge(); dac_verifier->VerifyAttestationInformation(attestationElements, attestationChallenge, signature, pai, dac, attestationNonce, - &mDeviceAttestationInformationVerificationCallback); + remoteVendorId, remoteProductId, &mDeviceAttestationInformationVerificationCallback); // TODO: Validate Firmware Information @@ -1798,7 +1799,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } mAttributeCache = std::move(attributeCache); mReadClient = std::move(readClient); - return; } break; case CommissioningStage::kConfigRegulatory: { @@ -1866,7 +1866,8 @@ 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.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() || + !params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue()) { ChipLogError(Controller, "Missing attestation information"); CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); @@ -1874,6 +1875,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } if (ValidateAttestationInfo(params.GetAttestationElements().Value(), params.GetAttestationSignature().Value(), params.GetAttestationNonce().Value(), params.GetPAI().Value(), params.GetDAC().Value(), + params.GetRemoteVendorId().Value(), params.GetRemoteProductId().Value(), proxy) != CHIP_NO_ERROR) { ChipLogError(Controller, "Error validating attestation information"); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 19d0d03793d123..9eab6ddb289e86 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -591,11 +591,13 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, * @param[in] attestationNonce Attestation nonce * @param[in] pai PAI certificate * @param[in] dac DAC certificates + * @param[in] remoteVendorId vendor ID read from the device + * @param[in] remoteProductId product ID read from the device * @param[in] proxy device proxy that is being attested. */ CHIP_ERROR ValidateAttestationInfo(const ByteSpan & attestationElements, const ByteSpan & signature, const ByteSpan & attestationNonce, const ByteSpan & pai, const ByteSpan & dac, - DeviceProxy * proxy); + VendorId remoteVendorId, uint16_t remoteProductId, DeviceProxy * proxy); /** * @brief diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 16474ee08460c1..55cbe417d53bed 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -97,6 +97,8 @@ class CommissioningParameters const Optional GetAttestationSignature() const { return mAttestationSignature; } const Optional GetPAI() const { return mPAI; } const Optional GetDAC() const { return mDAC; } + const Optional GetRemoteVendorId() const { return mRemoteVendorId; } + const Optional GetRemoteProductId() const { return mRemoteProductId; } CHIP_ERROR GetCompletionStatus() { return completionStatus; } CommissioningParameters & SetFailsafeTimerSeconds(uint16_t seconds) @@ -185,6 +187,16 @@ class CommissioningParameters mDAC = MakeOptional(dac); return *this; } + CommissioningParameters & SetRemoteVendorId(VendorId id) + { + mRemoteVendorId = MakeOptional(id); + return *this; + } + CommissioningParameters & SetRemoteProductId(uint16_t id) + { + mRemoteProductId = MakeOptional(id); + return *this; + } void SetCompletionStatus(CHIP_ERROR err) { completionStatus = err; } private: @@ -203,6 +215,8 @@ class CommissioningParameters Optional mAttestationSignature; Optional mPAI; Optional mDAC; + Optional mRemoteVendorId; + Optional mRemoteProductId; CHIP_ERROR completionStatus = CHIP_NO_ERROR; }; diff --git a/src/credentials/DeviceAttestationVerifier.cpp b/src/credentials/DeviceAttestationVerifier.cpp index 6775ac6c3528c5..ab71dc8b637191 100644 --- a/src/credentials/DeviceAttestationVerifier.cpp +++ b/src/credentials/DeviceAttestationVerifier.cpp @@ -32,7 +32,8 @@ class UnimplementedDACVerifier : public DeviceAttestationVerifier public: void VerifyAttestationInformation(const ByteSpan & attestationInfoBuffer, const ByteSpan & attestationChallengeBuffer, const ByteSpan & attestationSignatureBuffer, const ByteSpan & paiDerBuffer, - const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, + const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, VendorId vendorId, + uint16_t productId, Callback::Callback * onCompletion) override { (void) attestationInfoBuffer; diff --git a/src/credentials/DeviceAttestationVerifier.h b/src/credentials/DeviceAttestationVerifier.h index a19f11a75d8bfb..ae91633efbd615 100644 --- a/src/credentials/DeviceAttestationVerifier.h +++ b/src/credentials/DeviceAttestationVerifier.h @@ -213,7 +213,8 @@ class DeviceAttestationVerifier */ virtual void VerifyAttestationInformation(const ByteSpan & attestationInfoBuffer, const ByteSpan & attestationChallengeBuffer, const ByteSpan & attestationSignatureBuffer, const ByteSpan & paiDerBuffer, - const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, + const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, VendorId vendorId, + uint16_t productId, Callback::Callback * onCompletion) = 0; /** diff --git a/src/credentials/examples/DefaultDeviceAttestationVerifier.cpp b/src/credentials/examples/DefaultDeviceAttestationVerifier.cpp index 478893ef17a083..23ead70f4eb361 100644 --- a/src/credentials/examples/DefaultDeviceAttestationVerifier.cpp +++ b/src/credentials/examples/DefaultDeviceAttestationVerifier.cpp @@ -150,7 +150,8 @@ class DefaultDACVerifier : public DeviceAttestationVerifier void VerifyAttestationInformation(const ByteSpan & attestationInfoBuffer, const ByteSpan & attestationChallengeBuffer, const ByteSpan & attestationSignatureBuffer, const ByteSpan & paiDerBuffer, - const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, + const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, VendorId vendorId, + uint16_t productId, Callback::Callback * onCompletion) override; AttestationVerificationResult ValidateCertificationDeclarationSignature(const ByteSpan & cmsEnvelopeBuffer, @@ -175,6 +176,7 @@ void DefaultDACVerifier::VerifyAttestationInformation(const ByteSpan & attestati const ByteSpan & attestationChallengeBuffer, const ByteSpan & attestationSignatureBuffer, const ByteSpan & paiDerBuffer, const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, + VendorId vendorId, uint16_t productId, Callback::Callback * onCompletion) { AttestationVerificationResult attestationError = AttestationVerificationResult::kSuccess; @@ -271,8 +273,8 @@ void DefaultDACVerifier::VerifyAttestationInformation(const ByteSpan & attestati ByteSpan certificationDeclarationPayload; DeviceInfoForAttestation deviceInfo{ - .vendorId = 0xFFF1, - .productId = 0x8000, // TODO: Retrieve vendorId and ProductId from Basic Information Cluster + .vendorId = vendorId, + .productId = productId, .dacVendorId = dacVendorId, .paiVendorId = dacVendorId, }; diff --git a/src/credentials/tests/TestDeviceAttestationCredentials.cpp b/src/credentials/tests/TestDeviceAttestationCredentials.cpp index 743f7738851492..6dd338ebca6219 100644 --- a/src/credentials/tests/TestDeviceAttestationCredentials.cpp +++ b/src/credentials/tests/TestDeviceAttestationCredentials.cpp @@ -226,7 +226,8 @@ static void TestDACVerifierExample_AttestationInfoVerification(nlTestSuite * inS default_verifier->VerifyAttestationInformation( ByteSpan(attestationElementsTestVector), ByteSpan(attestationChallengeTestVector), ByteSpan(attestationSignatureTestVector), - pai_span, dac_span, ByteSpan(attestationNonceTestVector), &attestationInformationVerificationCallback); + pai_span, dac_span, ByteSpan(attestationNonceTestVector), static_cast(0xFFF1), 0x8000, + &attestationInformationVerificationCallback); NL_TEST_ASSERT(inSuite, attestationResult == AttestationVerificationResult::kSuccess); }