From 4b1a17146c3a7ab193bd1e30b06f5ecb9ca7f4a5 Mon Sep 17 00:00:00 2001 From: Jean-Francois Penven Date: Fri, 6 Oct 2023 09:15:59 -0400 Subject: [PATCH] Add key challenge --- .../icd-management-server.cpp | 3 +- src/app/icd/IcdManagementServer.cpp | 9 ++--- src/app/icd/IcdMonitoringTable.cpp | 39 +++++++++++++++++++ src/app/icd/IcdMonitoringTable.h | 1 + src/app/tests/TestIcdMonitoringTable.cpp | 33 +++++++++++++--- 5 files changed, 72 insertions(+), 13 deletions(-) diff --git a/src/app/clusters/icd-management-server/icd-management-server.cpp b/src/app/clusters/icd-management-server/icd-management-server.cpp index 4f59fe9cc959ee..3db1881dd12daf 100644 --- a/src/app/clusters/icd-management-server/icd-management-server.cpp +++ b/src/app/clusters/icd-management-server/icd-management-server.cpp @@ -109,8 +109,7 @@ CHIP_ERROR IcdManagementAttributeAccess::ReadRegisteredClients(EndpointId endpoi return encoder.EncodeList([supported_clients](const auto & subEncoder) -> CHIP_ERROR { IcdMonitoringEntry e(chip::Server::GetInstance().GetSessionKeystore()); - const auto & fabricTable = Server::GetInstance().GetFabricTable(); - uint8_t empty_key[CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES] = { 0 }; + const auto & fabricTable = Server::GetInstance().GetFabricTable(); for (const auto & fabricInfo : fabricTable) { PersistentStorageDelegate & storage = chip::Server::GetInstance().GetPersistentStorage(); diff --git a/src/app/icd/IcdManagementServer.cpp b/src/app/icd/IcdManagementServer.cpp index b2a268fefd8889..2eafabd541a5e4 100644 --- a/src/app/icd/IcdManagementServer.cpp +++ b/src/app/icd/IcdManagementServer.cpp @@ -23,8 +23,8 @@ Status IcdManagementServer::RegisterClient(PersistentStorageDelegate & storage, if (!is_admin) { VerifyOrReturnError(verification_key.HasValue(), InteractionModel::Status::Failure); - // TODO Specs issue #7519 - // VerifyOrReturnError(verification_key.Value().data_equal(entry.key()), InteractionModel::Status::Failure); + VerifyOrReturnError(entry.IsKeyCompatible(verification_key.Value()) == CHIP_NO_ERROR, + InteractionModel::Status::Failure); } } else if (CHIP_ERROR_NOT_FOUND == err) @@ -41,7 +41,7 @@ Status IcdManagementServer::RegisterClient(PersistentStorageDelegate & storage, // Save entry.checkInNodeID = node_id; entry.monitoredSubject = monitored_subject; - err = entry.SetKey(key); + err = entry.SetKey(key); VerifyOrReturnError(CHIP_ERROR_INVALID_ARGUMENT != err, InteractionModel::Status::ConstraintError); VerifyOrReturnError(CHIP_NO_ERROR == err, InteractionModel::Status::Failure); err = table.Set(entry.index, entry); @@ -73,8 +73,7 @@ Status IcdManagementServer::UnregisterClient(PersistentStorageDelegate & storage if (!is_admin) { VerifyOrReturnError(verificationKey.HasValue(), InteractionModel::Status::Failure); - // TODO Specs issue #7519 - // VerifyOrReturnError(verificationKey.Value().data_equal(entry.key), InteractionModel::Status::Failure); + VerifyOrReturnError(entry.IsKeyCompatible(verificationKey.Value()) == CHIP_NO_ERROR, InteractionModel::Status::Failure); } err = table.Remove(entry.index); diff --git a/src/app/icd/IcdMonitoringTable.cpp b/src/app/icd/IcdMonitoringTable.cpp index cb810f1798f7df..aa83db7a4bf274 100644 --- a/src/app/icd/IcdMonitoringTable.cpp +++ b/src/app/icd/IcdMonitoringTable.cpp @@ -17,6 +17,8 @@ #include "IcdMonitoringTable.h" +#include + namespace chip { enum class Fields : uint8_t @@ -98,6 +100,10 @@ CHIP_ERROR IcdMonitoringEntry::SetKey(chip::ByteSpan keyData) { VerifyOrReturnError(keyData.size() == chip::Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(sessionKeyStore != nullptr, CHIP_ERROR_INTERNAL); + + // DeleteKey in case the handle was already used. + DeleteKey(); + Crypto::Aes128KeyByteArray keyMaterial; memcpy(keyMaterial, keyData.data(), keyData.size()); @@ -113,6 +119,39 @@ CHIP_ERROR IcdMonitoringEntry::DeleteKey() return CHIP_NO_ERROR; } +CHIP_ERROR IcdMonitoringEntry::IsKeyCompatible(chip::ByteSpan keyData) +{ + VerifyOrReturnError(keyData.size() == chip::Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(sessionKeyStore != nullptr, CHIP_ERROR_INTERNAL); + Crypto::Aes128KeyByteArray keyMaterial; + Crypto::Aes128KeyHandle tempKey; + memcpy(keyMaterial, keyData.data(), keyData.size()); + + ReturnErrorOnFailure(sessionKeyStore->CreateKey(keyMaterial, tempKey)); + + // Challenge + uint8_t mic[chip::Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES] = { 0 }; + uint8_t aead[chip::Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES] = { 0 }; + + CHIP_ERROR err; + + uint64_t data = chip::Crypto::GetRandU64(), validation, encrypted; + validation = data; + + err = Crypto::AES_CCM_encrypt((uint8_t *) (&data), sizeof(data), nullptr, 0, tempKey, aead, + chip::Crypto::CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES, (uint8_t *) &encrypted, mic, + chip::Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES); + + data = 0; + err = Crypto::AES_CCM_decrypt((uint8_t *) &encrypted, sizeof(encrypted), nullptr, 0, mic, + chip::Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES, key, aead, + chip::Crypto::CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES, (uint8_t *) &data); + + sessionKeyStore->DestroyKey(tempKey); + + return (data == validation) ? CHIP_NO_ERROR : CHIP_ERROR_INTERNAL; +} + CHIP_ERROR IcdMonitoringTable::Get(uint16_t index, IcdMonitoringEntry & entry) const { entry.fabricIndex = this->mFabric; diff --git a/src/app/icd/IcdMonitoringTable.h b/src/app/icd/IcdMonitoringTable.h index e575210e57f3fc..ad6a3b87dc668e 100644 --- a/src/app/icd/IcdMonitoringTable.h +++ b/src/app/icd/IcdMonitoringTable.h @@ -55,6 +55,7 @@ struct IcdMonitoringEntry : public PersistentData void Clear() override; CHIP_ERROR SetKey(chip::ByteSpan keyData); CHIP_ERROR DeleteKey(void); + CHIP_ERROR IsKeyCompatible(chip::ByteSpan keyData); chip::FabricIndex fabricIndex = static_cast(0); chip::NodeId checkInNodeID = static_cast(0); diff --git a/src/app/tests/TestIcdMonitoringTable.cpp b/src/app/tests/TestIcdMonitoringTable.cpp index 871d32ecdecb8e..4b5c9170fcb4a4 100644 --- a/src/app/tests/TestIcdMonitoringTable.cpp +++ b/src/app/tests/TestIcdMonitoringTable.cpp @@ -63,6 +63,26 @@ constexpr uint8_t kKeyBuffer3a[] = { // constexpr uint8_t kKeyBuffer3b[] = { 0xf3, 0xe3, 0xd3, 0xc3, 0xb3, 0xa3, 0x93, 0x83, 0x73, 0x63, 0x53, 0x14, 0x33, 0x23, 0x13, // 0x03 }; +void TestEntryKeyFunctions(nlTestSuite * aSuite, void * aContext) +{ + TestSessionKeystoreImpl keystore; + IcdMonitoringEntry entry(&keystore); + + // Test Setting Key + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry.SetKey(ByteSpan(kKeyBuffer1a))); + + // Test Setting Key again + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry.SetKey(ByteSpan(kKeyBuffer1b))); + + // Test Comparing Key + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR != entry.IsKeyCompatible(ByteSpan(kKeyBuffer1a))); + + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry.IsKeyCompatible(ByteSpan(kKeyBuffer1b))); + + // Test Deleting Key + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry.DeleteKey()); +} + void TestSaveAndLoadRegistrationValue(nlTestSuite * aSuite, void * aContext) { TestPersistentStorageDelegate storage; @@ -388,12 +408,13 @@ int Test_Setup(void * inContext) int TestClientMonitoringRegistrationTable() { - static nlTest sTests[] = { - NL_TEST_DEF("TestSaveAndLoadRegistrationValue", TestSaveAndLoadRegistrationValue), - NL_TEST_DEF("TestSaveAllInvalidRegistrationValues", TestSaveAllInvalidRegistrationValues), - NL_TEST_DEF("TestSaveLoadRegistrationValueForMultipleFabrics", TestSaveLoadRegistrationValueForMultipleFabrics), - NL_TEST_DEF("TestDeleteValidEntryFromStorage", TestDeleteValidEntryFromStorage), NL_TEST_SENTINEL() - }; + static nlTest sTests[] = { NL_TEST_DEF("TestEntryKeyFunctions", TestEntryKeyFunctions), + NL_TEST_DEF("TestSaveAndLoadRegistrationValue", TestSaveAndLoadRegistrationValue), + NL_TEST_DEF("TestSaveAllInvalidRegistrationValues", TestSaveAllInvalidRegistrationValues), + NL_TEST_DEF("TestSaveLoadRegistrationValueForMultipleFabrics", + TestSaveLoadRegistrationValueForMultipleFabrics), + NL_TEST_DEF("TestDeleteValidEntryFromStorage", TestDeleteValidEntryFromStorage), + NL_TEST_SENTINEL() }; nlTestSuite cmSuite = { "TestClientMonitoringRegistrationTable", &sTests[0], &Test_Setup, nullptr };