-
Notifications
You must be signed in to change notification settings - Fork 219
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
fix!: replace AES-GCM with XChaCha20-Poly1305 #4550
fix!: replace AES-GCM with XChaCha20-Poly1305 #4550
Conversation
Note that existing applications of ChaCha20-Poly1305 using fixed nonces aren't affected by this. We certainly could move to XChaCha20-Poly1305 for consistency (and to avoid any accidental misuse of the code later) if desired, but it doesn't change security. |
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.
LGTM, however, the use of drop
is a bit weird. Just some small comments in one test.
Also, pub fn decrypt_bytes_integral_nonce(
makes a lot more sense now. -:)
Using |
@@ -535,7 +530,7 @@ impl WalletBackend for WalletSqliteDatabase { | |||
} | |||
|
|||
// Now that all the decryption has been completed we can safely remove the cipher fully | |||
let _ = (*current_cipher).take(); | |||
std::mem::drop((*current_cipher).take()); |
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.
nit: IMO this is the most idiomatic/simplest way to clear current_cipher
.
std::mem::drop((*current_cipher).take()); | |
*current_cipher = None; |
Description
This replaces AES-GCM, which is used for encrypted storage, with XChaCha20-Poly1305. It gets rid of a superfluous MAC. It updates tests to account for more failure modes.
This is an updated and cleaner version of PR 4529 (which used ChaCha20-Poly1305) that should be easier to review.
Motivation and Context
The work in PR 4340 binds field data into a MAC that is included with AES-GCM ciphertext. This binding is important and useful, but is done in the wrong place; AES-GCM already includes an authentication tag that is built in to the ciphertext and parsed automatically on decryption.
This work adds the field binding directly into the authenticated encryption as associated data, which is the standard way to include public context. It means the existing additional MAC is no longer needed, since malleated field data will fail the authentication phase of decryption.
Separately, the use of AES-GCM for authenticated encryption is suboptimal. Its nonce length is short enough that the use of random nonces can be risky if enough data is encrypted under a common key; if a nonce is ever reused, an attacker can use the resulting ciphertext to forge messages on arbitrary (even unused) nonces and the same key. While this seems unlikely to occur in the use cases of interest, there really isn't any good reason to use AES-GCM unless you have hardware support for it and speed is critical.
The XChaCha20-Poly1305 authenticated encryption construction uses longer nonces, and it is considered safe to generate them randomly. While this construction has not yet been standardized (an existing draft standard has expired), it is common. Its cryptographic sibling ChaCha20-Poly1305 uses the same nonce length as AES-GCM, and while nonce reuse is "somewhat less worse" than in AES-GCM, it would still be unsafe in this context if it occurred (forgeries are limited to existing nonces).
How Has This Been Tested?
Existing tests pass. New tests are included.