From 7813f5404f5b9beaca94fff7a679d5be2a555180 Mon Sep 17 00:00:00 2001 From: Ibraheem Saleh Date: Tue, 10 May 2022 13:42:30 -0700 Subject: [PATCH 1/2] AMMOSGH-91: Add sanity frame length checks against provided input frames --- include/crypto_error.h | 2 + src/src_main/crypto_tc.c | 24 ++++++++++++ util/src_util/ut_tc_apply.c | 68 ++++++++++++++++++++++++++++++++++ util/src_util/ut_tc_process.c | 70 +++++++++++++++++++++++++++++++++++ 4 files changed, 164 insertions(+) diff --git a/include/crypto_error.h b/include/crypto_error.h index 76a46e20..5f9b582a 100644 --- a/include/crypto_error.h +++ b/include/crypto_error.h @@ -90,5 +90,7 @@ #define CRYPTO_LIB_ERR_IV_LEN_SHORTER_THAN_SEC_HEADER_LENGTH (-37) #define CRYPTO_LIB_ERR_ARSN_LEN_SHORTER_THAN_SEC_HEADER_LENGTH (-38) #define CRYPTO_LIB_ERR_FRAME_COUNTER_DOESNT_MATCH_SA (-39) +#define CRYPTO_LIB_ERR_INPUT_FRAME_TOO_SHORT_FOR_TC_STANDARD (-40) +#define CRYPTO_LIB_ERR_INPUT_FRAME_LENGTH_SHORTER_THAN_FRAME_HEADERS_LENGTH (-41) #endif //_crypto_error_h_ diff --git a/src/src_main/crypto_tc.c b/src/src_main/crypto_tc.c index 59e60496..368a6619 100644 --- a/src/src_main/crypto_tc.c +++ b/src/src_main/crypto_tc.c @@ -86,6 +86,12 @@ int32_t Crypto_TC_ApplySecurity(const uint8_t* p_in_frame, const uint16_t in_fra return status; // return immediately so a NULL crypto_config is not dereferenced later } + if (in_frame_length < 5) // Frame length doesn't have enough bytes for TC TF header -- error out. + { + status = CRYPTO_LIB_ERR_INPUT_FRAME_TOO_SHORT_FOR_TC_STANDARD; + return status; + } + // Primary Header temp_tc_header.tfvn = ((uint8_t)p_in_frame[0] & 0xC0) >> 6; temp_tc_header.bypass = ((uint8_t)p_in_frame[0] & 0x20) >> 5; @@ -98,6 +104,12 @@ int32_t Crypto_TC_ApplySecurity(const uint8_t* p_in_frame, const uint16_t in_fra temp_tc_header.fl = temp_tc_header.fl | (uint8_t)p_in_frame[3]; temp_tc_header.fsn = (uint8_t)p_in_frame[4]; + if (in_frame_length < temp_tc_header.fl) // Specified frame length larger than provided frame! + { + status = CRYPTO_LIB_ERR_INPUT_FRAME_LENGTH_SHORTER_THAN_FRAME_HEADERS_LENGTH; + return status; + } + // Lookup-retrieve managed parameters for frame via gvcid: status = Crypto_Get_Managed_Parameters_For_Gvcid(temp_tc_header.tfvn, temp_tc_header.scid, temp_tc_header.vcid, gvcid_managed_parameters, ¤t_managed_parameters); @@ -659,6 +671,12 @@ int32_t Crypto_TC_ProcessSecurity(uint8_t* ingest, int *len_ingest, TC_t* tc_sdl printf(KYEL "\n----- Crypto_TC_ProcessSecurity START -----\n" RESET); #endif + if (*len_ingest < 5) // Frame length doesn't even have enough bytes for header -- error out. + { + status = CRYPTO_LIB_ERR_INPUT_FRAME_TOO_SHORT_FOR_TC_STANDARD; + return status; + } + int byte_idx = 0; // Primary Header tc_sdls_processed_frame->tc_header.tfvn = ((uint8_t)ingest[byte_idx] & 0xC0) >> 6; @@ -677,6 +695,12 @@ int32_t Crypto_TC_ProcessSecurity(uint8_t* ingest, int *len_ingest, TC_t* tc_sdl tc_sdls_processed_frame->tc_header.fsn = (uint8_t)ingest[byte_idx]; byte_idx++; + if (*len_ingest < tc_sdls_processed_frame->tc_header.fl) // Specified frame length larger than provided frame! + { + status = CRYPTO_LIB_ERR_INPUT_FRAME_LENGTH_SHORTER_THAN_FRAME_HEADERS_LENGTH; + return status; + } + // Lookup-retrieve managed parameters for frame via gvcid: status = Crypto_Get_Managed_Parameters_For_Gvcid( tc_sdls_processed_frame->tc_header.tfvn, tc_sdls_processed_frame->tc_header.scid, diff --git a/util/src_util/ut_tc_apply.c b/util/src_util/ut_tc_apply.c index 62363ad8..370a079b 100644 --- a/util/src_util/ut_tc_apply.c +++ b/util/src_util/ut_tc_apply.c @@ -520,4 +520,72 @@ UTEST(TC_APPLY_SECURITY, INVALID_FRAME_SIZE) ASSERT_EQ(CRYPTO_LIB_ERR_TC_FRAME_SIZE_EXCEEDS_SPEC_LIMIT, status); } +UTEST(TC_APPLY_SECURITY, ERROR_TC_INPUT_FRAME_TOO_SHORT_FOR_SPEC) +{ + int32_t status = CRYPTO_LIB_SUCCESS; + uint8_t* ptr_enc_frame = NULL; + uint16_t enc_frame_len = 0; + // Setup & Initialize CryptoLib + Crypto_Config_CryptoLib(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_TRUE, TC_UNIQUE_SA_PER_MAP_ID_FALSE, + TC_CHECK_FECF_TRUE, 0x3F, SA_INCREMENT_NONTRANSMITTED_IV_TRUE); + Crypto_Config_Add_Gvcid_Managed_Parameter(0, 0x0003, 0, TC_HAS_FECF, TC_HAS_SEGMENT_HDRS, 4); + status = Crypto_Init(); + ASSERT_EQ(CRYPTO_LIB_SUCCESS, status); + + + char* test_frame_pt_h = "2003001c"; + uint8_t *test_frame_pt_b = NULL; + int test_frame_pt_len = 0; + + // Expose/setup SAs for testing + SecurityAssociation_t* test_association = NULL; + test_association = malloc(sizeof(SecurityAssociation_t) * sizeof(uint8_t)); + sadb_routine->sadb_get_sa_from_spi(1, &test_association); + test_association->arsn_len = 0; + test_association->shsnf_len = 0; + + // Convert input test frame + hex_conversion(test_frame_pt_h, (char**) &test_frame_pt_b, &test_frame_pt_len); + // Should fail, as frame length violates the managed parameter + status = Crypto_TC_ApplySecurity(test_frame_pt_b, test_frame_pt_len, &ptr_enc_frame, &enc_frame_len); + ASSERT_EQ(CRYPTO_LIB_ERR_INPUT_FRAME_TOO_SHORT_FOR_TC_STANDARD, status); + + Crypto_Shutdown(); +} + +UTEST(TC_APPLY_SECURITY, ERROR_TC_INPUT_FRAME_TOO_SHORT_FOR_SPECIFIED_FRAME_LENGTH_HEADER) +{ + int32_t status = CRYPTO_LIB_SUCCESS; + uint8_t* ptr_enc_frame = NULL; + uint16_t enc_frame_len = 0; + // Setup & Initialize CryptoLib + Crypto_Config_CryptoLib(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_TRUE, TC_UNIQUE_SA_PER_MAP_ID_FALSE, + TC_CHECK_FECF_TRUE, 0x3F, SA_INCREMENT_NONTRANSMITTED_IV_TRUE); + Crypto_Config_Add_Gvcid_Managed_Parameter(0, 0x0003, 0, TC_HAS_FECF, TC_HAS_SEGMENT_HDRS, 4); + status = Crypto_Init(); + ASSERT_EQ(CRYPTO_LIB_SUCCESS, status); + + + char* test_frame_pt_h = "2003001c00000002ff"; + uint8_t *test_frame_pt_b = NULL; + int test_frame_pt_len = 0; + + // Expose/setup SAs for testing + SecurityAssociation_t* test_association = NULL; + test_association = malloc(sizeof(SecurityAssociation_t) * sizeof(uint8_t)); + sadb_routine->sadb_get_sa_from_spi(1, &test_association); + test_association->arsn_len = 0; + test_association->shsnf_len = 0; + + // Convert input test frame + hex_conversion(test_frame_pt_h, (char**) &test_frame_pt_b, &test_frame_pt_len); + // Should fail, as frame length violates the managed parameter + status = Crypto_TC_ApplySecurity(test_frame_pt_b, test_frame_pt_len, &ptr_enc_frame, &enc_frame_len); + ASSERT_EQ(CRYPTO_LIB_ERR_INPUT_FRAME_LENGTH_SHORTER_THAN_FRAME_HEADERS_LENGTH, status); + + Crypto_Shutdown(); +} + UTEST_MAIN(); diff --git a/util/src_util/ut_tc_process.c b/util/src_util/ut_tc_process.c index d2f92b5b..f531a8cf 100644 --- a/util/src_util/ut_tc_process.c +++ b/util/src_util/ut_tc_process.c @@ -517,5 +517,75 @@ UTEST(TC_PROCESS, HAPPY_PATH_PROCESS_NONTRANSMITTED_INCREMENTING_ARSN_ROLLOVER) // sadb_routine->sadb_close(); } +UTEST(TC_PROCESS, ERROR_TC_INPUT_FRAME_TOO_SHORT_FOR_SPEC) +{ + int32_t status = CRYPTO_LIB_SUCCESS; + // Setup & Initialize CryptoLib + Crypto_Config_CryptoLib(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_TRUE, TC_UNIQUE_SA_PER_MAP_ID_FALSE, + TC_CHECK_FECF_TRUE, 0x3F, SA_INCREMENT_NONTRANSMITTED_IV_TRUE); + Crypto_Config_Add_Gvcid_Managed_Parameter(0, 0x0003, 0, TC_HAS_FECF, TC_HAS_SEGMENT_HDRS, 4); + status = Crypto_Init(); + ASSERT_EQ(CRYPTO_LIB_SUCCESS, status); + + TC_t* tc_sdls_processed_frame; + tc_sdls_processed_frame = malloc(sizeof(uint8_t) * TC_SIZE); + memset(tc_sdls_processed_frame, 0, (sizeof(uint8_t) * TC_SIZE)); + + + char* test_frame_pt_h = "2003001c"; + uint8_t *test_frame_pt_b = NULL; + int test_frame_pt_len = 0; + + // Expose/setup SAs for testing + SecurityAssociation_t* test_association = NULL; + test_association = malloc(sizeof(SecurityAssociation_t) * sizeof(uint8_t)); + sadb_routine->sadb_get_sa_from_spi(1, &test_association); + test_association->arsn_len = 0; + test_association->shsnf_len = 0; + + // Convert input test frame + hex_conversion(test_frame_pt_h, (char**) &test_frame_pt_b, &test_frame_pt_len); + // Should fail, as frame length violates the managed parameter + status = Crypto_TC_ProcessSecurity(test_frame_pt_b, &test_frame_pt_len, tc_sdls_processed_frame); + ASSERT_EQ(CRYPTO_LIB_ERR_INPUT_FRAME_TOO_SHORT_FOR_TC_STANDARD, status); + + Crypto_Shutdown(); +} + +UTEST(TC_PROCESS, ERROR_TC_INPUT_FRAME_TOO_SHORT_FOR_SPECIFIED_FRAME_LENGTH_HEADER) +{ + int32_t status = CRYPTO_LIB_SUCCESS; + // Setup & Initialize CryptoLib + Crypto_Config_CryptoLib(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_TRUE, TC_UNIQUE_SA_PER_MAP_ID_FALSE, + TC_CHECK_FECF_TRUE, 0x3F, SA_INCREMENT_NONTRANSMITTED_IV_TRUE); + Crypto_Config_Add_Gvcid_Managed_Parameter(0, 0x0003, 0, TC_HAS_FECF, TC_HAS_SEGMENT_HDRS, 4); + status = Crypto_Init(); + ASSERT_EQ(CRYPTO_LIB_SUCCESS, status); + + TC_t* tc_sdls_processed_frame; + tc_sdls_processed_frame = malloc(sizeof(uint8_t) * TC_SIZE); + memset(tc_sdls_processed_frame, 0, (sizeof(uint8_t) * TC_SIZE)); + + char* test_frame_pt_h = "2003001c00000002ff"; + uint8_t *test_frame_pt_b = NULL; + int test_frame_pt_len = 0; + + // Expose/setup SAs for testing + SecurityAssociation_t* test_association = NULL; + test_association = malloc(sizeof(SecurityAssociation_t) * sizeof(uint8_t)); + sadb_routine->sadb_get_sa_from_spi(1, &test_association); + test_association->arsn_len = 0; + test_association->shsnf_len = 0; + + // Convert input test frame + hex_conversion(test_frame_pt_h, (char**) &test_frame_pt_b, &test_frame_pt_len); + // Should fail, as frame length violates the managed parameter + status = Crypto_TC_ProcessSecurity(test_frame_pt_b, &test_frame_pt_len, tc_sdls_processed_frame); + ASSERT_EQ(CRYPTO_LIB_ERR_INPUT_FRAME_LENGTH_SHORTER_THAN_FRAME_HEADERS_LENGTH, status); + + Crypto_Shutdown(); +} UTEST_MAIN(); From 46eb0f8dd7125f55571fc28b4a044b5e80eb6a56 Mon Sep 17 00:00:00 2001 From: Ibraheem Saleh Date: Tue, 10 May 2022 14:10:26 -0700 Subject: [PATCH 2/2] AMMOSGH-91: Fix length check --- src/src_main/crypto_tc.c | 4 ++-- util/src_util/ut_tc_process.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/src_main/crypto_tc.c b/src/src_main/crypto_tc.c index 368a6619..07349f04 100644 --- a/src/src_main/crypto_tc.c +++ b/src/src_main/crypto_tc.c @@ -104,7 +104,7 @@ int32_t Crypto_TC_ApplySecurity(const uint8_t* p_in_frame, const uint16_t in_fra temp_tc_header.fl = temp_tc_header.fl | (uint8_t)p_in_frame[3]; temp_tc_header.fsn = (uint8_t)p_in_frame[4]; - if (in_frame_length < temp_tc_header.fl) // Specified frame length larger than provided frame! + if (in_frame_length < temp_tc_header.fl+1) // Specified frame length larger than provided frame! { status = CRYPTO_LIB_ERR_INPUT_FRAME_LENGTH_SHORTER_THAN_FRAME_HEADERS_LENGTH; return status; @@ -695,7 +695,7 @@ int32_t Crypto_TC_ProcessSecurity(uint8_t* ingest, int *len_ingest, TC_t* tc_sdl tc_sdls_processed_frame->tc_header.fsn = (uint8_t)ingest[byte_idx]; byte_idx++; - if (*len_ingest < tc_sdls_processed_frame->tc_header.fl) // Specified frame length larger than provided frame! + if (*len_ingest < tc_sdls_processed_frame->tc_header.fl + 1) // Specified frame length larger than provided frame! { status = CRYPTO_LIB_ERR_INPUT_FRAME_LENGTH_SHORTER_THAN_FRAME_HEADERS_LENGTH; return status; diff --git a/util/src_util/ut_tc_process.c b/util/src_util/ut_tc_process.c index f531a8cf..fba4e541 100644 --- a/util/src_util/ut_tc_process.c +++ b/util/src_util/ut_tc_process.c @@ -568,7 +568,7 @@ UTEST(TC_PROCESS, ERROR_TC_INPUT_FRAME_TOO_SHORT_FOR_SPECIFIED_FRAME_LENGTH_HEAD tc_sdls_processed_frame = malloc(sizeof(uint8_t) * TC_SIZE); memset(tc_sdls_processed_frame, 0, (sizeof(uint8_t) * TC_SIZE)); - char* test_frame_pt_h = "2003001c00000002ff"; + char* test_frame_pt_h = "200304260000020000000000000000000000309e09deeaa375487983a89f3ed7519a230baf22"; uint8_t *test_frame_pt_b = NULL; int test_frame_pt_len = 0;