Avoid extra KDF call in Secure Cell #496
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently calling themis_auth_sym_encrypt_message() results in an unnecessary call to themis_sym_kdf() when we only want to validate input parameters and maybe return the correct output buffer length. Move KDF calls to the helper function and leave only parameter checks in the main one. While we're here, refactor the function to improve code readability and clarity.
themis_auth_sym_decrypt_message() has the same inefficient use of themis_sym_kdf() as in themis_auth_sym_encrypt_message(). Update and refactor it in a similar way. Complexity here stems from the fact that decryption path contains a workaround for a compatibility issue with Themis 0.9.6. We need to preserve the workaround. In order to make it more readable, move the compatibility code into a separate function that will be compiled out entirely when not needed.
Secure Cell format does not allow messages longer than 4 GB. Container length is limited by the length field in authentication context. This was the source of the original compatibility issue in Themis 0.9.6. However, currently we accept lengths over 4 GB, but silently truncate them when constructing authetication context. This is not right and we should check for the overflow instead of silently returning incorrect data. This code path is not tested because it's not very reliable to allocate over 4 GB of memory in CI environment.
Currently we leave the resulting derived keys on the stack after use. These keys are sensitive data so we should wipe them immediately after they have been used and are no longer needed. Move the derived key array into the parent function and wipe it there to ensure that we always do so. There are alternatives, but they will require to structure the code a bit differently or stop using these THEMIS_CHECK macros.
vixentael
approved these changes
Jul 11, 2019
| encrypted_message, | ||
| encrypted_message_length); | ||
|
|
||
| soter_wipe(derived_key, sizeof(derived_key)); |
| } | ||
|
|
||
| #ifdef SCELL_COMPAT | ||
| themis_status_t themis_auth_sym_decrypt_message_compat(const uint8_t* key, |
There was a problem hiding this comment.
great idea to extract this as separate function
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently calling
themis_auth_sym_encrypt_message()results in an unnecessary call tothemis_sym_kdf()when we only want to validate input parameters and maybe return the correct output buffer length.Move KDF calls to the helper function and leave only parameter checks in the main one. While we're here, refactor the function to improve code readability and clarity.
themis_auth_sym_decrypt_message()has the same inefficient use ofthemis_sym_kdf()as inthemis_auth_sym_encrypt_message(). Update and refactor it in a similar way.Additional complexity here stems from the fact that decryption path contains a workaround for a compatibility issue with Themis 0.9.6. We need to preserve the workaround. In order to make it more readable, move the compatibility code into a separate function that will be compiled out entirely when not needed.
Secure Cell format does not allow messages longer than 4 GB. Container length is limited by the length field in authentication context. This was the source of the original compatibility issue in Themis 0.9.6.
However, currently we accept lengths over 4 GB, but silently truncate them when constructing authentication context. This is not right and we should check for the overflow instead of silently returning incorrect data.
This code path is not tested because it's not very reliable to allocate over 4 GB of memory in CI environment.
Currently we leave the resulting derived keys on the stack after use. These keys are sensitive data so we should wipe them immediately after they have been used and are no longer needed.
Move the derived key array into the parent function and wipe it there to ensure that we always do so. There are alternatives, but they will require to structure the code a bit differently or stop using these THEMIS_CHECK macros.