From 4eb185e72dc3fb46797cc54a93ce2e2470a45667 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 18 Dec 2023 13:41:42 -0500 Subject: [PATCH 1/6] Add Nonce validation and parsing unit tests --- .../secure_channel/CheckinMessage.cpp | 19 +- src/protocols/secure_channel/CheckinMessage.h | 7 +- .../tests/CheckIn_Message_test_vectors.h | 42 +++ .../secure_channel/tests/TestCheckinMsg.cpp | 340 ++++++++++++------ 4 files changed, 300 insertions(+), 108 deletions(-) diff --git a/src/protocols/secure_channel/CheckinMessage.cpp b/src/protocols/secure_channel/CheckinMessage.cpp index 82f87cb478cb05..24fd07fba6be4f 100644 --- a/src/protocols/secure_channel/CheckinMessage.cpp +++ b/src/protocols/secure_channel/CheckinMessage.cpp @@ -105,10 +105,25 @@ CHIP_ERROR CheckinMessage::ParseCheckinMessagePayload(const Crypto::Aes128KeyHan } // Read decrypted counter and application data - counter = Encoding::LittleEndian::Get32(appData.data()); + static_assert(sizeof(CounterType) == sizeof(uint32_t), "Expect counter to be 32 bits for correct decoding"); + CounterType tempCounter = Encoding::LittleEndian::Get32(appData.data()); - // TODO : Validate received nonce by calculating it with the hmacKeyHandle and received Counter value + // Validate that the received nonce is correct + { + uint8_t calculatedNonceBuffer[CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES] = { 0 }; + Encoding::LittleEndian::BufferWriter writer(calculatedNonceBuffer, sizeof(calculatedNonceBuffer)); + + ReturnErrorOnFailure(GenerateCheckInMessageNonce(hmacKeyHandle, tempCounter, writer)); + + // Validate received nonce is the same as the calculated + ByteSpan nonce = payload.SubSpan(0, CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES); + VerifyOrReturnError(memcmp(nonce.data(), calculatedNonceBuffer, sizeof(calculatedNonceBuffer)) == 0, CHIP_ERROR_INTERNAL); + } + + // We have successfully decrypted and validated Check-In message + // Set output values + counter = tempCounter; // Shift to remove the counter from the appData memmove(appData.data(), sizeof(CounterType) + appData.data(), appDataSize); appData.reduce_size(appDataSize); diff --git a/src/protocols/secure_channel/CheckinMessage.h b/src/protocols/secure_channel/CheckinMessage.h index 530c601a2432fe..790c283c765158 100644 --- a/src/protocols/secure_channel/CheckinMessage.h +++ b/src/protocols/secure_channel/CheckinMessage.h @@ -74,13 +74,16 @@ class DLL_EXPORT CheckinMessage * @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 + * If an error occurs, no value will be set. * @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). + * 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). + * If an error occurs, appData might countain data, but the data CANNOT be used since we were not able to validate it. * * @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 * CHIP_ERROR_INVALID_ARGUMENT if the provided arguments cannot be used to parse the Check-In message + * CHIP_ERROR_INTERNAL if we were not able to decrypt or validate the Check-In message */ static CHIP_ERROR ParseCheckinMessagePayload(const Crypto::Aes128KeyHandle & aes128KeyHandle, const Crypto::Hmac128KeyHandle & hmacKeyHandle, ByteSpan & payload, diff --git a/src/protocols/secure_channel/tests/CheckIn_Message_test_vectors.h b/src/protocols/secure_channel/tests/CheckIn_Message_test_vectors.h index c49bf4e4766a85..5563150ebfee12 100644 --- a/src/protocols/secure_channel/tests/CheckIn_Message_test_vectors.h +++ b/src/protocols/secure_channel/tests/CheckIn_Message_test_vectors.h @@ -207,3 +207,45 @@ const CheckIn_Message_test_vector vector5 = { .key = kKey5, .payload_len = sizeof(kPayload5) }; const CheckIn_Message_test_vector checkIn_message_test_vectors[]{ vector1, vector2, vector3, vector4, vector5 }; + +/** + * Invalid Counter / Nonce Match vector + */ + +const uint8_t kInvalidNonceKey[] = { + 0xca, 0x67, 0xd4, 0x1f, 0xf7, 0x11, 0x29, 0x10, 0xfd, 0xd1, 0x8a, 0x1b, 0xf9, 0x9e, 0xa9, 0x74 +}; + +const uint8_t kInvalidNonceApplicationData[] = { 0x54, 0x68, 0x69, 0x73, 0x20, 0x69, 0x73, 0x20, 0x61, 0x20, 0x6c, + 0x6f, 0x6e, 0x67, 0x75, 0x65, 0x72, 0x20, 0x6c, 0x6f, 0x6e, 0x67, + 0x75, 0x65, 0x72, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6e, 0x67 }; + +const uint8_t kInvalidNonceNonce[] = { 0x06, 0x34, 0x67, 0x6e, 0xa6, 0xe0, 0x70, 0x7b, 0x7a, 0xd7, 0x81, 0x4f, 0xf8 }; + +const uint8_t kInvalidNonceCiphertext[] = { 0x29, 0x5b, 0x18, 0xd1, 0x9a, 0x23, 0xb2, 0xe4, 0xfa, 0xdf, 0x82, 0x92, + 0x53, 0x51, 0x7f, 0xf3, 0xc9, 0x1d, 0x9d, 0x50, 0xd6, 0x62, 0x42, 0x03, + 0x35, 0x01, 0xaa, 0x23, 0xad, 0x19, 0xcb, 0x6f, 0x5b, 0xee, 0x56, 0xb3 }; + +const uint8_t kInvalidNonceMic[] = { + 0xd5, 0x8a, 0x92, 0x2b, 0x66, 0x44, 0x89, 0x3e, 0x66, 0x31, 0x8b, 0xb5, 0x53, 0x79, 0x24, 0x8b +}; + +const uint8_t kInvalidNoncePayload[] = { 0x06, 0x34, 0x67, 0x6e, 0xa6, 0xe0, 0x70, 0x7b, 0x7a, 0xd7, 0x81, 0x4f, 0xf8, + 0x29, 0x5b, 0x18, 0xd1, 0x9a, 0x23, 0xb2, 0xe4, 0xfa, 0xdf, 0x82, 0x92, 0x53, + 0x51, 0x7f, 0xf3, 0xc9, 0x1d, 0x9d, 0x50, 0xd6, 0x62, 0x42, 0x03, 0x35, 0x01, + 0xaa, 0x23, 0xad, 0x19, 0xcb, 0x6f, 0x5b, 0xee, 0x56, 0xb3, 0xd5, 0x8a, 0x92, + 0x2b, 0x66, 0x44, 0x89, 0x3e, 0x66, 0x31, 0x8b, 0xb5, 0x53, 0x79, 0x24, 0x8b }; + +const CheckIn_Message_test_vector invalidNonceVector = { .key = kInvalidNonceKey, + .key_len = sizeof(kInvalidNonceKey), + .application_data = kInvalidNonceApplicationData, + .application_data_len = sizeof(kInvalidNonceApplicationData), + .counter = 12, + .nonce = kInvalidNonceNonce, + .nonce_len = sizeof(kInvalidNonceNonce), + .ciphertext = kInvalidNonceCiphertext, + .ciphertext_len = sizeof(kInvalidNonceCiphertext), + .mic = kInvalidNonceMic, + .mic_len = sizeof(kInvalidNonceMic), + .payload = kInvalidNoncePayload, + .payload_len = sizeof(kInvalidNoncePayload) }; diff --git a/src/protocols/secure_channel/tests/TestCheckinMsg.cpp b/src/protocols/secure_channel/tests/TestCheckinMsg.cpp index 8fe3a4eaeac768..887bf7c506d690 100644 --- a/src/protocols/secure_channel/tests/TestCheckinMsg.cpp +++ b/src/protocols/secure_channel/tests/TestCheckinMsg.cpp @@ -48,18 +48,27 @@ class TestCheckInMsg public: static void TestCheckinMessageGenerate_ValidInputsSameSizeOutputAsPayload(nlTestSuite * inSuite, void * inContext); static void TestCheckinMessageGenerate_ValidInputsBiggerSizeOutput(nlTestSuite * inSuite, void * inContext); - static void TestCheckInMessagePayloadSize(nlTestSuite * inSuite, void * inContext); - static void TestCheckInMessagePayloadSizeNullBuffer(nlTestSuite * inSuite, void * inContext); static void TestCheckinMessageGenerate_ValidInputsTooSmallOutput(nlTestSuite * inSuite, void * inContext); static void TestCheckInMessageGenerate_EmptyAesKeyHandle(nlTestSuite * inSuite, void * inContext); static void TestCheckInMessageGenerate_EmptyHmacKeyHandle(nlTestSuite * inSuite, void * inContext); + static void TestCheckinMessageParse_ValidInputsSameSizeMinAppData(nlTestSuite * inSuite, void * inContext); + static void TestCheckinMessageParse_ValidInputsBiggerSizeMinAppData(nlTestSuite * inSuite, void * inContext); + static void TestCheckinMessageParse_ValidInputsTooSmallAppData(nlTestSuite * inSuite, void * inContext); + static void TestCheckInMessageParse_EmptyAesKeyHandle(nlTestSuite * inSuite, void * inContext); + static void TestCheckInMessageParse_EmptyHmacKeyHandle(nlTestSuite * inSuite, void * inContext); + static void TestCheckInMessagePayloadSize(nlTestSuite * inSuite, void * inContext); + static void TestCheckInMessagePayloadSizeNullBuffer(nlTestSuite * inSuite, void * inContext); + static void TestCheckinMessageParse_CorruptedNonce(nlTestSuite * inSuite, void * inContext); + static void TestCheckinMessageParse_InvalidNonce(nlTestSuite * inSuite, void * inContext); - static void TestCheckinParse(nlTestSuite * inSuite, void * inContext); - static void TestCheckinGenerateParse(nlTestSuite * inSuite, void * inContext); + static void TestCheckinGenerateParseSanity(nlTestSuite * inSuite, void * inContext); private: static CHIP_ERROR GenerateAndVerifyPayload(nlTestSuite * inSuite, MutableByteSpan & output, const CheckIn_Message_test_vector & vector); + + static CHIP_ERROR ParseAndVerifyPayload(nlTestSuite * inSuite, MutableByteSpan & applicationData, + const CheckIn_Message_test_vector & vector, bool injectInvalidNonce); }; /** @@ -92,8 +101,15 @@ CHIP_ERROR TestCheckInMsg::GenerateAndVerifyPayload(nlTestSuite * inSuite, Mutab ByteSpan applicationData(vector.application_data, vector.application_data_len); // Verify that the generation succeeded - ReturnErrorOnFailure( - CheckinMessage::GenerateCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, vector.counter, applicationData, output)); + CHIP_ERROR err = + CheckinMessage::GenerateCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, vector.counter, applicationData, output); + if (err != CHIP_NO_ERROR) + { + keystore.DestroyKey(aes128KeyHandle); + keystore.DestroyKey(hmac128KeyHandle); + + return err; + } // Validate Full payload NL_TEST_ASSERT_EQUALS(inSuite, output.size(), vector.payload_len); @@ -120,7 +136,73 @@ CHIP_ERROR TestCheckInMsg::GenerateAndVerifyPayload(nlTestSuite * inSuite, Mutab keystore.DestroyKey(aes128KeyHandle); keystore.DestroyKey(hmac128KeyHandle); - return CHIP_NO_ERROR; + return err; +} + +/** + * @brief TODO + * + * @param inSuite + * @param applicationData + * @param vector + * @return CHIP_ERROR + */ +CHIP_ERROR TestCheckInMsg::ParseAndVerifyPayload(nlTestSuite * inSuite, MutableByteSpan & applicationData, + const CheckIn_Message_test_vector & vector, bool injectInvalidNonce) +{ + TestSessionKeystoreImpl keystore; + + // Copy payload to be able to modify it for invalid nonce tests + uint8_t payloadBuffer[300] = { 0 }; + memcpy(payloadBuffer, vector.payload, vector.payload_len); + + if (injectInvalidNonce) + { + // Modify nonce to validate that the parsing can detect that the message was manipulated + payloadBuffer[0] ^= 0xFF; + } + + // Create payload byte span + ByteSpan payload(payloadBuffer, vector.payload_len); + + CounterType decryptedCounter = 0; + + // Two distinct key material buffers to ensure crypto-hardware-assist with single-usage keys create two different handles. + Symmetric128BitsKeyByteArray aesKeyMaterial; + memcpy(aesKeyMaterial, vector.key, vector.key_len); + + Symmetric128BitsKeyByteArray hmacKeyMaterial; + memcpy(hmacKeyMaterial, vector.key, vector.key_len); + + Aes128KeyHandle aes128KeyHandle; + NL_TEST_ASSERT_SUCCESS(inSuite, keystore.CreateKey(aesKeyMaterial, aes128KeyHandle)); + + Hmac128KeyHandle hmac128KeyHandle; + NL_TEST_ASSERT_SUCCESS(inSuite, keystore.CreateKey(hmacKeyMaterial, hmac128KeyHandle)); + + // Verify that the Parsing succeeded + CHIP_ERROR err = + CheckinMessage::ParseCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, payload, decryptedCounter, applicationData); + if (err != CHIP_NO_ERROR) + { + keystore.DestroyKey(aes128KeyHandle); + keystore.DestroyKey(hmac128KeyHandle); + + return err; + } + + // Verify decrypted counter value + NL_TEST_ASSERT_EQUALS(inSuite, vector.counter, decryptedCounter); + + // Verify application data + NL_TEST_ASSERT_EQUALS(inSuite, vector.application_data_len, applicationData.size()); + NL_TEST_ASSERT(inSuite, memcmp(vector.application_data, applicationData.data(), applicationData.size()) == 0); + + // Cleanup + keystore.DestroyKey(aes128KeyHandle); + keystore.DestroyKey(hmac128KeyHandle); + + return err; } /** @@ -203,16 +285,16 @@ void TestCheckInMsg::TestCheckInMessageGenerate_EmptyAesKeyHandle(nlTestSuite * ByteSpan applicationData(vector.application_data, vector.application_data_len); /* - Passing an empty key handle while using PSA crypto will result in a failure. - When using OpenSSL this same test result in a success. - Issue #28986 - - Verify that the generation fails with an empty key handle + TODO(#28986): Passing an empty key handle while using PSA crypto will result in a failure. + When using OpenSSL this same test result in a success. + */ +#if 0 + // Verify that the generation fails with an empty key handle NL_TEST_ASSERT_(inSuite, CHIP_NO_ERROR != CheckinMessage::GenerateCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, vector.counter, applicationData, output)); - */ +#endif // Clean up keystore.DestroyKey(hmac128KeyHandle); @@ -245,133 +327,179 @@ void TestCheckInMsg::TestCheckInMessageGenerate_EmptyHmacKeyHandle(nlTestSuite * ByteSpan applicationData(vector.application_data, vector.application_data_len); /* - Passing an empty key handle while using PSA crypto will result in a failure. - When using OpenSSL this same test result in a success. - Issue #28986 - - Verify that the generation fails with an empty key handle + TODO(#28986): Passing an empty key handle while using PSA crypto will result in a failure. + When using OpenSSL this same test result in a success. + */ +#if 0 + // Verify that the generation fails with an empty key handle NL_TEST_ASSERT_(inSuite, CHIP_NO_ERROR != CheckinMessage::GenerateCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, vector.counter, applicationData, output)); - */ +#endif // Clean up keystore.DestroyKey(aes128KeyHandle); } -void TestCheckInMsg::TestCheckinParse(nlTestSuite * inSuite, void * inContext) +void TestCheckInMsg::TestCheckinMessageParse_ValidInputsSameSizeMinAppData(nlTestSuite * inSuite, void * inContext) { - uint8_t a[300] = { 0 }; - uint8_t b[300] = { 0 }; - MutableByteSpan outputBuffer{ a }; - MutableByteSpan buffer{ b }; - uint32_t counter = 0, decryptedCounter; - ByteSpan userData; + int numOfTestCases = ArraySize(checkIn_message_test_vectors); + for (int numOfTestsExecuted = 0; numOfTestsExecuted < numOfTestCases; numOfTestsExecuted++) + { + CheckIn_Message_test_vector vector = checkIn_message_test_vectors[numOfTestsExecuted]; - CHIP_ERROR err = CHIP_NO_ERROR; + uint8_t applicationDataBuffer[128] = { 0 }; + MutableByteSpan applicationData(applicationDataBuffer, sizeof(applicationDataBuffer)); + applicationData.reduce_size(vector.application_data_len + sizeof(CounterType)); - TestSessionKeystoreImpl keystore; + NL_TEST_ASSERT_SUCCESS(inSuite, ParseAndVerifyPayload(inSuite, applicationData, vector, false)); + } +} - // Verify User Data Encryption Decryption - uint8_t data[] = { "This is some user Data. It should be encrypted" }; - userData = chip::ByteSpan(data); - const ccm_128_test_vector & test = *ccm_128_test_vectors[0]; +void TestCheckInMsg::TestCheckinMessageParse_ValidInputsBiggerSizeMinAppData(nlTestSuite * inSuite, void * inContext) +{ + int numOfTestCases = ArraySize(checkIn_message_test_vectors); + for (int numOfTestsExecuted = 0; numOfTestsExecuted < numOfTestCases; numOfTestsExecuted++) + { + CheckIn_Message_test_vector vector = checkIn_message_test_vectors[numOfTestsExecuted]; - // Two distinct key material buffers to ensure crypto-hardware-assist with single-usage keys create two different handles. - Symmetric128BitsKeyByteArray aesKeyMaterial; - memcpy(aesKeyMaterial, test.key, test.key_len); + uint8_t applicationDataBuffer[128] = { 0 }; + MutableByteSpan applicationData(applicationDataBuffer, sizeof(applicationDataBuffer)); - Symmetric128BitsKeyByteArray hmacKeyMaterial; - memcpy(hmacKeyMaterial, test.key, test.key_len); + NL_TEST_ASSERT_SUCCESS(inSuite, ParseAndVerifyPayload(inSuite, applicationData, vector, false)); + } +} - Aes128KeyHandle aes128KeyHandle; - NL_TEST_ASSERT_SUCCESS(inSuite, keystore.CreateKey(aesKeyMaterial, aes128KeyHandle)); +void TestCheckInMsg::TestCheckinMessageParse_ValidInputsTooSmallAppData(nlTestSuite * inSuite, void * inContext) +{ + CheckIn_Message_test_vector vector = checkIn_message_test_vectors[0]; - Hmac128KeyHandle hmac128KeyHandle; - NL_TEST_ASSERT_SUCCESS(inSuite, keystore.CreateKey(hmacKeyMaterial, hmac128KeyHandle)); + // Create applicationData buffer with 0 size + MutableByteSpan applicationData; + + NL_TEST_ASSERT(inSuite, CHIP_ERROR_BUFFER_TOO_SMALL == ParseAndVerifyPayload(inSuite, applicationData, vector, false)); +} + +/** + * @brief Test verifies that the Check-In Message parsing returns an error if the AesKeyHandle is empty + */ +void TestCheckInMsg::TestCheckInMessageParse_EmptyAesKeyHandle(nlTestSuite * inSuite, void * inContexT) +{ + TestSessionKeystoreImpl keystore; + CheckIn_Message_test_vector vector = checkIn_message_test_vectors[0]; - //=================Encrypt======================= + // Create application data ByteSpan + uint8_t applicationDataBuffer[128] = { 0 }; + MutableByteSpan applicationData(applicationDataBuffer, sizeof(applicationDataBuffer)); + applicationData.reduce_size(vector.application_data_len + sizeof(CounterType)); - err = CheckinMessage::GenerateCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, counter, userData, outputBuffer); - ByteSpan payload = chip::ByteSpan(outputBuffer.data(), outputBuffer.size()); - NL_TEST_ASSERT(inSuite, (CHIP_NO_ERROR == err)); + // Create payload byte span + ByteSpan payload(vector.payload, vector.payload_len); - //=================Decrypt======================= + CounterType decryptedCounter = 0; + // + (void) decryptedCounter; - MutableByteSpan empty; - err = CheckinMessage::ParseCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, payload, decryptedCounter, empty); - NL_TEST_ASSERT(inSuite, (CHIP_NO_ERROR != err)); + // Empty AES Key handle + Aes128KeyHandle aes128KeyHandle; - ByteSpan emptyPayload; - err = CheckinMessage::ParseCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, emptyPayload, decryptedCounter, buffer); - NL_TEST_ASSERT(inSuite, (CHIP_NO_ERROR != err)); + Symmetric128BitsKeyByteArray hmacKeyMaterial; + memcpy(hmacKeyMaterial, vector.key, vector.key_len); - // Cleanup - keystore.DestroyKey(aes128KeyHandle); + Hmac128KeyHandle hmac128KeyHandle; + NL_TEST_ASSERT_SUCCESS(inSuite, keystore.CreateKey(hmacKeyMaterial, hmac128KeyHandle)); + + /* + TODO(#28986): Passing an empty key handle while using PSA crypto will result in a failure. + When using OpenSSL this same test result in a success. + */ +#if 0 + // Verify that the generation fails with an empty key handle + NL_TEST_ASSERT_(inSuite, + CHIP_ERROR err != + CheckinMessage::ParseCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, payload, decryptedCounter, applicationData)); +#endif + + // Clean up keystore.DestroyKey(hmac128KeyHandle); } -void TestCheckInMsg::TestCheckinGenerateParse(nlTestSuite * inSuite, void * inContext) +/** + * @brief Test verifies that the Check-In Message parsing returns an error if the HmacKeyHandle is empty + */ +void TestCheckInMsg::TestCheckInMessageParse_EmptyHmacKeyHandle(nlTestSuite * inSuite, void * inContexT) { - uint8_t a[300] = { 0 }; - uint8_t b[300] = { 0 }; - MutableByteSpan outputBuffer{ a }; - MutableByteSpan buffer{ b }; - uint32_t counter = 0xDEADBEEF; - ByteSpan userData; - - CHIP_ERROR err = CHIP_NO_ERROR; - TestSessionKeystoreImpl keystore; + CheckIn_Message_test_vector vector = checkIn_message_test_vectors[0]; - // Verify User Data Encryption Decryption - uint8_t data[] = { "This is some user Data. It should be encrypted" }; - userData = chip::ByteSpan(data); - for (const ccm_128_test_vector * testPtr : ccm_128_test_vectors) - { - const ccm_128_test_vector & test = *testPtr; + // Create application data ByteSpan + uint8_t applicationDataBuffer[128] = { 0 }; + MutableByteSpan applicationData(applicationDataBuffer, sizeof(applicationDataBuffer)); + applicationData.reduce_size(vector.application_data_len + sizeof(CounterType)); - // Two disctint key material to force the PSA unit tests to create two different Key IDs - Symmetric128BitsKeyByteArray aesKeyMaterial; - memcpy(aesKeyMaterial, test.key, test.key_len); + // Create payload byte span + ByteSpan payload(vector.payload, vector.payload_len); - Symmetric128BitsKeyByteArray hmacKeyMaterial; - memcpy(hmacKeyMaterial, test.key, test.key_len); + CounterType decryptedCounter = 0; + // + (void) decryptedCounter; - Aes128KeyHandle aes128KeyHandle; - NL_TEST_ASSERT_SUCCESS(inSuite, keystore.CreateKey(aesKeyMaterial, aes128KeyHandle)); + // Empty Hmac Key handle + Hmac128KeyHandle hmac128KeyHandle; - Hmac128KeyHandle hmac128KeyHandle; - NL_TEST_ASSERT_SUCCESS(inSuite, keystore.CreateKey(hmacKeyMaterial, hmac128KeyHandle)); + Symmetric128BitsKeyByteArray aesKeyMaterial; + memcpy(aesKeyMaterial, vector.key, vector.key_len); - //=================Encrypt======================= + Aes128KeyHandle aes128KeyHandle; + NL_TEST_ASSERT_SUCCESS(inSuite, keystore.CreateKey(aesKeyMaterial, aes128KeyHandle)); + + /* + TODO(#28986): Passing an empty key handle while using PSA crypto will result in a failure. + When using OpenSSL this same test result in a success. + */ +#if 0 + // Verify that the generation fails with an empty key handle + NL_TEST_ASSERT_(inSuite, + CHIP_ERROR err != + CheckinMessage::ParseCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, payload, decryptedCounter, applicationData)); +#endif - err = CheckinMessage::GenerateCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, counter, userData, outputBuffer); - NL_TEST_ASSERT(inSuite, (CHIP_NO_ERROR == err)); + // Clean up + keystore.DestroyKey(aes128KeyHandle); +} - //=================Decrypt======================= - uint32_t decryptedCounter = 0; - ByteSpan payload = chip::ByteSpan(outputBuffer.data(), outputBuffer.size()); +/** + * @brief Test verifies that the Check-In message processing throws an error if the nonce is corrupted + */ +void TestCheckInMsg::TestCheckinMessageParse_CorruptedNonce(nlTestSuite * inSuite, void * inContext) +{ + int numOfTestCases = ArraySize(checkIn_message_test_vectors); + for (int numOfTestsExecuted = 0; numOfTestsExecuted < numOfTestCases; numOfTestsExecuted++) + { + CheckIn_Message_test_vector vector = checkIn_message_test_vectors[numOfTestsExecuted]; - err = CheckinMessage::ParseCheckinMessagePayload(aes128KeyHandle, hmac128KeyHandle, payload, decryptedCounter, buffer); - NL_TEST_ASSERT(inSuite, (CHIP_NO_ERROR == err)); + uint8_t applicationDataBuffer[128] = { 0 }; + MutableByteSpan applicationData(applicationDataBuffer, sizeof(applicationDataBuffer)); + applicationData.reduce_size(vector.application_data_len + sizeof(CounterType)); - NL_TEST_ASSERT(inSuite, (memcmp(data, buffer.data(), sizeof(data)) == 0)); - NL_TEST_ASSERT(inSuite, (counter == decryptedCounter)); + NL_TEST_ASSERT(inSuite, CHIP_ERROR_INTERNAL == ParseAndVerifyPayload(inSuite, applicationData, vector, true)); + } +} - // reset buffers - memset(a, 0, sizeof(a)); - memset(b, 0, sizeof(b)); - outputBuffer = MutableByteSpan(a); - buffer = MutableByteSpan(b); +/** + * @brief Test verifies that the Check-In message processing throws an error if the nonce was not calculated with the counter in the + * payload + */ +void TestCheckInMsg::TestCheckinMessageParse_InvalidNonce(nlTestSuite * inSuite, void * inContext) +{ + CheckIn_Message_test_vector vector = invalidNonceVector; - counter += chip::Crypto::GetRandU32() + 1; + uint8_t applicationDataBuffer[128] = { 0 }; + MutableByteSpan applicationData(applicationDataBuffer, sizeof(applicationDataBuffer)); + applicationData.reduce_size(vector.application_data_len + sizeof(CounterType)); - // Cleanup - keystore.DestroyKey(aes128KeyHandle); - keystore.DestroyKey(hmac128KeyHandle); - } + NL_TEST_ASSERT(inSuite, CHIP_ERROR_INTERNAL == ParseAndVerifyPayload(inSuite, applicationData, vector, false)); } /** @@ -419,13 +547,17 @@ static const nlTest sTests[] = NL_TEST_DEF("TestCheckinMessageGenerate_ValidInputsSameSizeOutputAsPayload", TestCheckInMsg::TestCheckinMessageGenerate_ValidInputsSameSizeOutputAsPayload), NL_TEST_DEF("TestCheckinMessageGenerate_ValidInputsBiggerSizeOutput", TestCheckInMsg::TestCheckinMessageGenerate_ValidInputsBiggerSizeOutput), NL_TEST_DEF("TestCheckinMessageGenerate_ValidInputsTooSmallOutput", TestCheckInMsg::TestCheckinMessageGenerate_ValidInputsTooSmallOutput), - NL_TEST_DEF("TestCheckinMessageGenerate_ValidInputsTooSmallOutput", TestCheckInMsg::TestCheckInMessageGenerate_EmptyAesKeyHandle), - NL_TEST_DEF("TestCheckinMessageGenerate_ValidInputsTooSmallOutput", TestCheckInMsg::TestCheckInMessageGenerate_EmptyHmacKeyHandle), - NL_TEST_DEF("TestCheckinParse", TestCheckInMsg::TestCheckinParse), - NL_TEST_DEF("TestCheckinGenerateParse", TestCheckInMsg::TestCheckinGenerateParse), + NL_TEST_DEF("TestCheckInMessageGenerate_EmptyAesKeyHandle", TestCheckInMsg::TestCheckInMessageGenerate_EmptyAesKeyHandle), + NL_TEST_DEF("TestCheckInMessageGenerate_EmptyHmacKeyHandle", TestCheckInMsg::TestCheckInMessageGenerate_EmptyHmacKeyHandle), + NL_TEST_DEF("TestCheckinMessageParse_ValidInputsSameSizeMinAppData", TestCheckInMsg::TestCheckinMessageParse_ValidInputsSameSizeMinAppData), + NL_TEST_DEF("TestCheckinMessageParse_ValidInputsBiggerSizeMinAppData", TestCheckInMsg::TestCheckinMessageParse_ValidInputsBiggerSizeMinAppData), + NL_TEST_DEF("TestCheckinMessageParse_ValidInputsTooSmallAppData", TestCheckInMsg::TestCheckinMessageParse_ValidInputsTooSmallAppData), + NL_TEST_DEF("TestCheckInMessageParse_EmptyAesKeyHandle", TestCheckInMsg::TestCheckInMessageParse_EmptyAesKeyHandle), + NL_TEST_DEF("TestCheckInMessageParse_EmptyHmacKeyHandle", TestCheckInMsg::TestCheckInMessageParse_EmptyHmacKeyHandle), + NL_TEST_DEF("TestCheckinMessageParse_CorruptedNonce", TestCheckInMsg::TestCheckinMessageParse_CorruptedNonce), + NL_TEST_DEF("TestCheckinMessageParse_InvalidNonce", TestCheckInMsg::TestCheckinMessageParse_InvalidNonce), NL_TEST_DEF("TestCheckInMessagePayloadSize", TestCheckInMsg::TestCheckInMessagePayloadSize), NL_TEST_DEF("TestCheckInMessagePayloadSizeNullBuffer", TestCheckInMsg::TestCheckInMessagePayloadSizeNullBuffer), - NL_TEST_SENTINEL() }; // clang-format on From ca1f9b45a785ab33e54fb91ece86ddad1cea54b9 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 18 Dec 2023 13:46:11 -0500 Subject: [PATCH 2/6] Add missing comments --- .../secure_channel/tests/TestCheckinMsg.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/protocols/secure_channel/tests/TestCheckinMsg.cpp b/src/protocols/secure_channel/tests/TestCheckinMsg.cpp index 887bf7c506d690..a883ab457b3828 100644 --- a/src/protocols/secure_channel/tests/TestCheckinMsg.cpp +++ b/src/protocols/secure_channel/tests/TestCheckinMsg.cpp @@ -140,12 +140,12 @@ CHIP_ERROR TestCheckInMsg::GenerateAndVerifyPayload(nlTestSuite * inSuite, Mutab } /** - * @brief TODO + * @brief Helper function that parses the Check-In message based on the test vector + * and verifies parsed Check-In message + * Helper is to avoid having the same code in multiple tests * - * @param inSuite - * @param applicationData - * @param vector - * @return CHIP_ERROR + * @return CHIP_NO_ERROR if the parsing was successful + * error code if the generation failed - see ParseCheckinMessagePayload */ CHIP_ERROR TestCheckInMsg::ParseAndVerifyPayload(nlTestSuite * inSuite, MutableByteSpan & applicationData, const CheckIn_Message_test_vector & vector, bool injectInvalidNonce) @@ -342,6 +342,9 @@ void TestCheckInMsg::TestCheckInMessageGenerate_EmptyHmacKeyHandle(nlTestSuite * keystore.DestroyKey(aes128KeyHandle); } +/** + * @brief Test verifies that the Check-In message parsing succeeds with the Application buffer set to the minimum required size + */ void TestCheckInMsg::TestCheckinMessageParse_ValidInputsSameSizeMinAppData(nlTestSuite * inSuite, void * inContext) { int numOfTestCases = ArraySize(checkIn_message_test_vectors); @@ -357,6 +360,9 @@ void TestCheckInMsg::TestCheckinMessageParse_ValidInputsSameSizeMinAppData(nlTes } } +/** + * @brief Test verifies that the Check-In message parsing succeeds with the Application buffer set to a larger than necessary size + */ void TestCheckInMsg::TestCheckinMessageParse_ValidInputsBiggerSizeMinAppData(nlTestSuite * inSuite, void * inContext) { int numOfTestCases = ArraySize(checkIn_message_test_vectors); @@ -371,6 +377,9 @@ void TestCheckInMsg::TestCheckinMessageParse_ValidInputsBiggerSizeMinAppData(nlT } } +/** + * @brief Test verifies that the Check-In message throws an error if the application data buffer is too small + */ void TestCheckInMsg::TestCheckinMessageParse_ValidInputsTooSmallAppData(nlTestSuite * inSuite, void * inContext) { CheckIn_Message_test_vector vector = checkIn_message_test_vectors[0]; From efd585f5d2de017cca57afaff8fe91c67209f0a1 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 18 Dec 2023 13:46:58 -0500 Subject: [PATCH 3/6] Remove unused test signature --- src/protocols/secure_channel/tests/TestCheckinMsg.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/protocols/secure_channel/tests/TestCheckinMsg.cpp b/src/protocols/secure_channel/tests/TestCheckinMsg.cpp index a883ab457b3828..820e876deb42d9 100644 --- a/src/protocols/secure_channel/tests/TestCheckinMsg.cpp +++ b/src/protocols/secure_channel/tests/TestCheckinMsg.cpp @@ -61,8 +61,6 @@ class TestCheckInMsg static void TestCheckinMessageParse_CorruptedNonce(nlTestSuite * inSuite, void * inContext); static void TestCheckinMessageParse_InvalidNonce(nlTestSuite * inSuite, void * inContext); - static void TestCheckinGenerateParseSanity(nlTestSuite * inSuite, void * inContext); - private: static CHIP_ERROR GenerateAndVerifyPayload(nlTestSuite * inSuite, MutableByteSpan & output, const CheckIn_Message_test_vector & vector); From a344d283fdab047576b0f10c4e3dd6af7d33608f Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 18 Dec 2023 14:50:51 -0500 Subject: [PATCH 4/6] fix comment --- src/protocols/secure_channel/CheckinMessage.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/protocols/secure_channel/CheckinMessage.h b/src/protocols/secure_channel/CheckinMessage.h index 790c283c765158..baa10e9915419e 100644 --- a/src/protocols/secure_channel/CheckinMessage.h +++ b/src/protocols/secure_channel/CheckinMessage.h @@ -82,7 +82,6 @@ class DLL_EXPORT CheckinMessage * * @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 - * CHIP_ERROR_INVALID_ARGUMENT if the provided arguments cannot be used to parse the Check-In message * CHIP_ERROR_INTERNAL if we were not able to decrypt or validate the Check-In message */ static CHIP_ERROR ParseCheckinMessagePayload(const Crypto::Aes128KeyHandle & aes128KeyHandle, From 25a2c3417ddba5c1f74d452157d85b03b474ace9 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 18 Dec 2023 14:50:59 -0500 Subject: [PATCH 5/6] remove include --- src/protocols/secure_channel/tests/TestCheckinMsg.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/protocols/secure_channel/tests/TestCheckinMsg.cpp b/src/protocols/secure_channel/tests/TestCheckinMsg.cpp index 820e876deb42d9..f756b627defef9 100644 --- a/src/protocols/secure_channel/tests/TestCheckinMsg.cpp +++ b/src/protocols/secure_channel/tests/TestCheckinMsg.cpp @@ -20,19 +20,15 @@ #include #include #include +#include #include +#include #include #include #include #include #include #include -// AES_CCM_128_test_vectors is being replaced by the CheckIn_Message_test_vectors -// New tests need to use the CheckIn_Message_test_vectors -#include -#include -#include -#include using namespace chip; using namespace chip::Protocols; From 077d51b08b938273e87b6fa7841ceff852ab9793 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Dec 2023 09:09:59 -0500 Subject: [PATCH 6/6] fix comment --- src/protocols/secure_channel/CheckinMessage.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/protocols/secure_channel/CheckinMessage.h b/src/protocols/secure_channel/CheckinMessage.h index baa10e9915419e..a0123f3749ce2b 100644 --- a/src/protocols/secure_channel/CheckinMessage.h +++ b/src/protocols/secure_channel/CheckinMessage.h @@ -77,8 +77,9 @@ class DLL_EXPORT CheckinMessage * If an error occurs, no value will be set. * @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). - * If an error occurs, appData might countain data, but the data CANNOT be used since we were not able to validate it. + * buffer for the decryption process. The output size on success will be + * GetAppDataSize(payload). If an error occurs, appData might countain data, + * but the data CANNOT be used since we were not able to validate it. * * @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