Skip to content
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

Merged

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Aug 26, 2022

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.

@AaronFeickert
Copy link
Collaborator Author

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.

hansieodendaal
hansieodendaal previously approved these changes Aug 28, 2022
Copy link
Contributor

@hansieodendaal hansieodendaal left a 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. -:)

base_layer/wallet/src/util/encryption.rs Outdated Show resolved Hide resolved
base_layer/wallet/src/util/encryption.rs Outdated Show resolved Hide resolved
@AaronFeickert
Copy link
Collaborator Author

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 drop was to make clippy happy. It complains otherwise.

@AaronFeickert AaronFeickert changed the title fix: replace AES-GCM with XChaCha20-Poly1305 WIP (do not merge): fix: replace AES-GCM with XChaCha20-Poly1305 Aug 28, 2022
@AaronFeickert AaronFeickert changed the title WIP (do not merge): fix: replace AES-GCM with XChaCha20-Poly1305 fix: replace AES-GCM with XChaCha20-Poly1305 Aug 28, 2022
@@ -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());
Copy link
Member

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.

Suggested change
std::mem::drop((*current_cipher).take());
*current_cipher = None;

@stringhandler stringhandler changed the title fix: replace AES-GCM with XChaCha20-Poly1305 fix!: replace AES-GCM with XChaCha20-Poly1305 Aug 29, 2022
@stringhandler stringhandler merged commit 85acc2f into tari-project:development Aug 29, 2022
@AaronFeickert AaronFeickert deleted the xchacha20-poly1305 branch August 29, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants