-
Notifications
You must be signed in to change notification settings - Fork 576
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
Replace AES-CBC with AES-GCM and HMAC-SHA-1 with HMAC-SHA-256. #420
Replace AES-CBC with AES-GCM and HMAC-SHA-1 with HMAC-SHA-256. #420
Conversation
Wow! 👏 👏 👏 This is one of the best bug reports and pull requests I have ever received. Thank you for all the effort you have put into it.
Plug master requires Elixir v1.2 which is OTP18+ so we are good on that front. I think switching algorithms can be dangerous because of nodes using different OpenSSL being suddenly incompatible so I would go with the best algorithm for now and see how it will work in the long term.
Your choice as I am not familiar enough to decide so.
No.
I will study the code and let you know. :) |
defp decode_token(token) do | ||
case String.split(token, ".", parts: 5) do | ||
[protected, encrypted_key, iv, cipher_text, cipher_tag] -> | ||
with {:ok, protected} <- Base.url_decode64(protected, padding: false), |
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.
Can we leave a TODO here so we use with/else
once we depend on Elixir v1.3 only?
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.
Will do.
@potatosalad I have reviewed your changes and I have no further comment. Let me know when this is ready to merge from your side, thank you for the amazing work! |
@josevalim I've added the TODO comments about the |
@josevalim I lied, I forgot to include a comment in |
❤️ 💚 💙 💛 💜 |
…r-plug#420) * Replace AES-CBC with AES-GCM and HMAC-SHA-1 with HMAC-SHA-256. * Remove reporting on failure for AES-CBC mode from tests. * Add TODO comments about with/else statements in MessageEncryptor and MessageVerifier. * Add TODO about switching to verify/3 after backwards compatibility period.
First, this pull request may not be ready to merge quite yet. I wanted to get some feedback as to whether the direction I took is an acceptable one and if so, whether there needs to be any modifications prior to merging.
I'd also like to apologize for the length of my writing here, I wanted to make sure I had done my homework before submitting these recommendations. I have tried to summarize everything towards the beginning for those experiencing reading fatigue.
Summary of changes
Note: Whenever I say "deprecate" below, I really mean "soft deprecate" or "begin the transition away from" rather than an immediate hard deprecation.
Deprecate AES256-CBC with HMAC SHA-1 in favor of AES128-GCM with AES256-GCM Key Wrapping in
Plug.Crypto.MessageEncryptor
.Deprecate HMAC SHA-1 in favor of HMAC SHA-256 in
Plug.Crypto.MessageVerifier
.Maintain backwards compatibility in
Plug.Crypto.MessageEncryptor
andPlug.Crypto.MessageVerifier
so existing session cookies can be verified and decrypted, but new writes should use the new recommendations.Provide better methods of upgrading algorithms in the future while maintaining backwards compatibility (for example, 2-3 years from now, the recommended standards might be AES-GCM-SIV or the winning algorithm of the CAESAR competition for Authenticated Encryption and XMSS for Hash-Based Signatures).
This was done by using a modified form of JWE Compact Serialization and JWS Compact Serialization which just includes the algorithm name as the "header" in a list of Base64url without padding encoded terms. For example
BASE64URL("HS256") || '.' || BASE64URL("message") || '.' || BASE64URL(SIGNATURE)
is the basic format of a signature token using HMAC with SHA-256.This should make it easy to provide backwards compatibility support for older algorithms by examining the protected header prior to performing verification or decryption.
Deprecate
encrypt_and_sign/3,4
in favor ofencrypt/3
. Deprecateverify_and_decrypt/3,4
in favor ofdecrypt/3
.AES-GCM computes the authentication tag using the internal GHASH function. It's not really the same thing as a signature. Future encryption standards, like AES-GCM-SIV with its POLYVAL function, may compute additional tags/values which "authenticate" encrypted data before/after decryption, but behave differently from what we would normally consider a signature (verifying whether a particular message is from a specific sender).
Questions for discussion
Plug.Crypto.MessageVerifier.sign(message, sign_secret, :sha512)
. Are there any preferences for something other than HMAC SHA-256 as the default?Why are these changes necessary?
Short Explanation
AES256-CBC with HMAC SHA-1 uses an encryption key and signing key which are weakly linked. This allows a valid signing key and an invalid encryption key to erroneously decrypt a message resulting in garbage data returned. This occurs when a compatible initialization vector is randomly generated which returns a correctly padded plain text upon decryption with the wrong key, causing the tests to fail about 1 out of 120 runs.
Beyond that, AES-CBC in general is being phased out in many other communities/products/services and replaced with things like AES-CCM, AES-GCM, and CHACHA20-POLY1305. SHA-1 is also no longer recommended for future cryptographic use.
I would like to recommend the following two changes:
Medium Explanation
While running the tests for Plug locally, I noticed the occasional failure for the tests related to
Plug.Crypto.MessageEncryptor
.After closer examination, I found that the failure occurs approximately 0.8% of the time (or roughly 1 out of 120 runs) based on the frequency I measured on my machine.
I wound up writing a property test with ExCheck to demonstrate the issue, but have not included it in this pull request due to compilation issues with triq.
(ignore this paragraph if you know how quick check works) It essentially runs the
for_all
block ~100 times with randomly generated values as specified by thegenerator
. Iffalse
is ever returned, it will attempt to shrink or reduce the generated values to the smallest form that still produces a failure and report on the shrunk values.Here is an example walkthrough of the failure:
The same example using AES128-GCM with AES256-GCM Key Wrapping does not have the same problems:
Long Explanation
The steps involved for AES256-CBC with HMAC SHA-1 encryption:
message
)secret
)sign_secret
)"--"
(CTwithIV)."##"
(ENC).The steps involved for AES256-CBC with HMAC SHA-1 decryption:
encrypted
)secret
)sign_secret
)"##"
to get SI and TAG.:error
."--"
and Base64 decode to get CT and IV.Here is a reduced example of AES256-CBC with HMAC SHA-1 minus the PKCS-7 padding part to better demonstrate the problem.
The steps involved for AES128-GCM with AES256-GCM Key Wrapping encryption:
message
)secret
)sign_secret
)"A128GCM"
as AAD to get Cipher Text (CT) and Cipher Tag (TAG)."."
(ENC).The steps involved for AES128-GCM with AES256-GCM Key Wrapping decryption:
encrypted
)secret
)sign_secret
)"."
and Base64url decode without padding to get AAD, ENCKEY, IV, CT, and TAG.:error
.Here is a reduced example of AES128-GCM with AES256-GCM Key Wrapping.
References
Cisco: Next Generation Encryption
CloudFlare: Padding oracles and the decline of CBC-mode cipher suites
This deals more with the TLS standard of MAC-then-encrypt versus Plug's encrypt-then-MAC behavior, but it's an interesting read.
Cryptosense: Choice of Algorithms in the W3C Crypto API
chromium: Disable AES-256-CBC modes by default
The chromium team wound up deciding to keep AES-CBC around for a little longer, but the final comment on the issue illustrates that it will be eventually removed.
StackExchange: Practical disadvantages of GCM mode encryption
The question is actually about the downsides of AES-GCM, but the accepted answer has a few points as to why GCM is preferable over CBC.
COSE: CBOR Object Signing and Encryption
JWA: Key Encryption with AES GCM (defines A128GCMKW, A192GCMKW, and A256GCMKW standards)