From e2e5993ad157113a9a91ed610da29a6fd9db9a0e Mon Sep 17 00:00:00 2001 From: "D. Cody Cutright" Date: Thu, 22 Jun 2023 11:47:03 -0400 Subject: [PATCH 1/3] Fix for Issue 139 --- src/core/crypto_tc.c | 2 +- test/unit/ut_tc_apply.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/core/crypto_tc.c b/src/core/crypto_tc.c index 97a6f58a..0a22b8b7 100644 --- a/src/core/crypto_tc.c +++ b/src/core/crypto_tc.c @@ -280,7 +280,7 @@ int32_t Crypto_TC_ApplySecurity_Cam(const uint8_t* p_in_frame, const uint16_t in { case SA_PLAINTEXT: // Ingest length + spi_index (2) + some variable length fields - *p_enc_frame_len = temp_tc_header.fl + 1 + 2 + sa_ptr->shplf_len; + *p_enc_frame_len = temp_tc_header.fl + 1 + 2 + sa_ptr->shsnf_len + sa_ptr->shplf_len; new_enc_frame_header_field_length = (*p_enc_frame_len) - 1; break; case SA_AUTHENTICATION: diff --git a/test/unit/ut_tc_apply.c b/test/unit/ut_tc_apply.c index 51cd5deb..9bb2aaf7 100644 --- a/test/unit/ut_tc_apply.c +++ b/test/unit/ut_tc_apply.c @@ -38,6 +38,7 @@ UTEST(TC_APPLY_SECURITY, NO_CRYPTO_INIT) int raw_tc_sdls_ping_len = 0; hex_conversion(raw_tc_sdls_ping_h, &raw_tc_sdls_ping_b, &raw_tc_sdls_ping_len); + Crypto_Config_CryptoLib(KEY_TYPE_INTERNAL, SADB_TYPE_INMEMORY, CRYPTOGRAPHY_TYPE_LIBGCRYPT, CRYPTO_TC_CREATE_FECF_TRUE, TC_PROCESS_SDLS_PDUS_TRUE, TC_HAS_PUS_HDR, TC_IGNORE_SA_STATE_FALSE, TC_IGNORE_ANTI_REPLAY_FALSE, TC_UNIQUE_SA_PER_MAP_ID_TRUE, TC_CHECK_FECF_TRUE, 0x3F, SA_INCREMENT_NONTRANSMITTED_IV_TRUE); @@ -1117,5 +1118,45 @@ UTEST(TC_APPLY_SECURITY, CBC_NULL_IV_W_IVH) free(ptr_enc_frame); } +UTEST(TC_APPLY_SECURITY, ISSUE_139_TEST) +{ + // Setup & Initialize CryptoLib + Crypto_Config_CryptoLib(KEY_TYPE_INTERNAL, SADB_TYPE_INMEMORY, CRYPTOGRAPHY_TYPE_LIBGCRYPT, CRYPTO_TC_CREATE_FECF_TRUE, TC_PROCESS_SDLS_PDUS_TRUE, TC_HAS_PUS_HDR, + TC_IGNORE_SA_STATE_FALSE, TC_IGNORE_ANTI_REPLAY_FALSE, TC_UNIQUE_SA_PER_MAP_ID_TRUE, + TC_CHECK_FECF_TRUE, 0x3F, SA_INCREMENT_NONTRANSMITTED_IV_TRUE); + Crypto_Config_Add_Gvcid_Managed_Parameter(0, 0x0003, 0, TC_HAS_FECF, TC_NO_SEGMENT_HDRS, 1024); + Crypto_Init(); + // Ibraheem's test string + char* raw_tc_sdls_ping_h = "2003001F00000100011880D2C9000E197F0B001B0004000400003040D95E0000"; + char* raw_tc_sdls_ping_b = NULL; + int raw_tc_sdls_ping_len = 0; + + hex_conversion(raw_tc_sdls_ping_h, &raw_tc_sdls_ping_b, &raw_tc_sdls_ping_len); + + uint8_t* ptr_enc_frame = NULL; + uint16_t enc_frame_len = 0; + int32_t return_val = CRYPTO_LIB_ERROR; + + return_val = +Crypto_TC_ApplySecurity((uint8_t* )raw_tc_sdls_ping_b, raw_tc_sdls_ping_len, &ptr_enc_frame, &enc_frame_len); + + char* truth_data_h = "200300230000010000000100011880D2C9000E197F0B001B0004000400003040D95E85F3"; + uint8_t* truth_data_b = NULL; + int truth_data_l = 0; + + hex_conversion(truth_data_h, (char **)&truth_data_b, &truth_data_l); + //printf("Encrypted Frame:\n"); + for(int i = 0; i < enc_frame_len; i++) + { + printf("%02x -> %02x ", ptr_enc_frame[i], truth_data_b[i]); + ASSERT_EQ(ptr_enc_frame[i], truth_data_b[i]); + } + +Crypto_Shutdown(); +free(raw_tc_sdls_ping_b); +free(ptr_enc_frame); +ASSERT_EQ(CRYPTO_LIB_SUCCESS, return_val); +} + UTEST_MAIN(); From f3dbe299b9f9490135197019e47d23d71e99f3c0 Mon Sep 17 00:00:00 2001 From: "D. Cody Cutright" Date: Thu, 22 Jun 2023 13:25:03 -0400 Subject: [PATCH 2/3] Additional change to byte counting logic --- src/core/crypto_tc.c | 13 ++++++++++++- test/unit/ut_tc_apply.c | 8 ++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/core/crypto_tc.c b/src/core/crypto_tc.c index 0a22b8b7..91ae87e8 100644 --- a/src/core/crypto_tc.c +++ b/src/core/crypto_tc.c @@ -278,9 +278,20 @@ int32_t Crypto_TC_ApplySecurity_Cam(const uint8_t* p_in_frame, const uint16_t in switch (sa_service_type) { + /** + * A note on plaintext: Take a permissive approach to allow the lengths of fields that aren't going to be used. + * The 355.0-B-2 (July 2022) says the following in $4.2.2.4: + * 'It is possible to create a ‘clear mode’ SA using one of the defined service types by + specifying the algorithm as a ‘no-op’ function (no actual cryptographic operation to + be performed). Such an SA might be used, for example, during development + testing of other aspects of data link processing before cryptographic capabilities are + available for integrated testing.In this scenario, the Security Header and Trailer + field lengths are kept constant across all supported configurations. For security + reasons, the use of such an SA is not recommended in normal operation.' + */ case SA_PLAINTEXT: // Ingest length + spi_index (2) + some variable length fields - *p_enc_frame_len = temp_tc_header.fl + 1 + 2 + sa_ptr->shsnf_len + sa_ptr->shplf_len; + *p_enc_frame_len = temp_tc_header.fl + 1 + 2 + sa_ptr->shivf_len + sa_ptr->shsnf_len + sa_ptr->shplf_len + sa_ptr->stmacf_len; new_enc_frame_header_field_length = (*p_enc_frame_len) - 1; break; case SA_AUTHENTICATION: diff --git a/test/unit/ut_tc_apply.c b/test/unit/ut_tc_apply.c index 9bb2aaf7..963b4164 100644 --- a/test/unit/ut_tc_apply.c +++ b/test/unit/ut_tc_apply.c @@ -1118,7 +1118,11 @@ UTEST(TC_APPLY_SECURITY, CBC_NULL_IV_W_IVH) free(ptr_enc_frame); } -UTEST(TC_APPLY_SECURITY, ISSUE_139_TEST) +/* +* @brief: Unit Test: Verify plaintext with an ARSN places bytes appropriately. +* Use a full length verification of all bytes. Exists due to found bug. +*/ +UTEST(TC_APPLY_SECURITY, PLAINTEXT_W_ARSN) { // Setup & Initialize CryptoLib Crypto_Config_CryptoLib(KEY_TYPE_INTERNAL, SADB_TYPE_INMEMORY, CRYPTOGRAPHY_TYPE_LIBGCRYPT, CRYPTO_TC_CREATE_FECF_TRUE, TC_PROCESS_SDLS_PDUS_TRUE, TC_HAS_PUS_HDR, @@ -1126,7 +1130,7 @@ UTEST(TC_APPLY_SECURITY, ISSUE_139_TEST) TC_CHECK_FECF_TRUE, 0x3F, SA_INCREMENT_NONTRANSMITTED_IV_TRUE); Crypto_Config_Add_Gvcid_Managed_Parameter(0, 0x0003, 0, TC_HAS_FECF, TC_NO_SEGMENT_HDRS, 1024); Crypto_Init(); - // Ibraheem's test string + // Test string char* raw_tc_sdls_ping_h = "2003001F00000100011880D2C9000E197F0B001B0004000400003040D95E0000"; char* raw_tc_sdls_ping_b = NULL; int raw_tc_sdls_ping_len = 0; From ff42980f195a232c6033a6ec5d09ace948a970e1 Mon Sep 17 00:00:00 2001 From: "D. Cody Cutright" Date: Thu, 22 Jun 2023 13:37:43 -0400 Subject: [PATCH 3/3] Updated length checking logic to be more dynamic and not rely on cases --- src/core/crypto_tc.c | 41 ++++++++++------------------------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/src/core/crypto_tc.c b/src/core/crypto_tc.c index 91ae87e8..3bdba189 100644 --- a/src/core/crypto_tc.c +++ b/src/core/crypto_tc.c @@ -276,8 +276,6 @@ int32_t Crypto_TC_ApplySecurity_Cam(const uint8_t* p_in_frame, const uint16_t in // Calculate tf_payload length here to be used in other logic tf_payload_len = temp_tc_header.fl - TC_FRAME_HEADER_SIZE - segment_hdr_len - fecf_len + 1; - switch (sa_service_type) - { /** * A note on plaintext: Take a permissive approach to allow the lengths of fields that aren't going to be used. * The 355.0-B-2 (July 2022) says the following in $4.2.2.4: @@ -289,26 +287,13 @@ int32_t Crypto_TC_ApplySecurity_Cam(const uint8_t* p_in_frame, const uint16_t in field lengths are kept constant across all supported configurations. For security reasons, the use of such an SA is not recommended in normal operation.' */ - case SA_PLAINTEXT: - // Ingest length + spi_index (2) + some variable length fields - *p_enc_frame_len = temp_tc_header.fl + 1 + 2 + sa_ptr->shivf_len + sa_ptr->shsnf_len + sa_ptr->shplf_len + sa_ptr->stmacf_len; - new_enc_frame_header_field_length = (*p_enc_frame_len) - 1; - break; - case SA_AUTHENTICATION: - // Ingest length + spi_index (2) + shivf_len (varies) + shsnf_len (varies) - // + shplf_len + arsn_len + pad_size + stmacf_len - // TODO: If ARSN is transmitted in the SHSNF field (as in CMAC... don't double count those bytes) - *p_enc_frame_len = temp_tc_header.fl + 1 + 2 + sa_ptr->shivf_len + sa_ptr->shsnf_len + sa_ptr->shplf_len + sa_ptr->stmacf_len; - new_enc_frame_header_field_length = (*p_enc_frame_len) - 1; - break; - case SA_ENCRYPTION: - // Ingest length + 1 (accounts for -1 to length) + spi_index (2) + shivf_len (varies) + shsnf_len (varies) - // + shplf_len + arsn_len + pad_size - *p_enc_frame_len = temp_tc_header.fl + 1 + 2 + sa_ptr->shivf_len + sa_ptr->shsnf_len + sa_ptr->shplf_len; - //+ sa_ptr->arsn_len; //should point to shplf_len - - new_enc_frame_header_field_length = (*p_enc_frame_len) - 1; + // Calculate frame lengths based on SA fields + *p_enc_frame_len = temp_tc_header.fl + 1 + 2 + sa_ptr->shivf_len + sa_ptr->shsnf_len + sa_ptr->shplf_len + sa_ptr->stmacf_len; + new_enc_frame_header_field_length = (*p_enc_frame_len) - 1; + + if (sa_service_type == SA_ENCRYPTION) + { // Handle Padding, if necessary if(*(sa_ptr->ecs) == CRYPTO_CIPHER_AES256_CBC) { @@ -334,17 +319,11 @@ int32_t Crypto_TC_ApplySecurity_Cam(const uint8_t* p_in_frame, const uint16_t in } } - break; - case SA_AUTHENTICATED_ENCRYPTION: - // Ingest length + spi_index (2) + shivf_len (varies) + shsnf_len (varies) - // + shplf_len + arsn_len + pad_size + stmacf_len - *p_enc_frame_len = temp_tc_header.fl + 1 + 2 + sa_ptr->shivf_len + sa_ptr->shsnf_len + sa_ptr->shplf_len + - sa_ptr->arsn_len + sa_ptr->stmacf_len; - new_enc_frame_header_field_length = (*p_enc_frame_len) - 1; - break; - default: + } + + if (sa_service_type != (SA_PLAINTEXT || SA_AUTHENTICATED_ENCRYPTION || SA_ENCRYPTION || SA_AUTHENTICATION)) + { printf(KRED "Unknown SA Service Type Detected!" RESET); - break; } // Ensure the frame to be created will not violate managed parameter maximum length