From 99f7465b52041b30d12b323c72df4f390a7b5860 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 4 Feb 2022 13:42:50 -0500 Subject: [PATCH] Revert "Check VID/PID when doing device attestation (#14551)" This reverts commit a7d7d8dcff0614a772a187ae29a17cda901c5ee0. --- 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, 17 insertions(+), 36 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 04033fb622aca5..e46f2772e26fca 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: - mParams.SetRemoteVendorId(report.Get().vendorId); + mVendorId = report.Get().vendorId; break; case CommissioningStage::kReadProductId: - mParams.SetRemoteProductId(report.Get().productId); + mProductId = report.Get().productId; break; case CommissioningStage::kReadSoftwareVersion: mSoftwareVersion = report.Get().softwareVersion; @@ -394,6 +394,7 @@ 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 5f499275d55a64..90438703ed9a29 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -57,7 +57,10 @@ 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 71d494925e290d..cdb3a482bf74b8 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1153,8 +1153,7 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * conte CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const ByteSpan & attestationElements, const ByteSpan & signature, const ByteSpan & attestationNonce, const ByteSpan & pai, - const ByteSpan & dac, VendorId remoteVendorId, uint16_t remoteProductId, - DeviceProxy * proxy) + const ByteSpan & dac, DeviceProxy * proxy) { VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(proxy != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -1166,7 +1165,7 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const ByteSpan & attestat proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge(); dac_verifier->VerifyAttestationInformation(attestationElements, attestationChallenge, signature, pai, dac, attestationNonce, - remoteVendorId, remoteProductId, &mDeviceAttestationInformationVerificationCallback); + &mDeviceAttestationInformationVerificationCallback); // TODO: Validate Firmware Information @@ -1799,6 +1798,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } mAttributeCache = std::move(attributeCache); mReadClient = std::move(readClient); + return; } break; case CommissioningStage::kConfigRegulatory: { @@ -1866,8 +1866,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()) + !params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue()) { ChipLogError(Controller, "Missing attestation information"); CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); @@ -1875,7 +1874,6 @@ 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 7e4261f58924f2..ec8af2a956585d 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -591,13 +591,11 @@ 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, - VendorId remoteVendorId, uint16_t remoteProductId, DeviceProxy * proxy); + DeviceProxy * proxy); /** * @brief diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 55cbe417d53bed..16474ee08460c1 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -97,8 +97,6 @@ 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) @@ -187,16 +185,6 @@ 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: @@ -215,8 +203,6 @@ 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 ab71dc8b637191..6775ac6c3528c5 100644 --- a/src/credentials/DeviceAttestationVerifier.cpp +++ b/src/credentials/DeviceAttestationVerifier.cpp @@ -32,8 +32,7 @@ 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, VendorId vendorId, - uint16_t productId, + const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, Callback::Callback * onCompletion) override { (void) attestationInfoBuffer; diff --git a/src/credentials/DeviceAttestationVerifier.h b/src/credentials/DeviceAttestationVerifier.h index ae91633efbd615..a19f11a75d8bfb 100644 --- a/src/credentials/DeviceAttestationVerifier.h +++ b/src/credentials/DeviceAttestationVerifier.h @@ -213,8 +213,7 @@ class DeviceAttestationVerifier */ virtual void VerifyAttestationInformation(const ByteSpan & attestationInfoBuffer, const ByteSpan & attestationChallengeBuffer, const ByteSpan & attestationSignatureBuffer, const ByteSpan & paiDerBuffer, - const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, VendorId vendorId, - uint16_t productId, + const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, Callback::Callback * onCompletion) = 0; /** diff --git a/src/credentials/examples/DefaultDeviceAttestationVerifier.cpp b/src/credentials/examples/DefaultDeviceAttestationVerifier.cpp index 35bcf5115a130d..b79db0250050b0 100644 --- a/src/credentials/examples/DefaultDeviceAttestationVerifier.cpp +++ b/src/credentials/examples/DefaultDeviceAttestationVerifier.cpp @@ -150,8 +150,7 @@ class DefaultDACVerifier : public DeviceAttestationVerifier void VerifyAttestationInformation(const ByteSpan & attestationInfoBuffer, const ByteSpan & attestationChallengeBuffer, const ByteSpan & attestationSignatureBuffer, const ByteSpan & paiDerBuffer, - const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, VendorId vendorId, - uint16_t productId, + const ByteSpan & dacDerBuffer, const ByteSpan & attestationNonce, Callback::Callback * onCompletion) override; AttestationVerificationResult ValidateCertificationDeclarationSignature(const ByteSpan & cmsEnvelopeBuffer, @@ -176,7 +175,6 @@ 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; @@ -273,8 +271,8 @@ void DefaultDACVerifier::VerifyAttestationInformation(const ByteSpan & attestati ByteSpan certificationDeclarationPayload; DeviceInfoForAttestation deviceInfo{ - .vendorId = vendorId, - .productId = productId, + .vendorId = 0xFFF1, + .productId = 0x8000, // TODO: Retrieve vendorId and ProductId from Basic Information Cluster .dacVendorId = dacVendorId, .paiVendorId = dacVendorId, }; diff --git a/src/credentials/tests/TestDeviceAttestationCredentials.cpp b/src/credentials/tests/TestDeviceAttestationCredentials.cpp index 6dd338ebca6219..743f7738851492 100644 --- a/src/credentials/tests/TestDeviceAttestationCredentials.cpp +++ b/src/credentials/tests/TestDeviceAttestationCredentials.cpp @@ -226,8 +226,7 @@ static void TestDACVerifierExample_AttestationInfoVerification(nlTestSuite * inS default_verifier->VerifyAttestationInformation( ByteSpan(attestationElementsTestVector), ByteSpan(attestationChallengeTestVector), ByteSpan(attestationSignatureTestVector), - pai_span, dac_span, ByteSpan(attestationNonceTestVector), static_cast(0xFFF1), 0x8000, - &attestationInformationVerificationCallback); + pai_span, dac_span, ByteSpan(attestationNonceTestVector), &attestationInformationVerificationCallback); NL_TEST_ASSERT(inSuite, attestationResult == AttestationVerificationResult::kSuccess); }