Crypto_TC_ProcessSecurity with Anti-Replay Checking can not handle edge case where IV Rollover Happens

[IbraheemYSaleh]

For example, if we have the condition as follows:

Received IV (in Frame) is
000000000000000000000000
SA IV is
000000000000ffffffffffff
ARSNW is: 5
Crypto_Window return status is: -1
/home/isaleh/git/KMC/CryptoLib-IbraheemYSaleh/util/src_util/ut_tc_process.c:307: Failure
Expected : 0
Actual : -23

Since 000000000000ffffffffffff should roll-over to 000000000000000000000000 (or 000000000001000000000000 if incrementing the static portion), this Crypto Window status failure shouldn't happen.

To reproduce, see TC_PROCESS.HAPPY_PATH_PROCESS_STATIC_IV_ROLLOVER unit test case. (See the commented out checks!)

[IbraheemYSaleh]

https://github.jpl.nasa.gov/ASEC/AMMOS-CryptoLib/issues/57

[IbraheemYSaleh]

The code required to handle the incrementing non-transmitted IV portion is horrendous. The code required to handle non-incrementing non-transmitted IVs isn't so bad.

Here is a rough diff of a terrible solution to the non-transmitted IV portion:

--- a/src/src_main/crypto_tc.c
+++ b/src/src_main/crypto_tc.c
@@ -875,6 +875,8 @@ int32_t Crypto_TC_ProcessSecurity(uint8_t* ingest, int *len_ingest, TC_t* tc_sdl
     printf(KYEL "TC PDU Calculated Length: %d \n" RESET, tc_sdls_processed_frame->tc_pdu_len);
 #endif

+    uint8_t static_iv_rollover_flag = CRYPTO_FALSE;
+
     if(sa_service_type != SA_PLAINTEXT && ecs_is_aead_algorithm == CRYPTO_TRUE)
     {
         status = cryptography_if->cryptography_aead_decrypt(tc_sdls_processed_frame->tc_pdu,       // plaintext output
@@ -897,6 +899,48 @@ int32_t Crypto_TC_ProcessSecurity(uint8_t* ingest, int *len_ingest, TC_t* tc_sdl
                                                             sa_ptr->acs  // authentication cipher

         );
+        if(status == CRYPTO_LIB_ERR_MAC_VALIDATION_ERROR && crypto_config->crypto_increment_nontransmitted_iv == SA_INCREMENT_NONTRANSMITTED_IV_TRUE)
+        {
+            static_iv_rollover_flag = CRYPTO_TRUE;
+            Crypto_increment(sa_ptr->iv, sa_ptr->iv_len);
+            // Retrieve non-transmitted portion of IV from SA (if applicable)
+            memcpy(tc_sdls_processed_frame->tc_sec_header.iv, sa_ptr->iv, sa_ptr->iv_len-sa_ptr->shivf_len);
+
+            uint8_t* temp_inc_frame_iv = malloc(sa_ptr->iv_len);
+            memcpy(temp_inc_frame_iv,tc_sdls_processed_frame->tc_sec_header.iv,sa_ptr->iv_len);
+            Crypto_increment(temp_inc_frame_iv,sa_ptr->iv_len);
+
+            status = Crypto_Check_Anti_Replay(sa_ptr, tc_sdls_processed_frame->tc_sec_header.sn, temp_inc_frame_iv);
+            if(status != CRYPTO_LIB_SUCCESS)
+            {
+                return status;
+            }
+
+            status = cryptography_if->cryptography_aead_decrypt(tc_sdls_processed_frame->tc_pdu,       // plaintext output
+                                                                (size_t)(tc_sdls_processed_frame->tc_pdu_len),   // length of data
+                                                                &(ingest[tc_enc_payload_start_index]), // ciphertext input
+                                                                (size_t)(tc_sdls_processed_frame->tc_pdu_len),    // in data length
+                                                                NULL, // Key
+                                                                Crypto_Get_ECS_Algo_Keylen(*sa_ptr->ecs),
+                                                                sa_ptr, // SA for key reference
+                                                                tc_sdls_processed_frame->tc_sec_header.iv, // IV
+                                                                sa_ptr->iv_len, // IV Length
+                                                                tc_sdls_processed_frame->tc_sec_trailer.mac, // Frame Expected Tag
+                                                                sa_ptr->stmacf_len,                           // tag size
+                                                                aad,    // additional authenticated data
+                                                                aad_len, // length of AAD
+                                                                (sa_ptr->est), // Decryption Bool
+                                                                (sa_ptr->ast), // Authentication Bool
+                                                                (sa_ptr->ast), // AAD Bool
+                                                                sa_ptr->ecs, // encryption cipher
+                                                                sa_ptr->acs  // authentication cipher
+
+            );
+            if(status == CRYPTO_LIB_SUCCESS)
+            {
+                memcpy(sa_ptr->iv + (sa_ptr->iv_len - sa_ptr->shivf_len), tc_sdls_processed_frame->tc_sec_header.iv + (sa_ptr->iv_len - sa_ptr->shivf_len), sa_ptr->shivf_len);
+            }
+        }
     }else if (sa_service_type != SA_PLAINTEXT && ecs_is_aead_algorithm == CRYPTO_FALSE) // Non aead algorithm
     {
         // TODO - implement non-AEAD algorithm logic
@@ -938,7 +982,8 @@ int32_t Crypto_TC_ProcessSecurity(uint8_t* ingest, int *len_ingest, TC_t* tc_sdl
     // Now that MAC has been verified, check IV & ARSN if applicable
     if (crypto_config->ignore_anti_replay == TC_IGNORE_ANTI_REPLAY_FALSE && status == CRYPTO_LIB_SUCCESS)
     {
-        status = Crypto_Check_Anti_Replay(sa_ptr, tc_sdls_processed_frame->tc_sec_header.sn, tc_sdls_processed_frame->tc_sec_header.iv);
+        if(static_iv_rollover_flag != CRYPTO_TRUE)
+            status = Crypto_Check_Anti_Replay(sa_ptr, tc_sdls_processed_frame->tc_sec_header.sn, tc_sdls_processed_frame->tc_sec_header.iv);
     }

[IbraheemYSaleh]

Handled the non-incrementing non-transmitted portion rollover case in https://github.com/nasa/CryptoLib/pull/106/commits/cf322a8d65033a00452509728d58b20184cb2811

Given the amount of code to handle this tiny corner case, I do not recommend handling the incrementing non-transmitted portion rollover case. (see the incomplete atrocity above)

[IbraheemYSaleh]

Note, a similar issue may happen for ARSN rollover, but the mac validation check won't fail like it does for IV. It may be easier to fix the ARSN rollover case.

[IbraheemYSaleh]

So I slept on this... I think it may be a lot easier to solve this problem if we move the ARSNW check to before CryptoLib authenticates & decrypts the SDLS wrapped frame. That is probably the correct place to put it in the first place, and where we have it now is arguably wrong.

There are still a lot of subsequent edge cases to consider and get right though!

[IbraheemYSaleh]

I couldn't help myself: https://github.com/nasa/CryptoLib/pull/108

Seems clean enough to accept.