From 39073b39e7941c4efc41f2e0191106bf13972853 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 21 Jun 2022 10:21:21 -0400 Subject: [PATCH] Stop using Credentials::GetDefaultDACVerifier on Darwin. (#19697) Credentials::GetDefaultDACVerifier returns a singleton object, initialized the very first time the function is called. The Darwin framework wants to be able to shut down and restart with a new PAA trust store, which means not relying on this initialized-once singleton. The fixes to DeviceControllerSystemState shutdown were needed to make the tests (which start up a factory, then shut it down without bringing up any controllers, which is a valid thing to do) pass. --- .../CHIPDeviceControllerFactory.cpp | 8 ++++- .../CHIPDeviceControllerSystemState.h | 10 +++++- .../DefaultDeviceAttestationVerifier.cpp | 28 +---------------- .../DefaultDeviceAttestationVerifier.h | 31 ++++++++++++++++++- .../Framework/CHIP/CHIPDeviceController.mm | 2 ++ .../Framework/CHIP/MatterControllerFactory.mm | 18 +++++++++-- .../CHIP/MatterControllerFactory_Internal.h | 3 +- .../Framework/CHIPTests/CHIPClustersTests.m | 9 ++++++ 8 files changed, 75 insertions(+), 34 deletions(-) diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 1ed14c805406a8..ab2d0b3637a195 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -327,7 +327,13 @@ void DeviceControllerFactory::Shutdown() CHIP_ERROR DeviceControllerSystemState::Shutdown() { - VerifyOrReturnError(mRefCount == 1, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mRefCount <= 1, CHIP_ERROR_INCORRECT_STATE); + if (mHaveShutDown) + { + // Nothing else to do here. + return CHIP_NO_ERROR; + } + mHaveShutDown = true; ChipLogDetail(Controller, "Shutting down the System State, this will teardown the CHIP Stack"); diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h index e0c6c8723d3d50..390339f7fc33ed 100644 --- a/src/controller/CHIPDeviceControllerSystemState.h +++ b/src/controller/CHIPDeviceControllerSystemState.h @@ -106,7 +106,13 @@ class DeviceControllerSystemState using CASEClientPool = DeviceControllerSystemStateParams::CASEClientPool; public: - ~DeviceControllerSystemState(){}; + ~DeviceControllerSystemState() + { + // We could get here if a DeviceControllerFactory is shut down + // without ever creating any controllers, so our refcount never goes + // above 1. In that case we need to make sure we call Shutdown(). + Shutdown(); + }; DeviceControllerSystemState(DeviceControllerSystemStateParams params) : mSystemLayer(params.systemLayer), mTCPEndPointManager(params.tcpEndPointManager), mUDPEndPointManager(params.udpEndPointManager), mTransportMgr(params.transportMgr), mSessionMgr(params.sessionMgr), @@ -195,6 +201,8 @@ class DeviceControllerSystemState std::atomic mRefCount{ 1 }; + bool mHaveShutDown = false; + CHIP_ERROR Shutdown(); }; diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp index 61172baefb9075..518ebc2eb8f6f8 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp @@ -145,31 +145,7 @@ CHIP_ERROR GetCertificationDeclarationCertificate(const ByteSpan & skid, Mutable return CopySpanToMutableSpan(ByteSpan{ sCertChainLookupTable[certChainLookupTableIdx].mCertificate }, outCertificate); } -class DefaultDACVerifier : public DeviceAttestationVerifier -{ -public: - DefaultDACVerifier(const AttestationTrustStore * paaRootStore) : mAttestationTrustStore(paaRootStore) {} - - void VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info, - Callback::Callback * onCompletion) override; - - AttestationVerificationResult ValidateCertificationDeclarationSignature(const ByteSpan & cmsEnvelopeBuffer, - ByteSpan & certDeclBuffer) override; - - AttestationVerificationResult ValidateCertificateDeclarationPayload(const ByteSpan & certDeclBuffer, - const ByteSpan & firmwareInfo, - const DeviceInfoForAttestation & deviceInfo) override; - - CHIP_ERROR VerifyNodeOperationalCSRInformation(const ByteSpan & nocsrElementsBuffer, - const ByteSpan & attestationChallengeBuffer, - const ByteSpan & attestationSignatureBuffer, const P256PublicKey & dacPublicKey, - const ByteSpan & csrNonce) override; - -protected: - DefaultDACVerifier() {} - - const AttestationTrustStore * mAttestationTrustStore; -}; +} // namespace void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info, Callback::Callback * onCompletion) @@ -443,8 +419,6 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa return CHIP_NO_ERROR; } -} // namespace - const AttestationTrustStore * GetTestAttestationTrustStore() { return &kTestAttestationTrustStore; diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h index b79c44a360d2c6..99f2fb5f2b85ae 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h @@ -21,6 +21,32 @@ namespace chip { namespace Credentials { +class DefaultDACVerifier : public DeviceAttestationVerifier +{ +public: + DefaultDACVerifier(const AttestationTrustStore * paaRootStore) : mAttestationTrustStore(paaRootStore) {} + + void VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info, + Callback::Callback * onCompletion) override; + + AttestationVerificationResult ValidateCertificationDeclarationSignature(const ByteSpan & cmsEnvelopeBuffer, + ByteSpan & certDeclBuffer) override; + + AttestationVerificationResult ValidateCertificateDeclarationPayload(const ByteSpan & certDeclBuffer, + const ByteSpan & firmwareInfo, + const DeviceInfoForAttestation & deviceInfo) override; + + CHIP_ERROR VerifyNodeOperationalCSRInformation(const ByteSpan & nocsrElementsBuffer, + const ByteSpan & attestationChallengeBuffer, + const ByteSpan & attestationSignatureBuffer, + const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) override; + +protected: + DefaultDACVerifier() {} + + const AttestationTrustStore * mAttestationTrustStore; +}; + /** * @brief Get implementation of a PAA root store containing a basic set of static PAA roots * sufficient for *testing* only. @@ -35,7 +61,7 @@ namespace Credentials { const AttestationTrustStore * GetTestAttestationTrustStore(); /** - * @brief Get implementation of a sample DAC verifier to validate device + * @brief Get a singleton implementation of a sample DAC verifier to validate device * attestation procedure. * * @param[in] paaRootStore Pointer to the AttestationTrustStore instance to be used by implementation @@ -43,6 +69,9 @@ const AttestationTrustStore * GetTestAttestationTrustStore(); * always available while the DeviceAttestationVerifier could be used. * * @returns a singleton DeviceAttestationVerifier that satisfies basic device attestation procedure requirements. + * This has process lifetime, so the paaRootStore must also have + * process lifetime. In particular, after the first call it's not + * possible to change which AttestationTrustStore is used by this verifier. */ DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore); diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController.mm b/src/darwin/Framework/CHIP/CHIPDeviceController.mm index e90ba51f752e19..857b7c837f6e79 100644 --- a/src/darwin/Framework/CHIP/CHIPDeviceController.mm +++ b/src/darwin/Framework/CHIP/CHIPDeviceController.mm @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -277,6 +278,7 @@ - (BOOL)startup:(CHIPDeviceControllerStartupParamsInternal *)startupParams commissionerParams.controllerNOC = noc; } commissionerParams.controllerVendorId = static_cast([startupParams.vendorId unsignedShortValue]); + commissionerParams.deviceAttestationVerifier = _factory.deviceAttestationVerifier; auto & factory = chip::Controller::DeviceControllerFactory::GetInstance(); diff --git a/src/darwin/Framework/CHIP/MatterControllerFactory.mm b/src/darwin/Framework/CHIP/MatterControllerFactory.mm index 4e533dc78c4433..b08f328ff298e6 100644 --- a/src/darwin/Framework/CHIP/MatterControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MatterControllerFactory.mm @@ -45,6 +45,7 @@ static NSString * const kErrorPersistentStorageInit = @"Init failure while creating a persistent storage delegate"; static NSString * const kErrorAttestationTrustStoreInit = @"Init failure while creating the attestation trust store"; +static NSString * const kErrorDACVerifierInit = @"Init failure while creating the device attestation verifier"; static NSString * const kInfoFactoryShutdown = @"Shutting down the Matter controller factory"; static NSString * const kErrorGroupProviderInit = @"Init failure while initializing group data provider"; static NSString * const kErrorControllersInit = @"Init controllers array failure"; @@ -64,6 +65,7 @@ @interface MatterControllerFactory () @property (readonly) Credentials::GroupDataProviderImpl * groupDataProvider; @property (readonly) NSMutableArray * controllers; @property (readonly) PersistentStorageOperationalKeystore * keystore; +@property () chip::Credentials::DeviceAttestationVerifier * deviceAttestationVerifier; - (BOOL)findMatchingFabric:(FabricTable &)fabricTable params:(CHIPDeviceControllerStartupParams *)params @@ -156,6 +158,11 @@ - (void)cleanupInitObjects - (void)cleanupStartupObjects { + if (_deviceAttestationVerifier) { + delete _deviceAttestationVerifier; + _deviceAttestationVerifier = nullptr; + } + if (_attestationTrustStoreBridge) { delete _attestationTrustStoreBridge; _attestationTrustStoreBridge = nullptr; @@ -209,17 +216,22 @@ - (BOOL)startup:(MatterControllerFactoryParams *)startupParams } // Initialize device attestation verifier + const Credentials::AttestationTrustStore * trustStore; if (startupParams.paaCerts) { _attestationTrustStoreBridge = new CHIPAttestationTrustStoreBridge(startupParams.paaCerts); if (_attestationTrustStoreBridge == nullptr) { CHIP_LOG_ERROR("Error: %@", kErrorAttestationTrustStoreInit); return; } - chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::GetDefaultDACVerifier(_attestationTrustStoreBridge)); + trustStore = _attestationTrustStoreBridge; } else { // TODO: Replace testingRootStore with a AttestationTrustStore that has the necessary official PAA roots available - const chip::Credentials::AttestationTrustStore * testingRootStore = chip::Credentials::GetTestAttestationTrustStore(); - chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::GetDefaultDACVerifier(testingRootStore)); + trustStore = Credentials::GetTestAttestationTrustStore(); + } + _deviceAttestationVerifier = new Credentials::DefaultDACVerifier(trustStore); + if (_deviceAttestationVerifier == nullptr) { + CHIP_LOG_ERROR("Error: %@", kErrorDACVerifierInit); + return; } chip::Controller::FactoryInitParams params; diff --git a/src/darwin/Framework/CHIP/MatterControllerFactory_Internal.h b/src/darwin/Framework/CHIP/MatterControllerFactory_Internal.h index ed65ef17960385..05148855faba9a 100644 --- a/src/darwin/Framework/CHIP/MatterControllerFactory_Internal.h +++ b/src/darwin/Framework/CHIP/MatterControllerFactory_Internal.h @@ -31,6 +31,7 @@ class CHIPPersistentStorageDelegateBridge; namespace chip { namespace Credentials { class GroupDataProvider; + class DeviceAttestationVerifier; } // namespace Credentials } // namespace chip @@ -44,7 +45,7 @@ NS_ASSUME_NONNULL_BEGIN @property (readonly) CHIPPersistentStorageDelegateBridge * storageDelegateBridge; @property (readonly) chip::Credentials::GroupDataProvider * groupData; - +@property (readonly) chip::Credentials::DeviceAttestationVerifier * deviceAttestationVerifier; @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIPTests/CHIPClustersTests.m b/src/darwin/Framework/CHIPTests/CHIPClustersTests.m index cb103165fa05e5..e4300b379aff58 100644 --- a/src/darwin/Framework/CHIPTests/CHIPClustersTests.m +++ b/src/darwin/Framework/CHIPTests/CHIPClustersTests.m @@ -103,10 +103,19 @@ - (void)testInitStack __auto_type * storage = [[CHIPTestStorage alloc] init]; __auto_type * factoryParams = [[MatterControllerFactoryParams alloc] initWithStorage:storage]; factoryParams.port = @(kLocalPort); + // For our first startup, use a dummy paaCerts array. + factoryParams.paaCerts = [[NSArray alloc] init]; BOOL ok = [factory startup:factoryParams]; XCTAssertTrue(ok); + // Test that things still work right if we then shut down and + // restart the factory. + [factory shutdown]; + factoryParams.paaCerts = nil; + ok = [factory startup:factoryParams]; + XCTAssertTrue(ok); + __auto_type * testKeys = [[CHIPTestKeys alloc] init]; XCTAssertNotNil(testKeys);