aes-gcm support #219
Closed
aes-gcm support #219
Conversation
This file contains hidden or 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
There were some small issues: a) it never called EVP_EncryptFinal_ex. For CTR mode, this had no visible consequences as EVP_EncryptUpdate already yielded all ciphertext. For cleanliness and to have correctness even in other modes, the missing call was added. b) decrypt = encrypt hack This is a nice hack to abbreviate, but it only works for modes without padding and without authentication. For cleanliness and to have correctness even in other modes, the missing usage of the decrypt api was added. c) outl == inl assumption Again, True for CTR mode, but not for padding or authenticating modes. Fixed so it computes the ciphertext / plaintext length based on api return values. Other changes: As encrypt and decrypt API calls are different even for initialization/reset, added a is_encrypt flag. Defensive output buffer allocation. Added the length of one extra AES block (16bytes) so it would work even with padding modes. 16bytes are needed because a full block of padding might get added when the plaintext was a multiple of aes block size. These changes are based on some experimental code I did for aes-cbc and aes-gcm. While we likely won't ever want aes-cbc in attic (maybe gcm though?), I think it is cleaner to not make too many mode specific assumptions and hacks, but just use the API as it was meant to be used.
https://www.openssl.org/docs/crypto/EVP_aes_256_cbc.html EVP_DecryptInit_ex(), EVP_DecryptUpdate() and EVP_DecryptFinal_ex() are the corresponding decryption operations. EVP_DecryptFinal() will return an error code if padding is enabled and the final block is not correctly formatted. The parameters and restrictions are identical to the encryption operations except that if padding is enabled the decrypted data buffer out passed to EVP_DecryptUpdate() should have sufficient room for (inl + cipher_block_size) bytes unless the cipher block size is 1 in which case inl bytes is sufficient. I doubt this is correct, but let's rather be defensive here.
This has special and extremely fast HW acceleration on e.g recent Intel CPUs: AES-NI and PCLMULQDQ. Notes: a) I had to kill AES.iv method, it just did not work for aes-gcm as done by openssl. As the incremented IV (counter) can't be read back, we have to keep and manually increment it in Key.enc_iv. b) there is a hack in AES.compute_tag_and_encrypt to add 16B of zero to the right of the gmac (which is also 16B) because the current callers expects 32B. AES.check_tag_and_encrypt is tolerant of such a 32B tag, but will only use the left 16B and ignore the right 16B if needed. this is a bit dirty, but I didn't want to change the header layout within this changeset. c) switched from mac&encrypt to encrypt-then-mac (using aes-gcm) for the keyfile 'data' entry d) also added a test that creates the testdata needed for the constants at top of testsuite/key.py e) I kept enc_hmac_key although it is not used by the code in this changeset. But we'll need to keep supporting the old algorithms, too.
Contributor
Author
|
opinions? suggestions? code review? |
Contributor
Author
|
closing this pull request, seems unwanted. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This is based on the crypto_cleanup code and not yet ready for pulling.
I wanted it listed in pull requests though to get some feedback from others.
Code review and practical testing would be very welcome.
note:
The code simply REPLACES the aes-ctr and hmac-sha256 code by aes-gcm and gmac, so the amount of change is easier to see.
Of course, if aes-gcm support is welcome, it would need to be ADDED and the old algorithms kept for backwards compatibility.