From 2306904aeaaeb3430d72c4ba826ed120d88a42ec Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 11 May 2022 09:27:16 -0400 Subject: [PATCH] Store the updated fabric info when bringing up controller on existing fabric. (#18260) Right now bringing up a controller on an existing fabric updates the in-memory fabric table but does not store the updated data. This is not very consistent with our desire to not even keep the fabric table in memory, and leads to inconsistent behavior if someone later reloads the fabric table from storage in some way. Specific changes: 1. Change FabricInfo::SetFabricInfo to not modify any member state until validation of the incoming info is complete, so we won't corrupt an existing fabric due to mistaken input and leave it in a bad state. 2. If updating the existing fabric in ProcessControllerNOCChain succeeds, store the updated fabric info. This is done outside the fabric table for now, pending changes to the fabric table to store things itself instead of keeping them in RAM. --- src/controller/CHIPDeviceController.cpp | 3 +++ src/credentials/FabricTable.cpp | 31 +++++++++++++++-------- src/credentials/FabricTable.h | 10 ++++++-- src/credentials/tests/TestFabricTable.cpp | 4 +-- 4 files changed, 34 insertions(+), 14 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 2d75459bb8acf5..d94449dbc5ea66 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -184,6 +184,9 @@ CHIP_ERROR DeviceController::ProcessControllerNOCChain(const ControllerInitParam if (mFabricInfo != nullptr) { ReturnErrorOnFailure(mFabricInfo->SetFabricInfo(newFabric)); + // Store the new fabric info, since we might now have new certificates + // and whatnot. + ReturnErrorOnFailure(params.systemState->Fabrics()->Store(mFabricInfo->GetFabricIndex())); } else { diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index 3d188530f149dc..6a8a1a71c5c759 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -190,7 +190,7 @@ CHIP_ERROR FabricInfo::LoadFromStorage(PersistentStorageDelegate * storage) // The compressed fabric ID doesn't change for a fabric over time. // Computing it here will save computational overhead when it's accessed by other // parts of the code. - ReturnErrorOnFailure(GeneratePeerId(mFabricId, nodeId, &mOperationalId)); + ReturnErrorOnFailure(GeneratePeerId(mRootCert, mFabricId, nodeId, &mOperationalId)); ReturnErrorOnFailure(SetNOCCert(nocCert)); } @@ -267,7 +267,7 @@ CHIP_ERROR FabricInfo::LoadFromStorage(PersistentStorageDelegate * storage) return CHIP_NO_ERROR; } -CHIP_ERROR FabricInfo::GeneratePeerId(FabricId fabricId, NodeId nodeId, PeerId * compressedPeerId) const +CHIP_ERROR FabricInfo::GeneratePeerId(const ByteSpan & rcac, FabricId fabricId, NodeId nodeId, PeerId * compressedPeerId) { ReturnErrorCodeIf(compressedPeerId == nullptr, CHIP_ERROR_INVALID_ARGUMENT); uint8_t compressedFabricIdBuf[sizeof(uint64_t)]; @@ -276,7 +276,7 @@ CHIP_ERROR FabricInfo::GeneratePeerId(FabricId fabricId, NodeId nodeId, PeerId * { P256PublicKeySpan rootPubkeySpan; - ReturnErrorOnFailure(GetRootPubkey(rootPubkeySpan)); + ReturnErrorOnFailure(ExtractPublicKeyFromChipCert(rcac, rootPubkeySpan)); rootPubkey = rootPubkeySpan; } @@ -365,6 +365,13 @@ CHIP_ERROR FabricInfo::SetCert(MutableByteSpan & dstCert, const ByteSpan & srcCe CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan & icac, ValidationContext & context, PeerId & nocPeerId, FabricId & fabricId, Crypto::P256PublicKey & nocPubkey) const +{ + return VerifyCredentials(noc, icac, mRootCert, context, nocPeerId, fabricId, nocPubkey); +} + +CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac, + ValidationContext & context, PeerId & nocPeerId, FabricId & fabricId, + Crypto::P256PublicKey & nocPubkey) { // TODO - Optimize credentials verification logic // The certificate chain construction and verification is a compute and memory intensive operation. @@ -375,7 +382,7 @@ CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan & ChipCertificateSet certificates; ReturnErrorOnFailure(certificates.Init(kMaxNumCertsInOpCreds)); - ReturnErrorOnFailure(certificates.LoadCert(mRootCert, BitFlags(CertDecodeFlags::kIsTrustAnchor))); + ReturnErrorOnFailure(certificates.LoadCert(rcac, BitFlags(CertDecodeFlags::kIsTrustAnchor))); if (!icac.empty()) { @@ -423,7 +430,7 @@ CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan & return err; } - ReturnErrorOnFailure(GeneratePeerId(fabricId, nodeId, &nocPeerId)); + ReturnErrorOnFailure(GeneratePeerId(rcac, fabricId, nodeId, &nocPeerId)); nocPubkey = P256PublicKey(certificates.GetLastCert()[0].mPublicKey); return CHIP_NO_ERROR; @@ -551,13 +558,17 @@ CHIP_ERROR FabricInfo::SetFabricInfo(FabricInfo & newFabric) validContext.mRequiredKeyUsages.Set(KeyUsageFlags::kDigitalSignature); validContext.mRequiredKeyPurposes.Set(KeyPurposeFlags::kServerAuth); - SetOperationalKeypair(newFabric.GetOperationalKey()); - SetRootCert(newFabric.mRootCert); - + // Make sure to not modify any of our state until VerifyCredentials passes. + PeerId operationalId; + FabricId fabricId; ChipLogProgress(Discovery, "Verifying the received credentials"); - ReturnErrorOnFailure( - VerifyCredentials(newFabric.mNOCCert, newFabric.mICACert, validContext, mOperationalId, mFabricId, pubkey)); + ReturnErrorOnFailure(VerifyCredentials(newFabric.mNOCCert, newFabric.mICACert, newFabric.mRootCert, validContext, operationalId, + fabricId, pubkey)); + SetOperationalKeypair(newFabric.GetOperationalKey()); + SetRootCert(newFabric.mRootCert); + mOperationalId = operationalId; + mFabricId = fabricId; SetICACert(newFabric.mICACert); SetNOCCert(newFabric.mNOCCert); SetVendorId(newFabric.GetVendorId()); diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 42a0f30397210c..6f2ba3ffa2b20e 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -168,9 +168,15 @@ class DLL_EXPORT FabricInfo return Credentials::ExtractPublicKeyFromChipCert(mRootCert, publicKey); } + // Verifies credentials, using this fabric info's root certificate. CHIP_ERROR VerifyCredentials(const ByteSpan & noc, const ByteSpan & icac, Credentials::ValidationContext & context, PeerId & nocPeerId, FabricId & fabricId, Crypto::P256PublicKey & nocPubkey) const; + // Verifies credentials, using the provided root certificate. + static CHIP_ERROR VerifyCredentials(const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac, + Credentials::ValidationContext & context, PeerId & nocPeerId, FabricId & fabricId, + Crypto::P256PublicKey & nocPubkey); + /** * Reset the state to a completely uninitialized status. */ @@ -192,9 +198,9 @@ class DLL_EXPORT FabricInfo CHIP_ERROR SetFabricInfo(FabricInfo & fabric); /* Generate a compressed peer ID (containing compressed fabric ID) using provided fabric ID, node ID and - root public key of the fabric. The generated compressed ID is returned via compressedPeerId + root public key of the provided root certificate. The generated compressed ID is returned via compressedPeerId output parameter */ - CHIP_ERROR GeneratePeerId(FabricId fabricId, NodeId nodeId, PeerId * compressedPeerId) const; + static CHIP_ERROR GeneratePeerId(const ByteSpan & rcac, FabricId fabricId, NodeId nodeId, PeerId * compressedPeerId); friend class FabricTable; diff --git a/src/credentials/tests/TestFabricTable.cpp b/src/credentials/tests/TestFabricTable.cpp index 54e5cea88a4166..90f29c3ab98c81 100644 --- a/src/credentials/tests/TestFabricTable.cpp +++ b/src/credentials/tests/TestFabricTable.cpp @@ -56,7 +56,7 @@ void TestGetCompressedFabricID(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, fabricInfo.SetRootCert(ByteSpan(sTestRootCert)) == CHIP_NO_ERROR); PeerId compressedId; - NL_TEST_ASSERT(inSuite, fabricInfo.GeneratePeerId(1234, 4321, &compressedId) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, fabricInfo.GeneratePeerId(ByteSpan(sTestRootCert), 1234, 4321, &compressedId) == CHIP_NO_ERROR); // We are compairing with hard coded values here (which are generated manually when the test was written) // This is to ensure that the same value is generated on big endian and little endian platforms. @@ -64,7 +64,7 @@ void TestGetCompressedFabricID(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, compressedId.GetCompressedFabricId() == 0x090F17C67be7b663); NL_TEST_ASSERT(inSuite, compressedId.GetNodeId() == 4321); - NL_TEST_ASSERT(inSuite, fabricInfo.GeneratePeerId(0xabcd, 0xdeed, &compressedId) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, fabricInfo.GeneratePeerId(ByteSpan(sTestRootCert), 0xabcd, 0xdeed, &compressedId) == CHIP_NO_ERROR); // We are compairing with hard coded values here (which are generated manually when the test was written) // This is to ensure that the same value is generated on big endian and little endian platforms