[ICD] Add Hmac Key handle to ICDCheckInSender to support PSA backend (#…

…30926) * refactor CheckInMessage impl * finish ICD Check-In cleanup * Restyled by whitespace * Restyled by clang-format * Remove unused includes * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * update test comments * Restyle ICDCheckInSender * Update error and associated comments * refactor Check-In message impl * rename gargantua to veryLarge * add missing include * Add buffer writer when encoding ActiveModeThreshold * Refactor subspan logic to use a cusorIndex * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Fix function comments rename min payload size * Update src/app/icd/ICDCheckInSender.cpp Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> --------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>
project-chip · Jan 15, 2024 · da1b825 · da1b825
1 parent d8e0dd2
commit da1b825
Show file tree

Hide file tree

Showing 9 changed files with 244 additions and 119 deletions.
diff --git a/examples/lit-icd-app/silabs/src/ShellCommands.cpp b/examples/lit-icd-app/silabs/src/ShellCommands.cpp
@@ -18,9 +18,6 @@
 #if defined(ENABLE_CHIP_SHELL)
 
 #include "ShellCommands.h"
-#include "BindingHandler.h"
-
-#include <app/clusters/bindings/bindings.h>
 #include <lib/shell/Engine.h>
 #include <lib/shell/commands/Help.h>
 #include <platform/CHIPDeviceLayer.h>

