From 304328ea122957ffe44ccdf50a733680febbe322 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Thu, 14 Dec 2023 13:47:27 -0500 Subject: [PATCH] Fix function comments rename min payload size --- src/app/icd/ICDCheckInSender.cpp | 2 +- .../secure_channel/CheckinMessage.cpp | 15 +++++++++----- src/protocols/secure_channel/CheckinMessage.h | 20 +++++++++---------- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/app/icd/ICDCheckInSender.cpp b/src/app/icd/ICDCheckInSender.cpp index 79c01d6acf473d..d6fb9090c92dbc 100644 --- a/src/app/icd/ICDCheckInSender.cpp +++ b/src/app/icd/ICDCheckInSender.cpp @@ -53,7 +53,7 @@ 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() }; diff --git a/src/protocols/secure_channel/CheckinMessage.cpp b/src/protocols/secure_channel/CheckinMessage.cpp index 6e4f8026398d9b..f8545b913687f7 100644 --- a/src/protocols/secure_channel/CheckinMessage.cpp +++ b/src/protocols/secure_channel/CheckinMessage.cpp @@ -35,7 +35,7 @@ CHIP_ERROR CheckinMessage::GenerateCheckinMessagePayload(const Crypto::Aes128Key const CounterType & counter, const ByteSpan & appData, MutableByteSpan & output) { - VerifyOrReturnError(output.size() >= (appData.size() + sMinPayloadSize), CHIP_ERROR_BUFFER_TOO_SMALL); + VerifyOrReturnError(output.size() >= (appData.size() + kMinPayloadSize), CHIP_ERROR_BUFFER_TOO_SMALL); size_t cursorIndex = 0; // Generate Nonce from Key and counter value @@ -61,15 +61,17 @@ CHIP_ERROR CheckinMessage::GenerateCheckinMessagePayload(const Crypto::Aes128Key MutableByteSpan mic = output.SubSpan(cursorIndex, CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES); cursorIndex += mic.size(); - // Validate that cursorIndex is within the available output space + // 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); 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())); } - output.reduce_size(appData.size() + sMinPayloadSize); + output.reduce_size(appData.size() + kMinPayloadSize); return CHIP_NO_ERROR; } @@ -79,7 +81,7 @@ CHIP_ERROR CheckinMessage::ParseCheckinMessagePayload(const Crypto::Aes128KeyHan { size_t appDataSize = GetAppDataSize(payload); - VerifyOrReturnError(payload.size() >= sMinPayloadSize, CHIP_ERROR_INVALID_MESSAGE_LENGTH); + 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_BUFFER_TOO_SMALL); @@ -105,6 +107,9 @@ CHIP_ERROR CheckinMessage::ParseCheckinMessagePayload(const Crypto::Aes128KeyHan // 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); @@ -136,7 +141,7 @@ CHIP_ERROR CheckinMessage::GenerateCheckInMessageNonce(const Crypto::Hmac128KeyH size_t CheckinMessage::GetAppDataSize(ByteSpan & payload) { - return (payload.size() <= sMinPayloadSize) ? 0 : payload.size() - sMinPayloadSize; + return (payload.size() <= kMinPayloadSize) ? 0 : payload.size() - kMinPayloadSize; } } // namespace SecureChannel diff --git a/src/protocols/secure_channel/CheckinMessage.h b/src/protocols/secure_channel/CheckinMessage.h index ffbda714356184..530c601a2432fe 100644 --- a/src/protocols/secure_channel/CheckinMessage.h +++ b/src/protocols/secure_channel/CheckinMessage.h @@ -58,6 +58,7 @@ class DLL_EXPORT CheckinMessage * * @return CHIP_ERROR_BUFFER_TOO_SMALL if output buffer is too small * CHIP_ERROR_INVALID_ARGUMENT if the provided arguments cannot be used to generate the Check-In message + * CHIP_ERROR_INTERNAL if an error occurs during the generation of the Check-In message */ static CHIP_ERROR GenerateCheckinMessagePayload(const Crypto::Aes128KeyHandle & aes128KeyHandle, const Crypto::Hmac128KeyHandle & hmacKeyHandle, const CounterType & counter, @@ -66,17 +67,16 @@ class DLL_EXPORT CheckinMessage /** * @brief Parse Check-in Message payload * - * @note Function requires two key handles to generate the Check-In message. - * Due to PSA requirements, the same key handle cannot be used for AES-CCM and HMAC-SHA-256 operations. + * @note Function requires two key handles to parse the Check-In message. + * Due to the way some key stores work, the same key handle cannot be used for AES-CCM and HMAC-SHA-256 operations. * - * @param[in] aes128KeyHandle Key handle with which to encrypt the check-in payload with the AEAD - * @param[in] hmac128KeyHandle Key handle with which to generate the nonce for the check-in payload with the HMAC + * @param[in] aes128KeyHandle Key handle with which to decrypt the received check-in payload (using AEAD). + * @param[in] hmac128KeyHandle Key handle with which to verify the received nonce in the check-in payload (using HMAC). * @param[in] payload The received payload to decrypt and parse * @param[out] counter The counter value retrieved from the payload - * @param[in,out] appData The optional application data decrypted. The input size of appData must be at least the size of - * GetAppDataSize(payload) + sizeof(CounterType), because - * appData is used as a work buffer for the decryption process. The output size - * on success will be GetAppDataSize(payload). + * @param[in,out] appData The optional application data decrypted. The input size of appData must be at least the + * size of GetAppDataSize(payload) + sizeof(CounterType), because appData is used as a work buffer for the decryption process. + * The output size on success will be GetAppDataSize(payload). * * @return CHIP_ERROR_INVALID_MESSAGE_LENGTH if the payload is shorter than the minimum payload size * CHIP_ERROR_BUFFER_TOO_SMALL if appData buffer is too small @@ -86,7 +86,7 @@ class DLL_EXPORT CheckinMessage const Crypto::Hmac128KeyHandle & hmacKeyHandle, ByteSpan & payload, CounterType & counter, MutableByteSpan & appData); - static inline size_t GetCheckinPayloadSize(size_t appDataSize) { return appDataSize + sMinPayloadSize; } + static inline size_t GetCheckinPayloadSize(size_t appDataSize) { return appDataSize + kMinPayloadSize; } /** * @brief Get the App Data Size @@ -96,7 +96,7 @@ class DLL_EXPORT CheckinMessage */ static size_t GetAppDataSize(ByteSpan & payload); - static constexpr uint16_t sMinPayloadSize = + static constexpr uint16_t kMinPayloadSize = CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES + sizeof(CounterType) + CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES; private: