Skip to content

Commit

Permalink
fix!: replace AES-GCM with XChaCha20-Poly1305 (#4550)
Browse files Browse the repository at this point in the history
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](#4529) (which used ChaCha20-Poly1305) that should be easier to review.

Motivation and Context
---
The work in [PR 4340](#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.
  • Loading branch information
AaronFeickert authored Aug 29, 2022
1 parent 887df88 commit 85acc2f
Show file tree
Hide file tree
Showing 26 changed files with 285 additions and 245 deletions.
102 changes: 41 additions & 61 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion base_layer/wallet/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ tari_utilities = { git = "https://github.com/tari-project/tari_utilities.git", t
# Uncomment for normal use (non tokio-console tracing)
tokio = { version = "1.14", features = ["sync", "macros"] }

aes-gcm = "0.10.1"
async-trait = "0.1.50"
argon2 = "0.2"
bincode = "1.3.1"
Expand Down Expand Up @@ -56,6 +55,7 @@ thiserror = "1.0.26"
tower = "0.4"
prost = "0.9"
itertools = "0.10.3"
chacha20poly1305 = "0.10.1"

[dependencies.tari_core]
path = "../../base_layer/core"
Expand Down
4 changes: 2 additions & 2 deletions base_layer/wallet/src/key_manager_service/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

use std::sync::Arc;

use aes_gcm::Aes256Gcm;
use chacha20poly1305::XChaCha20Poly1305;
use tari_common_types::types::PrivateKey;
use tari_key_manager::cipher_seed::CipherSeed;
use tokio::sync::RwLock;
Expand Down Expand Up @@ -70,7 +70,7 @@ where TBackend: KeyManagerBackend + 'static
.await
}

async fn apply_encryption(&self, cipher: Aes256Gcm) -> Result<(), KeyManagerServiceError> {
async fn apply_encryption(&self, cipher: XChaCha20Poly1305) -> Result<(), KeyManagerServiceError> {
(*self.key_manager_inner).write().await.apply_encryption(cipher).await
}

Expand Down
4 changes: 2 additions & 2 deletions base_layer/wallet/src/key_manager_service/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

use aes_gcm::Aes256Gcm;
use chacha20poly1305::XChaCha20Poly1305;
use tari_common_types::types::{PrivateKey, PublicKey};
use tari_crypto::keys::PublicKey as PublicKeyTrait;

Expand Down Expand Up @@ -57,7 +57,7 @@ pub trait KeyManagerInterface: Clone + Send + Sync + 'static {

/// Encrypts the key manager state using the provided cipher. An error is returned if the state is already
/// encrypted.
async fn apply_encryption(&self, cipher: Aes256Gcm) -> Result<(), KeyManagerServiceError>;
async fn apply_encryption(&self, cipher: XChaCha20Poly1305) -> Result<(), KeyManagerServiceError>;

/// Decrypts the key manager state using the provided cipher. An error is returned if the state is not encrypted.
async fn remove_encryption(&self) -> Result<(), KeyManagerServiceError>;
Expand Down
4 changes: 2 additions & 2 deletions base_layer/wallet/src/key_manager_service/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

use aes_gcm::Aes256Gcm;
use chacha20poly1305::XChaCha20Poly1305;
use log::*;
use tari_common_types::types::PrivateKey;
use tari_key_manager::{cipher_seed::CipherSeed, key_manager::KeyManager};
Expand Down Expand Up @@ -154,7 +154,7 @@ impl KeyManagerInterface for KeyManagerMock {
self.get_key_at_index_mock(branch.into(), index).await
}

async fn apply_encryption(&self, _cipher: Aes256Gcm) -> Result<(), KeyManagerServiceError> {
async fn apply_encryption(&self, _cipher: XChaCha20Poly1305) -> Result<(), KeyManagerServiceError> {
unimplemented!("Not supported");
}

Expand Down
4 changes: 2 additions & 2 deletions base_layer/wallet/src/key_manager_service/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
use aes_gcm::Aes256Gcm;
use chacha20poly1305::XChaCha20Poly1305;
use futures::lock::Mutex;
use log::*;
use tari_common_types::types::PrivateKey;
Expand Down Expand Up @@ -110,7 +110,7 @@ where TBackend: KeyManagerBackend + 'static
Ok(key.k)
}

pub async fn apply_encryption(&self, cipher: Aes256Gcm) -> Result<(), KeyManagerServiceError> {
pub async fn apply_encryption(&self, cipher: XChaCha20Poly1305) -> Result<(), KeyManagerServiceError> {
self.db.apply_encryption(cipher).await?;
Ok(())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
use aes_gcm::Aes256Gcm;
use chacha20poly1305::XChaCha20Poly1305;

use crate::key_manager_service::{error::KeyManagerStorageError, storage::database::KeyManagerState};

Expand All @@ -35,7 +35,7 @@ pub trait KeyManagerBackend: Send + Sync + Clone {
/// This method will set the currently stored key index for the key manager.
fn set_key_index(&self, branch: String, index: u64) -> Result<(), KeyManagerStorageError>;
/// Apply encryption to the backend.
fn apply_encryption(&self, cipher: Aes256Gcm) -> Result<(), KeyManagerStorageError>;
fn apply_encryption(&self, cipher: XChaCha20Poly1305) -> Result<(), KeyManagerStorageError>;
/// Remove encryption from the backend.
fn remove_encryption(&self) -> Result<(), KeyManagerStorageError>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
mod backend;
use std::sync::Arc;

use aes_gcm::Aes256Gcm;
pub use backend::KeyManagerBackend;
use chacha20poly1305::XChaCha20Poly1305;

use crate::key_manager_service::error::KeyManagerStorageError;

Expand Down Expand Up @@ -95,7 +95,7 @@ where T: KeyManagerBackend + 'static

/// Encrypts the entire key manager with all branches.
/// This will only encrypt the index used, as the master seed phrase is not directly stored with the key manager.
pub async fn apply_encryption(&self, cipher: Aes256Gcm) -> Result<(), KeyManagerStorageError> {
pub async fn apply_encryption(&self, cipher: XChaCha20Poly1305) -> Result<(), KeyManagerStorageError> {
let db_clone = self.db.clone();
tokio::task::spawn_blocking(move || db_clone.apply_encryption(cipher))
.await
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

use std::convert::TryFrom;

use aes_gcm::Aes256Gcm;
use chacha20poly1305::XChaCha20Poly1305;
use chrono::{NaiveDateTime, Utc};
use diesel::{prelude::*, SqliteConnection};

Expand Down Expand Up @@ -150,43 +150,43 @@ pub struct KeyManagerStateUpdateSql {
primary_key_index: Option<Vec<u8>>,
}

impl Encryptable<Aes256Gcm> for KeyManagerStateSql {
impl Encryptable<XChaCha20Poly1305> for KeyManagerStateSql {
fn domain(&self, field_name: &'static str) -> Vec<u8> {
[Self::KEY_MANAGER, self.branch_seed.as_bytes(), field_name.as_bytes()]
.concat()
.to_vec()
}

fn encrypt(&mut self, cipher: &Aes256Gcm) -> Result<(), String> {
fn encrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> {
self.primary_key_index =
encrypt_bytes_integral_nonce(cipher, self.domain("primary_key_index"), self.primary_key_index.clone())?;

Ok(())
}

fn decrypt(&mut self, cipher: &Aes256Gcm) -> Result<(), String> {
fn decrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> {
self.primary_key_index =
decrypt_bytes_integral_nonce(cipher, self.domain("primary_key_index"), self.primary_key_index.clone())?;

Ok(())
}
}

impl Encryptable<Aes256Gcm> for NewKeyManagerStateSql {
impl Encryptable<XChaCha20Poly1305> for NewKeyManagerStateSql {
fn domain(&self, field_name: &'static str) -> Vec<u8> {
[Self::KEY_MANAGER, self.branch_seed.as_bytes(), field_name.as_bytes()]
.concat()
.to_vec()
}

fn encrypt(&mut self, cipher: &Aes256Gcm) -> Result<(), String> {
fn encrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> {
self.primary_key_index =
encrypt_bytes_integral_nonce(cipher, self.domain("primary_key_index"), self.primary_key_index.clone())?;

Ok(())
}

fn decrypt(&mut self, _cipher: &Aes256Gcm) -> Result<(), String> {
fn decrypt(&mut self, _cipher: &XChaCha20Poly1305) -> Result<(), String> {
unimplemented!("Not supported")
}
}
Loading

0 comments on commit 85acc2f

Please sign in to comment.