diff --git a/src/app/icd/BUILD.gn b/src/app/icd/BUILD.gn
@@ -81,6 +81,7 @@ source_set("sender") {
   ]
 
   public_deps = [
+    ":configuration-data",
     ":monitoring-table",
     ":notifier",
     "${chip_root}/src/credentials:credentials",

diff --git a/src/app/icd/ICDCheckInSender.cpp b/src/app/icd/ICDCheckInSender.cpp
@@ -15,15 +15,12 @@
  *    limitations under the License.
  */
 
-#include "ICDCheckInSender.h"
-
-#include "ICDNotifier.h"
-
-#include <system/SystemPacketBuffer.h>
-
-#include <protocols/secure_channel/CheckinMessage.h>
-
+#include <app/icd/ICDCheckInSender.h>
+#include <app/icd/ICDConfigurationData.h>
+#include <app/icd/ICDNotifier.h>
 #include <lib/dnssd/Resolver.h>
+#include <protocols/secure_channel/CheckinMessage.h>
+#include <system/SystemPacketBuffer.h>
 
 namespace chip {
 namespace app {
@@ -56,12 +53,26 @@ void ICDCheckInSender::OnNodeAddressResolutionFailed(const PeerId & peerId, CHIP
 
 CHIP_ERROR ICDCheckInSender::SendCheckInMsg(const Transport::PeerAddress & addr)
 {
-    System::PacketBufferHandle buffer = MessagePacketBuffer::New(CheckinMessage::sMinPayloadSize);
+    System::PacketBufferHandle buffer = MessagePacketBuffer::New(CheckinMessage::kMinPayloadSize);
 
     VerifyOrReturnError(!buffer.IsNull(), CHIP_ERROR_NO_MEMORY);
     MutableByteSpan output{ buffer->Start(), buffer->MaxDataLength() };
 
-    ReturnErrorOnFailure(CheckinMessage::GenerateCheckinMessagePayload(mAesKeyHandle, mICDCounter, ByteSpan(), output));
+    // Encoded ActiveModeThreshold in littleEndian for Check-In message application data
+    {
+        uint8_t activeModeThresholdBuffer[kApplicationDataSize] = { 0 };
+        size_t writtenBytes                                     = 0;
+        Encoding::LittleEndian::BufferWriter writer(activeModeThresholdBuffer, sizeof(activeModeThresholdBuffer));
+
+        writer.Put16(ICDConfigurationData::GetInstance().GetActiveModeThresholdMs());
+        VerifyOrReturnError(writer.Fit(writtenBytes), CHIP_ERROR_INTERNAL);
+
+        ByteSpan activeModeThresholdByteSpan(writer.Buffer(), writtenBytes);
+
+        ReturnErrorOnFailure(CheckinMessage::GenerateCheckinMessagePayload(mAes128KeyHandle, mHmac128KeyHandle, mICDCounter,
+                                                                           activeModeThresholdByteSpan, output));
+    }
+
     buffer->SetDataLength(static_cast<uint16_t>(output.size()));
 
     VerifyOrReturnError(mExchangeManager->GetSessionManager() != nullptr, CHIP_ERROR_INTERNAL);
@@ -89,9 +100,12 @@ CHIP_ERROR ICDCheckInSender::RequestResolve(ICDMonitoringEntry & entry, FabricTa
 
     AddressResolve::NodeLookupRequest request(peerId);
 
-    memcpy(mAesKeyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>(),
+    memcpy(mAes128KeyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>(),
            entry.aesKeyHandle.As<Crypto::Symmetric128BitsKeyByteArray>(), sizeof(Crypto::Symmetric128BitsKeyByteArray));
 
+    memcpy(mHmac128KeyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>(),
+           entry.hmacKeyHandle.As<Crypto::Symmetric128BitsKeyByteArray>(), sizeof(Crypto::Symmetric128BitsKeyByteArray));
+
     CHIP_ERROR err = AddressResolve::Resolver::Instance().LookupNode(request, mAddressLookupHandle);
 
     if (err == CHIP_NO_ERROR)

diff --git a/src/app/icd/ICDCheckInSender.h b/src/app/icd/ICDCheckInSender.h
@@ -43,14 +43,17 @@ class ICDCheckInSender : public AddressResolve::NodeListener
     bool mResolveInProgress = false;
 
 private:
+    static constexpr uint8_t kApplicationDataSize = 2; // ActiveModeThreshold is 2 bytes
+
     CHIP_ERROR SendCheckInMsg(const Transport::PeerAddress & addr);
 
     // This is used when a node address is required.
     AddressResolve::NodeLookupHandle mAddressLookupHandle;
 
     Messaging::ExchangeManager * mExchangeManager = nullptr;
 
-    Crypto::Aes128KeyHandle mAesKeyHandle = Crypto::Aes128KeyHandle();
+    Crypto::Aes128KeyHandle mAes128KeyHandle   = Crypto::Aes128KeyHandle();
+    Crypto::Hmac128KeyHandle mHmac128KeyHandle = Crypto::Hmac128KeyHandle();
 
     uint32_t mICDCounter = 0;
 };

diff --git a/src/app/icd/ICDConfigurationData.cpp b/src/app/icd/ICDConfigurationData.cpp
@@ -28,9 +28,9 @@ System::Clock::Milliseconds32 ICDConfigurationData::GetSlowPollingInterval()
     // When in SIT mode, the slow poll interval SHOULDN'T be greater than the SIT mode polling threshold, per spec.
     // This is important for ICD device configured for LIT operation but currently operating as a SIT
     // due to a lack of client registration
-    if (mICDMode == ICDMode::SIT && GetSlowPollingInterval() > GetSITPollingThreshold())
+    if (mICDMode == ICDMode::SIT && mSlowPollingInterval > kSITPollingThreshold)
     {
-        return GetSITPollingThreshold();
+        return kSITPollingThreshold;
     }
 #endif
     return mSlowPollingInterval;

diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp
@@ -275,15 +275,6 @@ void ICDManager::UpdateOperationState(OperationalState state)
 
         System::Clock::Milliseconds32 slowPollInterval = ICDConfigurationData::GetInstance().GetSlowPollingInterval();
 
-#if ICD_ENFORCE_SIT_SLOW_POLL_LIMIT
-        // When in SIT mode, the slow poll interval SHOULDN'T be greater than the SIT mode polling threshold, per spec.
-        if (ICDConfigurationData::GetInstance().GetICDMode() == ICDConfigurationData::ICDMode::SIT &&
-            GetSlowPollingInterval() > GetSITPollingThreshold())
-        {
-            slowPollInterval = GetSITPollingThreshold();
-        }
-#endif
-
         // Going back to Idle, all Check-In messages are sent
         mICDSenderPool.ReleaseAll();
 

diff --git a/src/protocols/secure_channel/CheckinMessage.cpp b/src/protocols/secure_channel/CheckinMessage.cpp
@@ -30,71 +30,118 @@ namespace chip {
 namespace Protocols {
 namespace SecureChannel {
 
-CHIP_ERROR CheckinMessage::GenerateCheckinMessagePayload(Crypto::Aes128KeyHandle & key, CounterType counter,
-                                                         const ByteSpan & appData, MutableByteSpan & output)
+CHIP_ERROR CheckinMessage::GenerateCheckinMessagePayload(const Crypto::Aes128KeyHandle & aes128KeyHandle,
+                                                         const Crypto::Hmac128KeyHandle & hmacKeyHandle,
+                                                         const CounterType & counter, const ByteSpan & appData,
+                                                         MutableByteSpan & output)
 {
-    VerifyOrReturnError(appData.size() <= sMaxAppDataSize, CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(output.size() >= (appData.size() + sMinPayloadSize), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(output.size() >= (appData.size() + kMinPayloadSize), CHIP_ERROR_BUFFER_TOO_SMALL);
+    size_t cursorIndex = 0;
 
-    CHIP_ERROR err            = CHIP_NO_ERROR;
-    uint8_t * appDataStartPtr = output.data() + CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES;
-    Encoding::LittleEndian::Put32(appDataStartPtr, counter);
+    // Generate Nonce from Key and counter value
+    {
+        MutableByteSpan nonce = output.SubSpan(0, CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES);
+        cursorIndex += nonce.size();
 
-    chip::Crypto::HMAC_sha shaHandler;
-    uint8_t nonceWorkBuffer[CHIP_CRYPTO_HASH_LEN_BYTES] = { 0 };
+        Encoding::LittleEndian::BufferWriter writer(nonce);
+        ReturnErrorOnFailure(GenerateCheckInMessageNonce(hmacKeyHandle, counter, writer));
+    }
+
+    // Encrypt Counter and Application Data
+    {
+        MutableByteSpan payloadByteSpan = output.SubSpan(cursorIndex, sizeof(CounterType) + appData.size());
+        cursorIndex += payloadByteSpan.size();
 
-    ReturnErrorOnFailure(shaHandler.HMAC_SHA256(key.As<Symmetric128BitsKeyByteArray>(), sizeof(Symmetric128BitsKeyByteArray),
-                                                appDataStartPtr, sizeof(CounterType), nonceWorkBuffer, CHIP_CRYPTO_HASH_LEN_BYTES));
+        Encoding::LittleEndian::BufferWriter payloadWriter(payloadByteSpan);
 
-    static_assert(sizeof(nonceWorkBuffer) >= CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES, "We're reading off the end of our buffer.");
-    memcpy(output.data(), nonceWorkBuffer, CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES);
+        payloadWriter.EndianPut(counter, sizeof(counter));
+        payloadWriter.Put(appData.data(), appData.size());
+        VerifyOrReturnError(payloadWriter.Fit(), CHIP_ERROR_INTERNAL);
 
-    // In place encryption to save some RAM
-    memcpy(appDataStartPtr + sizeof(CounterType), appData.data(), appData.size());
+        MutableByteSpan mic = output.SubSpan(cursorIndex, CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES);
+        cursorIndex += mic.size();
 
-    uint8_t * micPtr = appDataStartPtr + sizeof(CounterType) + appData.size();
-    ReturnErrorOnFailure(Crypto::AES_CCM_encrypt(appDataStartPtr, sizeof(CounterType) + appData.size(), nullptr, 0, key,
-                                                 output.data(), CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES, appDataStartPtr, micPtr,
-                                                 CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES));
+        // Validate that the cursorIndex is within the available output space
+        VerifyOrReturnError(cursorIndex <= output.size(), CHIP_ERROR_BUFFER_TOO_SMALL);
+        // Validate that the cursorIndex matchs the message length
+        VerifyOrReturnError(cursorIndex == appData.size() + kMinPayloadSize, CHIP_ERROR_INTERNAL);
 
-    output.reduce_size(appData.size() + sMinPayloadSize);
+        ReturnErrorOnFailure(Crypto::AES_CCM_encrypt(payloadByteSpan.data(), payloadByteSpan.size(), nullptr, 0, aes128KeyHandle,
+                                                     output.data(), CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES, payloadByteSpan.data(),
+                                                     mic.data(), mic.size()));
+    }
 
-    return err;
+    output.reduce_size(appData.size() + kMinPayloadSize);
+    return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR CheckinMessage::ParseCheckinMessagePayload(Crypto::Aes128KeyHandle & key, ByteSpan & payload, CounterType & counter,
-                                                      MutableByteSpan & appData)
+CHIP_ERROR CheckinMessage::ParseCheckinMessagePayload(const Crypto::Aes128KeyHandle & aes128KeyHandle,
+                                                      const Crypto::Hmac128KeyHandle & hmacKeyHandle, ByteSpan & payload,
+                                                      CounterType & counter, MutableByteSpan & appData)
 {
-    VerifyOrReturnError(payload.size() >= sMinPayloadSize, CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(payload.size() <= (sMinPayloadSize + sMaxAppDataSize), CHIP_ERROR_INVALID_ARGUMENT);
-
-    CHIP_ERROR err     = CHIP_NO_ERROR;
     size_t appDataSize = GetAppDataSize(payload);
 
+    VerifyOrReturnError(payload.size() >= kMinPayloadSize, CHIP_ERROR_INVALID_MESSAGE_LENGTH);
     // To prevent workbuffer usage, appData size needs to be large enough to hold both the appData and the counter
-    VerifyOrReturnError(appData.size() >= sizeof(CounterType) + appDataSize, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(appData.size() >= sizeof(CounterType) + appDataSize, CHIP_ERROR_BUFFER_TOO_SMALL);
+
+    // Decrypt received data
+    {
+        size_t cursorIndex = 0;
+
+        ByteSpan nonce = payload.SubSpan(cursorIndex, CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES);
+        cursorIndex += nonce.size();
 
-    ByteSpan nonce         = payload.SubSpan(0, CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES);
-    ByteSpan encryptedData = payload.SubSpan(CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES, sizeof(CounterType) + appDataSize);
-    ByteSpan mic =
-        payload.SubSpan(CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES + sizeof(CounterType) + appDataSize, CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES);
+        ByteSpan encryptedData = payload.SubSpan(cursorIndex, sizeof(CounterType) + appDataSize);
+        cursorIndex += encryptedData.size();
 
-    err = Crypto::AES_CCM_decrypt(encryptedData.data(), encryptedData.size(), nullptr, 0, mic.data(), mic.size(), key, nonce.data(),
-                                  nonce.size(), appData.data());
+        ByteSpan mic = payload.SubSpan(cursorIndex, CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES);
+        cursorIndex += mic.size();
 
-    ReturnErrorOnFailure(err);
+        // Return Invalid message length since the payload isn't the right size
+        VerifyOrReturnError(cursorIndex == payload.size(), CHIP_ERROR_INVALID_MESSAGE_LENGTH);
 
+        ReturnErrorOnFailure(Crypto::AES_CCM_decrypt(encryptedData.data(), encryptedData.size(), nullptr, 0, mic.data(), mic.size(),
+                                                     aes128KeyHandle, nonce.data(), nonce.size(), appData.data()));
+    }
+
+    // Read decrypted counter and application data
     counter = Encoding::LittleEndian::Get32(appData.data());
+
+    // TODO : Validate received nonce by calculating it with the hmacKeyHandle and received Counter value
+
     // Shift to remove the counter from the appData
     memmove(appData.data(), sizeof(CounterType) + appData.data(), appDataSize);
-
     appData.reduce_size(appDataSize);
-    return err;
+
+    return CHIP_NO_ERROR;
+}
+
+CHIP_ERROR CheckinMessage::GenerateCheckInMessageNonce(const Crypto::Hmac128KeyHandle & hmacKeyHandle, CounterType counter,
+                                                       Encoding::LittleEndian::BufferWriter & writer)
+{
+    VerifyOrReturnError(writer.Available() >= CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES, CHIP_ERROR_BUFFER_TOO_SMALL);
+
+    uint8_t hashWorkBuffer[CHIP_CRYPTO_HASH_LEN_BYTES] = { 0 };
+    uint8_t counterBuffer[sizeof(CounterType)];
+
+    // validate that Check-In counter is a uint32_t
+    static_assert(sizeof(CounterType) == sizeof(uint32_t), "Expect counter to be 32 bits for correct encoding");
+    Encoding::LittleEndian::Put32(counterBuffer, counter);
+
+    chip::Crypto::HMAC_sha shaHandler;
+    ReturnErrorOnFailure(
+        shaHandler.HMAC_SHA256(hmacKeyHandle, counterBuffer, sizeof(CounterType), hashWorkBuffer, CHIP_CRYPTO_HASH_LEN_BYTES));
+
+    writer.Put(hashWorkBuffer, CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES);
+    VerifyOrReturnError(writer.Fit(), CHIP_ERROR_BUFFER_TOO_SMALL);
+
+    return CHIP_NO_ERROR;
 }
 
 size_t CheckinMessage::GetAppDataSize(ByteSpan & payload)
 {
-    return (payload.size() <= sMinPayloadSize) ? 0 : payload.size() - sMinPayloadSize;
+    return (payload.size() <= kMinPayloadSize) ? 0 : payload.size() - kMinPayloadSize;
 }
 
 } // namespace SecureChannel