Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMAC Encryption/Decryption Added #52

Merged
merged 8 commits into from
Jan 10, 2022
Merged

CMAC Encryption/Decryption Added #52

merged 8 commits into from
Jan 10, 2022

Conversation

dccutrig
Copy link
Contributor

@dccutrig dccutrig commented Jan 7, 2022

  • Added support CMAC enc/dec
  • Added UTs for above
  • Changed cipher/authentication suites to enums
  • Some changes to SA Struct for pointers (ecs, abm)

@codecov-commenter
Copy link

Codecov Report

Merging #52 (aaf47de) into collab_main (89ceed0) will increase coverage by 1.19%.
The diff coverage is 83.29%.

Impacted file tree graph

@@               Coverage Diff               @@
##           collab_main      #52      +/-   ##
===============================================
+ Coverage        60.76%   61.95%   +1.19%     
===============================================
  Files               20       22       +2     
  Lines             3362     3746     +384     
===============================================
+ Hits              2043     2321     +278     
- Misses            1319     1425     +106     
Impacted Files Coverage Δ
src/src_main/crypto_print.c 0.00% <0.00%> (ø)
src/src_main/sadb_routine_inmemory.template.c 45.77% <38.46%> (-0.83%) ⬇️
...gcrypt/cryptography_interface_libgcrypt.template.c 85.22% <50.53%> (-4.92%) ⬇️
util/src_util/et_dt_validation.c 98.22% <95.91%> (+0.76%) ⬆️
src/src_main/crypto.c 46.37% <100.00%> (+0.21%) ⬆️
src/src_main/crypto_tc.c 88.74% <100.00%> (+4.65%) ⬆️
util/src_util/ut_tc_apply.c 100.00% <100.00%> (ø)
util/include/utest.h 12.98% <0.00%> (-31.18%) ⬇️
src/src_main/crypto_config.c 72.88% <0.00%> (-2.35%) ⬇️
util/src_util/ut_mysql_tls_connection.c 0.00% <0.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89ceed0...aaf47de. Read the comment docs.

@@ -213,8 +220,9 @@ int32_t Crypto_TC_ApplySecurity(const uint8_t *p_in_frame, const uint16_t in_fra
case SA_AUTHENTICATION:
// Ingest length + spi_index (2) + shivf_len (varies) + shsnf_len (varies)
// + shplf_len + arc_len + pad_size + stmacf_len
// TODO: If ARC is transmitted in the SHSNF field (as in CMAC... don't double count those bytes)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should make sure we create an issue for the couple of TODOs you've already identified

}

// Need to copy the data over, since authentication won't change/move the data directly
memcpy(data_out, data_in, len_data_in);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check on the data_out ptr to ensure it's not NULL prior to use?

}

// Need to copy the data over, since authentication won't change/move the data directly
memcpy(data_out, data_in, len_data_in);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

@@ -68,8 +68,8 @@ typedef struct
uint8_t shsnf_len : 6; // Sec. Header SN Field Length
uint8_t shplf_len : 2; // Sec. Header PL Field Length
uint8_t stmacf_len : 8; // Sec. Trailer MAC Field Length
uint8_t *ecs; // Encryption Cipher Suite (algorithm / mode ID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize pointers to NULL when applicable

Copy link
Contributor

@szemerick szemerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed changes. built and ran tests on local CentOS 8

Copy link
Contributor

@IbraheemYSaleh IbraheemYSaleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍 ... The only thing weird is the way we work with the ECS field as a bit-field, but I get that that's because it's also used in some of the EP PDU handling. This PR is ready to merge by my account.

@dccutrig dccutrig merged commit c274bbc into collab_main Jan 10, 2022
@rjbrown2 rjbrown2 deleted the cmac_python branch January 11, 2022 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants