-
Notifications
You must be signed in to change notification settings - Fork 28
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
Tc process refactor #78
Conversation
…t of window ARSNs/IVs with error message
Failing tests noted - looking into it. |
Codecov Report
@@ Coverage Diff @@
## dev #78 +/- ##
======================================
Coverage ? 83.30%
======================================
Files ? 18
Lines ? 4116
Branches ? 0
======================================
Hits ? 3429
Misses ? 687
Partials ? 0 Continue to review full report at Codecov.
|
// Now that MAC has been verified, check IV & ARSN if applicable | ||
if (crypto_config->ignore_anti_replay == TC_IGNORE_ANTI_REPLAY_FALSE) | ||
{ | ||
status = Crypto_Check_Anti_Replay(sa_ptr, arsn, iv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The anti-replay counter check is not specific to a cryptography interface... This check belongs in crypto_tc.c, not in the libgcrypt/kmc crypto template files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is there some reason that I'm missing about why this needs to live here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, looking into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I recall now - the reason I did this here is that we cannot validate the ARSN until we know the MAC (if present) is valid - which requires several libgcrypt calls to open/key/validate. Rather than having half of the ARSN checks in crypto_tc.c, and the other half in the decryption crypto_ifs, I opted to place them all the same way. But, I'm open to discussion and more thoughts on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... I'm still a bit unclear about your reasoning... The cryptography interface process calls (cryptography_if->cryptography_aead_decrypt, cryptography_if->cryptography_validate_authentication) do validate the MAC as part of those functions -- can't Anti-Replay checking just happen after those calls to achieve what you're describing? My primary reasoning for why it belongs in crypto_tc.c instead of the cryptography interface is that this logic will need to be duplicated in both crypto interfaces if it lives at that level, whereas it'll only exist once in crypto_tc.c if it lives there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tracking better what you're saying now.... so it's fine if we decrypt the text, and then just discard it if the ARSN/IV turned out be be invalid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think so... for AEAD algorithms, we don't have a choice... for non-AEAD algorithms, we can still have the logic between the authenticate and decrypt calls if we wanted.
…t of window ARSNs/IVs with error message
…vice MAC validation bug
Added the MariaDB changes for the acs update... This branch was based off of collab_dev instead of dev, and behind by a number of commits which are conflicting... I did a merge which is now failing. |
@dccutrig the ut_tc_process test suite is failing on the EXERCISE_ARSN test because the MAC validation step fails before it gets to the ARSN window validation checks ... This is because of this bug fix which now properly returns after a MAC validation failure: The test data will need to be modified with valid MACs to get past the current part of the process function which is failing and properly test the ARSN windows before this will pass. We should delete the collab_dev branch so that people don't accidentally branch off of that in the future (we should be branching off of dev) |
@IbraheemYSaleh Will work on updating the unit tests. I think I mistakenly picked the wrong base branch originally, but if I'm interpreting my log history correctly this was originally based from dev. Agreed we can nuke collab_dev and collab_main if you're ready to do so. |
UTs work out well for ARSN IV, looking for any feedback or comments