From e364994d30cb5e71b9dd87b485197d023d3121e0 Mon Sep 17 00:00:00 2001 From: jorgeantonio21 Date: Mon, 28 Nov 2022 09:29:07 +0000 Subject: [PATCH] fix: minimize potential memory leaks of sensitive data on the wallet code (#4953) Description --- Sensitive data is transmitted from wallet and db (ideally encrypted). However, the code is not completely robust against memory leaks. This PR proposes a way to minimize this risk. This implies enforcing encryption immediately whenever we process data for Sqlite interaction (or when fetching it). We further remove any update of sensitive data on SQL outputs, as this would increase the potential risk of memory leaks across the code. Motivation and Context --- Address security vulnerabilities in the wallet code. This PR is related to issue #4846. How Has This Been Tested? --- Refactor some of the existing unit tests. --- .../storage/sqlite_db/key_manager_state.rs | 15 +- .../storage/sqlite_db/mod.rs | 510 ++++++------------ .../storage/sqlite_db/new_output_sql.rs | 24 +- .../storage/sqlite_db/output_sql.rs | 105 ++-- .../wallet/src/storage/sqlite_db/wallet.rs | 105 ++-- .../transaction_service/storage/sqlite_db.rs | 23 +- base_layer/wallet/src/util/encryption.rs | 13 +- 7 files changed, 322 insertions(+), 473 deletions(-) diff --git a/base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs b/base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs index f5ee46da49..e4f55ae04f 100644 --- a/base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs +++ b/base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs @@ -25,6 +25,7 @@ use std::convert::TryFrom; use chacha20poly1305::XChaCha20Poly1305; use chrono::{NaiveDateTime, Utc}; use diesel::{prelude::*, SqliteConnection}; +use tari_utilities::Hidden; use crate::{ key_manager_service::{error::KeyManagerStorageError, storage::database::KeyManagerState}, @@ -158,8 +159,11 @@ impl Encryptable for KeyManagerStateSql { } 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())?; + self.primary_key_index = encrypt_bytes_integral_nonce( + cipher, + self.domain("primary_key_index"), + Hidden::hide(self.primary_key_index.clone()), + )?; Ok(()) } @@ -180,8 +184,11 @@ impl Encryptable for NewKeyManagerStateSql { } 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())?; + self.primary_key_index = encrypt_bytes_integral_nonce( + cipher, + self.domain("primary_key_index"), + Hidden::hide(self.primary_key_index.clone()), + )?; Ok(()) } diff --git a/base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs b/base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs index 8ec90ab19d..a581d30496 100644 --- a/base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs +++ b/base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs @@ -21,7 +21,7 @@ // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. use std::{ - convert::{TryFrom, TryInto}, + convert::TryFrom, sync::{Arc, RwLock}, }; @@ -44,7 +44,9 @@ use tari_common_types::{ use tari_core::transactions::transaction_components::{OutputType, TransactionOutput}; use tari_crypto::tari_utilities::{hex::Hex, ByteArray}; use tari_script::{ExecutionStack, TariScript}; +use tari_utilities::Hidden; use tokio::time::Instant; +use zeroize::Zeroize; use crate::{ output_manager_service::{ @@ -83,68 +85,44 @@ impl OutputManagerSqliteDatabase { } } - fn decrypt_if_necessary>( - &self, - o: &mut T, - ) -> Result<(), OutputManagerStorageError> { + fn insert(&self, key_value_pair: DbKeyValuePair, conn: &SqliteConnection) -> Result<(), OutputManagerStorageError> { let cipher = acquire_read_lock!(self.cipher); - if let Some(cipher) = cipher.as_ref() { - o.decrypt(cipher) - .map_err(|_| OutputManagerStorageError::AeadError("Decryption Error".to_string()))?; - } - Ok(()) - } - fn encrypt_if_necessary>( - &self, - o: &mut T, - ) -> Result<(), OutputManagerStorageError> { - let cipher = acquire_read_lock!(self.cipher); - if let Some(cipher) = cipher.as_ref() { - o.encrypt(cipher) - .map_err(|_| OutputManagerStorageError::AeadError("Encryption Error".to_string()))?; - } - Ok(()) - } - - fn insert(&self, key_value_pair: DbKeyValuePair, conn: &SqliteConnection) -> Result<(), OutputManagerStorageError> { match key_value_pair { DbKeyValuePair::UnspentOutput(c, o) => { if OutputSql::find_by_commitment_and_cancelled(&c.to_vec(), false, conn).is_ok() { return Err(OutputManagerStorageError::DuplicateOutput); } - let mut new_output = NewOutputSql::new(*o, OutputStatus::Unspent, None, None)?; - self.encrypt_if_necessary(&mut new_output)?; + let new_output = NewOutputSql::new(*o, OutputStatus::Unspent, None, None, cipher.as_ref())?; new_output.commit(conn)? }, DbKeyValuePair::UnspentOutputWithTxId(c, (tx_id, o)) => { if OutputSql::find_by_commitment_and_cancelled(&c.to_vec(), false, conn).is_ok() { return Err(OutputManagerStorageError::DuplicateOutput); } - let mut new_output = NewOutputSql::new(*o, OutputStatus::Unspent, Some(tx_id), None)?; - self.encrypt_if_necessary(&mut new_output)?; + let new_output = NewOutputSql::new(*o, OutputStatus::Unspent, Some(tx_id), None, cipher.as_ref())?; new_output.commit(conn)? }, DbKeyValuePair::OutputToBeReceived(c, (tx_id, o, coinbase_block_height)) => { if OutputSql::find_by_commitment_and_cancelled(&c.to_vec(), false, conn).is_ok() { return Err(OutputManagerStorageError::DuplicateOutput); } - let mut new_output = NewOutputSql::new( + let new_output = NewOutputSql::new( *o, OutputStatus::EncumberedToBeReceived, Some(tx_id), coinbase_block_height, + cipher.as_ref(), )?; - self.encrypt_if_necessary(&mut new_output)?; new_output.commit(conn)? }, DbKeyValuePair::KnownOneSidedPaymentScripts(script) => { - let mut script_sql = KnownOneSidedPaymentScriptSql::from(script); + let script_sql = + KnownOneSidedPaymentScriptSql::from_known_one_sided_payment_script(script, cipher.as_ref())?; if KnownOneSidedPaymentScriptSql::find(&script_sql.script_hash, conn).is_ok() { return Err(OutputManagerStorageError::DuplicateScript); } - self.encrypt_if_necessary(&mut script_sql)?; script_sql.commit(conn)? }, } @@ -153,19 +131,27 @@ impl OutputManagerSqliteDatabase { } impl OutputManagerBackend for OutputManagerSqliteDatabase { + fn apply_encryption(&self, _cipher: XChaCha20Poly1305) -> Result<(), OutputManagerStorageError> { + Ok(()) + } + + fn remove_encryption(&self) -> Result<(), OutputManagerStorageError> { + Ok(()) + } + #[allow(clippy::cognitive_complexity)] #[allow(clippy::too_many_lines)] fn fetch(&self, key: &DbKey) -> Result, OutputManagerStorageError> { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); + let cipher = acquire_read_lock!(self.cipher); let result = match key { DbKey::SpentOutput(k) => match OutputSql::find_status(&k.to_vec(), OutputStatus::Spent, &conn) { - Ok(mut o) => { - self.decrypt_if_necessary(&mut o)?; - Some(DbValue::SpentOutput(Box::new(DbUnblindedOutput::try_from(o)?))) - }, + Ok(o) => Some(DbValue::SpentOutput(Box::new( + o.to_db_unblinded_output(cipher.as_ref())?, + ))), Err(e) => { match e { OutputManagerStorageError::DieselError(DieselError::NotFound) => (), @@ -175,10 +161,9 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { }, }, DbKey::UnspentOutput(k) => match OutputSql::find_status(&k.to_vec(), OutputStatus::Unspent, &conn) { - Ok(mut o) => { - self.decrypt_if_necessary(&mut o)?; - Some(DbValue::UnspentOutput(Box::new(DbUnblindedOutput::try_from(o)?))) - }, + Ok(o) => Some(DbValue::UnspentOutput(Box::new( + o.to_db_unblinded_output(cipher.as_ref())?, + ))), Err(e) => { match e { OutputManagerStorageError::DieselError(DieselError::NotFound) => (), @@ -189,10 +174,9 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { }, DbKey::UnspentOutputHash(hash) => { match OutputSql::find_by_hash(hash.as_slice(), OutputStatus::Unspent, &(*conn)) { - Ok(mut o) => { - self.decrypt_if_necessary(&mut o)?; - Some(DbValue::UnspentOutput(Box::new(DbUnblindedOutput::try_from(o)?))) - }, + Ok(o) => Some(DbValue::UnspentOutput(Box::new( + o.to_db_unblinded_output(cipher.as_ref())?, + ))), Err(e) => { match e { OutputManagerStorageError::DieselError(DieselError::NotFound) => (), @@ -204,10 +188,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { }, DbKey::AnyOutputByCommitment(commitment) => { match OutputSql::find_by_commitment(&commitment.to_vec(), &conn) { - Ok(mut o) => { - self.decrypt_if_necessary(&mut o)?; - Some(DbValue::AnyOutput(Box::new(DbUnblindedOutput::try_from(o)?))) - }, + Ok(o) => Some(DbValue::AnyOutput(Box::new(o.to_db_unblinded_output(cipher.as_ref())?))), Err(e) => { match e { OutputManagerStorageError::DieselError(DieselError::NotFound) => (), @@ -218,79 +199,62 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { } }, DbKey::OutputsByTxIdAndStatus(tx_id, status) => { - let mut outputs = OutputSql::find_by_tx_id_and_status(*tx_id, *status, &conn)?; - for o in &mut outputs { - self.decrypt_if_necessary(o)?; - } + let outputs = OutputSql::find_by_tx_id_and_status(*tx_id, *status, &conn)?; + Some(DbValue::AnyOutputs( outputs .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) + .map(|o| o.clone().to_db_unblinded_output(cipher.as_ref())) .collect::, _>>()?, )) }, DbKey::UnspentOutputs => { - let mut outputs = OutputSql::index_status(OutputStatus::Unspent, &conn)?; - for o in &mut outputs { - self.decrypt_if_necessary(o)?; - } + let outputs = OutputSql::index_status(OutputStatus::Unspent, &conn)?; Some(DbValue::UnspentOutputs( outputs .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) + .map(|o| o.clone().to_db_unblinded_output(cipher.as_ref())) .collect::, _>>()?, )) }, DbKey::SpentOutputs => { - let mut outputs = OutputSql::index_status(OutputStatus::Spent, &conn)?; - for o in &mut outputs { - self.decrypt_if_necessary(o)?; - } + let outputs = OutputSql::index_status(OutputStatus::Spent, &conn)?; Some(DbValue::SpentOutputs( outputs .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) + .map(|o| o.clone().to_db_unblinded_output(cipher.as_ref())) .collect::, _>>()?, )) }, DbKey::TimeLockedUnspentOutputs(tip) => { - let mut outputs = OutputSql::index_time_locked(*tip, &conn)?; - for o in &mut outputs { - self.decrypt_if_necessary(o)?; - } + let outputs = OutputSql::index_time_locked(*tip, &conn)?; Some(DbValue::UnspentOutputs( outputs .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) + .map(|o| o.clone().to_db_unblinded_output(cipher.as_ref())) .collect::, _>>()?, )) }, DbKey::InvalidOutputs => { - let mut outputs = OutputSql::index_status(OutputStatus::Invalid, &conn)?; - for o in &mut outputs { - self.decrypt_if_necessary(o)?; - } + let outputs = OutputSql::index_status(OutputStatus::Invalid, &conn)?; Some(DbValue::InvalidOutputs( outputs .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) + .map(|o| o.clone().to_db_unblinded_output(cipher.as_ref())) .collect::, _>>()?, )) }, DbKey::KnownOneSidedPaymentScripts => { - let mut known_one_sided_payment_scripts = KnownOneSidedPaymentScriptSql::index(&conn)?; - for script in &mut known_one_sided_payment_scripts { - self.decrypt_if_necessary(script)?; - } + let known_one_sided_payment_scripts = KnownOneSidedPaymentScriptSql::index(&conn)?; Some(DbValue::KnownOneSidedPaymentScripts( known_one_sided_payment_scripts .iter() - .map(|script| KnownOneSidedPaymentScript::try_from(script.clone())) + .map(|script| script.clone().to_known_one_sided_payment_script(cipher.as_ref())) .collect::, _>>()?, )) }, @@ -314,27 +278,23 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { output_type: OutputType, ) -> Result, OutputManagerStorageError> { let conn = self.database_connection.get_pooled_connection()?; - let mut outputs = OutputSql::index_by_output_type(output_type, &conn)?; - for o in &mut outputs { - self.decrypt_if_necessary(o)?; - } + let outputs = OutputSql::index_by_output_type(output_type, &conn)?; + let cipher = acquire_read_lock!(self.cipher); outputs .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) + .map(|o| o.clone().to_db_unblinded_output(cipher.as_ref())) .collect::, _>>() } fn fetch_sorted_unspent_outputs(&self) -> Result, OutputManagerStorageError> { let conn = self.database_connection.get_pooled_connection()?; - let mut outputs = OutputSql::index_unspent(&conn)?; - for output in &mut outputs { - self.decrypt_if_necessary(output)?; - } + let outputs = OutputSql::index_unspent(&conn)?; + let cipher = acquire_read_lock!(self.cipher); outputs .into_iter() - .map(DbUnblindedOutput::try_from) + .map(|o| o.to_db_unblinded_output(cipher.as_ref())) .collect::, _>>() } @@ -342,10 +302,9 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); - let mut outputs = OutputSql::index_marked_deleted_in_block_is_null(&conn)?; - for output in &mut outputs { - self.decrypt_if_necessary(output)?; - } + let outputs = OutputSql::index_marked_deleted_in_block_is_null(&conn)?; + let cipher = acquire_read_lock!(self.cipher); + if start.elapsed().as_millis() > 0 { trace!( target: LOG_TARGET, @@ -358,7 +317,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { outputs .into_iter() - .map(DbUnblindedOutput::try_from) + .map(|o| o.to_db_unblinded_output(cipher.as_ref())) .collect::, _>>() } @@ -366,10 +325,9 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); - let mut outputs = OutputSql::index_unconfirmed(&conn)?; - for output in &mut outputs { - self.decrypt_if_necessary(output)?; - } + let outputs = OutputSql::index_unconfirmed(&conn)?; + let cipher = acquire_read_lock!(self.cipher); + if start.elapsed().as_millis() > 0 { trace!( target: LOG_TARGET, @@ -382,7 +340,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { outputs .into_iter() - .map(DbUnblindedOutput::try_from) + .map(|o| o.to_db_unblinded_output(cipher.as_ref())) .collect::, _>>() } @@ -390,6 +348,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); + let cipher = acquire_read_lock!(self.cipher); let mut msg = "".to_string(); let result = match op { @@ -404,10 +363,11 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { msg.push_str("Remove"); // Used by coinbase when mining. match OutputSql::find_by_commitment(&commitment.to_vec(), &conn) { - Ok(mut o) => { + Ok(o) => { o.delete(&conn)?; - self.decrypt_if_necessary(&mut o)?; - Ok(Some(DbValue::AnyOutput(Box::new(DbUnblindedOutput::try_from(o)?)))) + Ok(Some(DbValue::AnyOutput(Box::new( + o.to_db_unblinded_output(cipher.as_ref())?, + )))) }, Err(e) => match e { OutputManagerStorageError::DieselError(DieselError::NotFound) => Ok(None), @@ -445,6 +405,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); + let cipher = acquire_read_lock!(self.cipher); let mut outputs = OutputSql::index_status(OutputStatus::EncumberedToBeReceived, &conn)?; outputs.extend(OutputSql::index_status( @@ -452,9 +413,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { &conn, )?); outputs.extend(OutputSql::index_status(OutputStatus::UnspentMinedUnconfirmed, &conn)?); - for o in &mut outputs { - self.decrypt_if_necessary(o)?; - } + if start.elapsed().as_millis() > 0 { trace!( target: LOG_TARGET, @@ -466,7 +425,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { } outputs .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) + .map(|o| o.clone().to_db_unblinded_output(cipher.as_ref())) .collect::, _>>() } @@ -657,6 +616,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); + let cipher = acquire_read_lock!(self.cipher); let mut commitments = Vec::with_capacity(outputs_to_send.len()); for output in outputs_to_send { @@ -693,13 +653,13 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { })?; for co in outputs_to_receive { - let mut new_output = NewOutputSql::new( + let new_output = NewOutputSql::new( co.clone(), OutputStatus::ShortTermEncumberedToBeReceived, Some(tx_id), None, + cipher.as_ref(), )?; - self.encrypt_if_necessary(&mut new_output)?; new_output.commit(&conn)?; } if start.elapsed().as_millis() > 0 { @@ -785,6 +745,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); + let cipher = acquire_read_lock!(self.cipher); let output = OutputSql::first_by_mined_height_desc(&conn)?; if start.elapsed().as_millis() > 0 { @@ -797,10 +758,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { ); } match output { - Some(mut o) => { - self.decrypt_if_necessary(&mut o)?; - Ok(Some(o.try_into()?)) - }, + Some(o) => Ok(Some(o.to_db_unblinded_output(cipher.as_ref())?)), None => Ok(None), } } @@ -809,6 +767,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); + let cipher = acquire_read_lock!(self.cipher); let output = OutputSql::first_by_marked_deleted_height_desc(&conn)?; if start.elapsed().as_millis() > 0 { @@ -821,10 +780,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { ); } match output { - Some(mut o) => { - self.decrypt_if_necessary(&mut o)?; - Ok(Some(o.try_into()?)) - }, + Some(o) => Ok(Some(o.to_db_unblinded_output(cipher.as_ref())?)), None => Ok(None), } } @@ -985,104 +941,6 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { Ok(()) } - fn apply_encryption(&self, cipher: XChaCha20Poly1305) -> Result<(), OutputManagerStorageError> { - let mut current_cipher = acquire_write_lock!(self.cipher); - - if (*current_cipher).is_some() { - return Err(OutputManagerStorageError::AlreadyEncrypted); - } - - let start = Instant::now(); - let conn = self.database_connection.get_pooled_connection()?; - let acquire_lock = start.elapsed(); - let mut outputs = OutputSql::index(&conn)?; - - // If the db is already encrypted then the very first output we try to encrypt will fail. - for o in &mut outputs { - // Test if this output is encrypted or not to avoid a double encryption. - let _secret_key = PrivateKey::from_vec(&o.spending_key).map_err(|_| { - error!( - target: LOG_TARGET, - "Could not create PrivateKey from stored bytes, They might already be encrypted" - ); - OutputManagerStorageError::AlreadyEncrypted - })?; - o.encrypt(&cipher) - .map_err(|_| OutputManagerStorageError::AeadError("Encryption Error".to_string()))?; - o.update_encryption(&conn)?; - } - - let mut known_one_sided_payment_scripts = KnownOneSidedPaymentScriptSql::index(&conn)?; - - for script in &mut known_one_sided_payment_scripts { - let _secret_key = PrivateKey::from_vec(&script.private_key).map_err(|_| { - error!( - target: LOG_TARGET, - "Could not create PrivateKey from stored bytes, They might already be encrypted" - ); - OutputManagerStorageError::AlreadyEncrypted - })?; - script - .encrypt(&cipher) - .map_err(|_| OutputManagerStorageError::AeadError("Encryption Error".to_string()))?; - script.update_encryption(&conn)?; - } - - (*current_cipher) = Some(cipher); - if start.elapsed().as_millis() > 0 { - trace!( - target: LOG_TARGET, - "sqlite profile - apply_encryption: lock {} + db_op {} = {} ms", - acquire_lock.as_millis(), - (start.elapsed() - acquire_lock).as_millis(), - start.elapsed().as_millis() - ); - } - - Ok(()) - } - - fn remove_encryption(&self) -> Result<(), OutputManagerStorageError> { - let mut current_cipher = acquire_write_lock!(self.cipher); - let cipher = if let Some(cipher) = (*current_cipher).clone().take() { - cipher - } else { - return Ok(()); - }; - let start = Instant::now(); - let conn = self.database_connection.get_pooled_connection()?; - let acquire_lock = start.elapsed(); - let mut outputs = OutputSql::index(&conn)?; - - for o in &mut outputs { - o.decrypt(&cipher) - .map_err(|_| OutputManagerStorageError::AeadError("Encryption Error".to_string()))?; - o.update_encryption(&conn)?; - } - - let mut known_one_sided_payment_scripts = KnownOneSidedPaymentScriptSql::index(&conn)?; - - for script in &mut known_one_sided_payment_scripts { - script - .decrypt(&cipher) - .map_err(|_| OutputManagerStorageError::AeadError("Encryption Error".to_string()))?; - script.update_encryption(&conn)?; - } - - // Now that all the decryption has been completed we can safely remove the cipher fully - std::mem::drop((*current_cipher).take()); - if start.elapsed().as_millis() > 0 { - trace!( - target: LOG_TARGET, - "sqlite profile - remove_encryption: lock {} + db_op {} = {} ms", - acquire_lock.as_millis(), - (start.elapsed() - acquire_lock).as_millis(), - start.elapsed().as_millis() - ); - } - Ok(()) - } - fn set_coinbase_abandoned(&self, tx_id: TxId, abandoned: bool) -> Result<(), OutputManagerStorageError> { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; @@ -1152,12 +1010,18 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); + let cipher = acquire_read_lock!(self.cipher); if OutputSql::find_by_commitment_and_cancelled(&output.commitment.to_vec(), false, &conn).is_ok() { return Err(OutputManagerStorageError::DuplicateOutput); } - let mut new_output = NewOutputSql::new(output, OutputStatus::EncumberedToBeReceived, Some(tx_id), None)?; - self.encrypt_if_necessary(&mut new_output)?; + let new_output = NewOutputSql::new( + output, + OutputStatus::EncumberedToBeReceived, + Some(tx_id), + None, + cipher.as_ref(), + )?; new_output.commit(&conn)?; if start.elapsed().as_millis() > 0 { @@ -1182,10 +1046,10 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { let start = Instant::now(); let conn = self.database_connection.get_pooled_connection()?; let acquire_lock = start.elapsed(); - let mut outputs = OutputSql::fetch_unspent_outputs_for_spending(selection_criteria, amount, tip_height, &conn)?; - for o in &mut outputs { - self.decrypt_if_necessary(o)?; - } + let cipher = acquire_read_lock!(self.cipher); + + let outputs = OutputSql::fetch_unspent_outputs_for_spending(selection_criteria, amount, tip_height, &conn)?; + trace!( target: LOG_TARGET, "sqlite profile - fetch_unspent_outputs_for_spending: lock {} + db_op {} = {} ms", @@ -1195,33 +1059,28 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase { ); outputs .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) + .map(|o| o.clone().to_db_unblinded_output(cipher.as_ref())) .collect::, _>>() } fn fetch_outputs_by_tx_id(&self, tx_id: TxId) -> Result, OutputManagerStorageError> { let conn = self.database_connection.get_pooled_connection()?; - let mut outputs = OutputSql::find_by_tx_id(tx_id, &conn)?; - for o in &mut outputs { - self.decrypt_if_necessary(o)?; - } + let outputs = OutputSql::find_by_tx_id(tx_id, &conn)?; + let cipher = acquire_read_lock!(self.cipher); + outputs .iter() - .map(|o| DbUnblindedOutput::try_from(o.clone())) + .map(|o| o.clone().to_db_unblinded_output(cipher.as_ref())) .collect::, _>>() } fn fetch_outputs_by(&self, q: OutputBackendQuery) -> Result, OutputManagerStorageError> { let conn = self.database_connection.get_pooled_connection()?; + let cipher = acquire_read_lock!(self.cipher); Ok(OutputSql::fetch_outputs_by(q, &conn)? .into_iter() - .filter_map(|mut x| { - if let Err(e) = self.decrypt_if_necessary(&mut x) { - error!(target: LOG_TARGET, "failed to `decrypt_if_necessary`: {:#?}", e); - return None; - } - - DbUnblindedOutput::try_from(x) + .filter_map(|x| { + x.to_db_unblinded_output(cipher.as_ref()) .map_err(|e| { error!( target: LOG_TARGET, @@ -1256,13 +1115,11 @@ fn update_outputs_with_tx_id_and_status_to_new_status( } /// These are the fields that can be updated for an Output -#[derive(Default)] +#[derive(Clone, Default)] pub struct UpdateOutput { status: Option, received_in_tx_id: Option>, spent_in_tx_id: Option>, - spending_key: Option>, - script_private_key: Option>, metadata_signature_ephemeral_commitment: Option>, metadata_signature_ephemeral_pubkey: Option>, metadata_signature_u_a: Option>, @@ -1278,8 +1135,6 @@ pub struct UpdateOutputSql { status: Option, received_in_tx_id: Option>, spent_in_tx_id: Option>, - spending_key: Option>, - script_private_key: Option>, metadata_signature_ephemeral_commitment: Option>, metadata_signature_ephemeral_pubkey: Option>, metadata_signature_u_a: Option>, @@ -1294,8 +1149,6 @@ impl From for UpdateOutputSql { fn from(u: UpdateOutput) -> Self { Self { status: u.status.map(|t| t as i32), - spending_key: u.spending_key, - script_private_key: u.script_private_key, metadata_signature_ephemeral_commitment: u.metadata_signature_ephemeral_commitment, metadata_signature_ephemeral_pubkey: u.metadata_signature_ephemeral_pubkey, metadata_signature_u_a: u.metadata_signature_u_a, @@ -1327,7 +1180,6 @@ pub struct KnownOneSidedPaymentScriptSql { #[derive(AsChangeset)] #[table_name = "known_one_sided_payment_scripts"] pub struct UpdateKnownOneSidedPaymentScript { - private_key: Option>, script: Option>, input: Option>, } @@ -1386,27 +1238,17 @@ impl KnownOneSidedPaymentScriptSql { KnownOneSidedPaymentScriptSql::find(&self.script_hash, conn) } - /// Update the changed fields of this record after encryption/decryption is performed - pub fn update_encryption(&self, conn: &SqliteConnection) -> Result<(), OutputManagerStorageError> { - let _known_one_sided_payment_script_sql = self.update( - UpdateKnownOneSidedPaymentScript { - private_key: Some(self.private_key.clone()), - script: None, - input: None, - }, - conn, - )?; - Ok(()) - } -} - -/// Conversion from an KnownOneSidedPaymentScript to the Sql datatype form -impl TryFrom for KnownOneSidedPaymentScript { - type Error = OutputManagerStorageError; + /// Conversion from an KnownOneSidedPaymentScriptSQL to the datatype form + pub fn to_known_one_sided_payment_script( + mut self, + cipher: Option<&XChaCha20Poly1305>, + ) -> Result { + if let Some(cipher) = cipher { + self.decrypt(cipher).map_err(OutputManagerStorageError::AeadError)?; + } - fn try_from(o: KnownOneSidedPaymentScriptSql) -> Result { - let script_hash = o.script_hash; - let private_key = PrivateKey::from_bytes(&o.private_key).map_err(|_| { + let script_hash = self.script_hash; + let private_key = PrivateKey::from_bytes(&self.private_key).map_err(|_| { error!( target: LOG_TARGET, "Could not create PrivateKey from stored bytes, They might be encrypted" @@ -1415,19 +1257,24 @@ impl TryFrom for KnownOneSidedPaymentScript { reason: "PrivateKey could not be converted from bytes".to_string(), } })?; - let script = TariScript::from_bytes(&o.script).map_err(|_| { + + // in order to avoid memory leaks of sensitive data, we zeroize the current private key buffer + self.private_key.zeroize(); + + let script = TariScript::from_bytes(&self.script).map_err(|_| { error!(target: LOG_TARGET, "Could not create tari script from stored bytes"); OutputManagerStorageError::ConversionError { reason: "Tari Script could not be converted from bytes".to_string(), } })?; - let input = ExecutionStack::from_bytes(&o.input).map_err(|_| { + let input = ExecutionStack::from_bytes(&self.input).map_err(|_| { error!(target: LOG_TARGET, "Could not create execution stack from stored bytes"); OutputManagerStorageError::ConversionError { reason: "ExecutionStack could not be converted from bytes".to_string(), } })?; - let script_lock_height = o.script_lock_height as u64; + let script_lock_height = self.script_lock_height as u64; + Ok(KnownOneSidedPaymentScript { script_hash, private_key, @@ -1436,23 +1283,31 @@ impl TryFrom for KnownOneSidedPaymentScript { script_lock_height, }) } -} -/// Conversion from an KnownOneSidedPaymentScriptSQL to the datatype form -impl From for KnownOneSidedPaymentScriptSql { - fn from(known_script: KnownOneSidedPaymentScript) -> Self { + /// Conversion from an KnownOneSidedPaymentScriptSQL to the datatype form + pub fn from_known_one_sided_payment_script( + known_script: KnownOneSidedPaymentScript, + cipher: Option<&XChaCha20Poly1305>, + ) -> Result { let script_lock_height = known_script.script_lock_height as i64; let script_hash = known_script.script_hash; let private_key = known_script.private_key.as_bytes().to_vec(); let script = known_script.script.to_bytes().to_vec(); let input = known_script.input.to_bytes().to_vec(); - KnownOneSidedPaymentScriptSql { + + let mut output = KnownOneSidedPaymentScriptSql { script_hash, private_key, script, input, script_lock_height, + }; + + // encrypt in place the output, so no private_key memory leaks remain + if let Some(cipher) = cipher { + output.encrypt(cipher).map_err(OutputManagerStorageError::AeadError)?; } + Ok(output) } } @@ -1468,7 +1323,11 @@ impl Encryptable for KnownOneSidedPaymentScriptSql { } fn encrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> { - self.private_key = encrypt_bytes_integral_nonce(cipher, self.domain("private_key"), self.private_key.clone())?; + self.private_key = encrypt_bytes_integral_nonce( + cipher, + self.domain("private_key"), + Hidden::hide(self.private_key.clone()), + )?; Ok(()) } @@ -1480,12 +1339,11 @@ impl Encryptable for KnownOneSidedPaymentScriptSql { #[cfg(test)] mod test { - use std::{mem::size_of, time::Duration}; + use std::mem::size_of; use chacha20poly1305::{aead::NewAead, Key, XChaCha20Poly1305}; use diesel::{Connection, SqliteConnection}; use rand::{rngs::OsRng, RngCore}; - use tari_common_sqlite::sqlite_connection_pool::SqliteConnectionPool; use tari_common_types::types::CommitmentFactory; use tari_core::transactions::{ tari_amount::MicroTari, @@ -1495,22 +1353,15 @@ mod test { }; use tari_script::script; use tari_test_utils::random; + use tari_utilities::ByteArray; use tempfile::tempdir; use crate::{ output_manager_service::storage::{ - database::{DbKey, OutputManagerBackend}, models::DbUnblindedOutput, - sqlite_db::{ - new_output_sql::NewOutputSql, - output_sql::OutputSql, - OutputManagerSqliteDatabase, - OutputStatus, - UpdateOutput, - }, + sqlite_db::{new_output_sql::NewOutputSql, output_sql::OutputSql, OutputStatus, UpdateOutput}, OutputSource, }, - storage::sqlite_utilities::wallet_db_connection::WalletDbConnection, util::encryption::Encryptable, }; @@ -1547,7 +1398,7 @@ mod test { for _i in 0..2 { let (_, uo) = make_input(MicroTari::from(100 + OsRng.next_u64() % 1000)); let uo = DbUnblindedOutput::from_unblinded_output(uo, &factories, None, OutputSource::Unknown).unwrap(); - let o = NewOutputSql::new(uo, OutputStatus::Unspent, None, None).unwrap(); + let o = NewOutputSql::new(uo, OutputStatus::Unspent, None, None, None).unwrap(); outputs.push(o.clone()); outputs_unspent.push(o.clone()); o.commit(&conn).unwrap(); @@ -1556,7 +1407,7 @@ mod test { for _i in 0..3 { let (_, uo) = make_input(MicroTari::from(100 + OsRng.next_u64() % 1000)); let uo = DbUnblindedOutput::from_unblinded_output(uo, &factories, None, OutputSource::Unknown).unwrap(); - let o = NewOutputSql::new(uo, OutputStatus::Spent, None, None).unwrap(); + let o = NewOutputSql::new(uo, OutputStatus::Spent, None, None, None).unwrap(); outputs.push(o.clone()); outputs_spent.push(o.clone()); o.commit(&conn).unwrap(); @@ -1655,35 +1506,39 @@ mod test { conn.execute("PRAGMA foreign_keys = ON").unwrap(); let factories = CryptoFactories::default(); - let (_, uo) = make_input(MicroTari::from(100 + OsRng.next_u64() % 1000)); - let uo = DbUnblindedOutput::from_unblinded_output(uo, &factories, None, OutputSource::Unknown).unwrap(); - let output = NewOutputSql::new(uo, OutputStatus::Unspent, None, None).unwrap(); - let mut key = [0u8; size_of::()]; OsRng.fill_bytes(&mut key); let key_ga = Key::from_slice(&key); let cipher = XChaCha20Poly1305::new(key_ga); + let (_, uo) = make_input(MicroTari::from(100 + OsRng.next_u64() % 1000)); + let decrypted_spending_key = uo.spending_key.clone().to_vec(); + + let uo = DbUnblindedOutput::from_unblinded_output(uo, &factories, None, OutputSource::Unknown).unwrap(); + + let mut output = NewOutputSql::new(uo, OutputStatus::Unspent, None, None, Some(&cipher)).unwrap(); + output.commit(&conn).unwrap(); - let unencrypted_output = OutputSql::find(output.spending_key.as_slice(), &conn).unwrap(); + let mut encrypted_output = OutputSql::find(output.spending_key.as_slice(), &conn).unwrap(); - assert!(unencrypted_output.clone().decrypt(&cipher).is_err()); - unencrypted_output.delete(&conn).unwrap(); + // Aead encryption of spending key contains 24 bytes nonce + 16 bytes tag + 32 bytes encrypted spneding key + assert_eq!(encrypted_output.spending_key.len(), 32 + 24 + 16); + assert_eq!(encrypted_output.spending_key, output.spending_key); - let mut encrypted_output = output.clone(); - encrypted_output.encrypt(&cipher).unwrap(); - encrypted_output.commit(&conn).unwrap(); + let mut decrypted_output = encrypted_output.clone(); - let outputs = OutputSql::index(&conn).unwrap(); - let mut decrypted_output = outputs[0].clone(); decrypted_output.decrypt(&cipher).unwrap(); - assert_eq!(decrypted_output.spending_key, output.spending_key); + assert_eq!(decrypted_output.spending_key.len(), 32); + assert_eq!(decrypted_output.spending_key, decrypted_spending_key); + + let mut output_2 = output.clone(); + output_2.decrypt(&cipher).unwrap(); + assert_eq!(decrypted_output.spending_key, output_2.spending_key); let wrong_key = Key::from_slice(b"an example very very wrong key!!"); let wrong_cipher = XChaCha20Poly1305::new(wrong_key); - assert!(outputs[0].clone().decrypt(&wrong_cipher).is_err()); - - decrypted_output.update_encryption(&conn).unwrap(); + assert!(encrypted_output.decrypt(&wrong_cipher).is_err()); + assert!(output.decrypt(&wrong_cipher).is_err()); assert_eq!( OutputSql::find(output.spending_key.as_slice(), &conn) @@ -1692,66 +1547,9 @@ mod test { output.spending_key ); - decrypted_output.encrypt(&cipher).unwrap(); - decrypted_output.update_encryption(&conn).unwrap(); - let outputs = OutputSql::index(&conn).unwrap(); let mut decrypted_output2 = outputs[0].clone(); decrypted_output2.decrypt(&cipher).unwrap(); - assert_eq!(decrypted_output2.spending_key, output.spending_key); - } - - #[test] - fn test_apply_remove_encryption() { - let db_name = format!("{}.sqlite3", random::string(8).as_str()); - let temp_dir = tempdir().unwrap(); - let db_folder = temp_dir.path().to_str().unwrap().to_string(); - let db_path = format!("{}{}", db_folder, db_name); - - embed_migrations!("./migrations"); - let mut pool = SqliteConnectionPool::new(db_path.clone(), 1, true, true, Duration::from_secs(60)); - pool.create_pool() - .unwrap_or_else(|_| panic!("Error connecting to {}", db_path)); - // Note: For this test the connection pool is setup with a pool size of one; the pooled connection must go out - // of scope to be released once obtained otherwise subsequent calls to obtain a pooled connection will fail . - { - let conn = pool - .get_pooled_connection() - .unwrap_or_else(|_| panic!("Error connecting to {}", db_path)); - - embedded_migrations::run_with_output(&conn, &mut std::io::stdout()).expect("Migration failed"); - let factories = CryptoFactories::default(); - - let (_, uo) = make_input(MicroTari::from(100 + OsRng.next_u64() % 1000)); - let uo = DbUnblindedOutput::from_unblinded_output(uo, &factories, None, OutputSource::Unknown).unwrap(); - let output = NewOutputSql::new(uo, OutputStatus::Unspent, None, None).unwrap(); - output.commit(&conn).unwrap(); - - let (_, uo2) = make_input(MicroTari::from(100 + OsRng.next_u64() % 1000)); - let uo2 = DbUnblindedOutput::from_unblinded_output(uo2, &factories, None, OutputSource::Unknown).unwrap(); - let output2 = NewOutputSql::new(uo2, OutputStatus::Unspent, None, None).unwrap(); - output2.commit(&conn).unwrap(); - } - - let mut key = [0u8; size_of::()]; - OsRng.fill_bytes(&mut key); - let key_ga = Key::from_slice(&key); - let cipher = XChaCha20Poly1305::new(key_ga); - - let connection = WalletDbConnection::new(pool, None); - - let db1 = OutputManagerSqliteDatabase::new(connection.clone(), Some(cipher.clone())); - assert!(db1.apply_encryption(cipher.clone()).is_err()); - - let db2 = OutputManagerSqliteDatabase::new(connection.clone(), None); - assert!(db2.remove_encryption().is_ok()); - db2.apply_encryption(cipher).unwrap(); - - let db3 = OutputManagerSqliteDatabase::new(connection, None); - assert!(db3.fetch(&DbKey::UnspentOutputs).is_err()); - - db2.remove_encryption().unwrap(); - - assert!(db3.fetch(&DbKey::UnspentOutputs).is_ok()); + assert_eq!(decrypted_output2.spending_key, decrypted_output.spending_key); } } diff --git a/base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs b/base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs index f5a3a2f247..f479848a01 100644 --- a/base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs +++ b/base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs @@ -25,7 +25,7 @@ use chacha20poly1305::XChaCha20Poly1305; use derivative::Derivative; use diesel::{prelude::*, SqliteConnection}; use tari_common_types::transaction::TxId; -use tari_utilities::ByteArray; +use tari_utilities::{ByteArray, Hidden}; use crate::{ output_manager_service::{ @@ -77,10 +77,12 @@ impl NewOutputSql { status: OutputStatus, received_in_tx_id: Option, coinbase_block_height: Option, + cipher: Option<&XChaCha20Poly1305>, ) -> Result { let mut covenant = Vec::new(); BorshSerialize::serialize(&output.unblinded_output.covenant, &mut covenant).unwrap(); - Ok(Self { + + let mut output = Self { commitment: Some(output.commitment.to_vec()), spending_key: output.unblinded_output.spending_key.to_vec(), value: output.unblinded_output.value.as_u64() as i64, @@ -113,7 +115,14 @@ impl NewOutputSql { encrypted_value: output.unblinded_output.encrypted_value.to_vec(), minimum_value_promise: output.unblinded_output.minimum_value_promise.as_u64() as i64, source: output.source as i32, - }) + }; + + if let Some(cipher) = cipher { + output + .encrypt(cipher) + .map_err(|_| OutputManagerStorageError::AeadError("Encryption Error".to_string()))?; + } + Ok(output) } /// Write this struct to the database @@ -132,13 +141,16 @@ impl Encryptable for NewOutputSql { } fn encrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> { - self.spending_key = - encrypt_bytes_integral_nonce(cipher, self.domain("spending_key"), self.spending_key.clone())?; + self.spending_key = encrypt_bytes_integral_nonce( + cipher, + self.domain("spending_key"), + Hidden::hide(self.spending_key.clone()), + )?; self.script_private_key = encrypt_bytes_integral_nonce( cipher, self.domain("script_private_key"), - self.script_private_key.clone(), + Hidden::hide(self.script_private_key.clone()), )?; Ok(()) diff --git a/base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs b/base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs index 0da1952fd7..97877f86ad 100644 --- a/base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs +++ b/base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs @@ -39,6 +39,8 @@ use tari_core::transactions::{ }; use tari_crypto::{commitment::HomomorphicCommitmentFactory, tari_utilities::ByteArray}; use tari_script::{ExecutionStack, TariScript}; +use tari_utilities::Hidden; +use zeroize::Zeroize; use crate::{ output_manager_service::{ @@ -607,33 +609,21 @@ impl OutputSql { OutputSql::find(&self.spending_key, conn) } - /// Update the changed fields of this record after encryption/decryption is performed - pub fn update_encryption(&self, conn: &SqliteConnection) -> Result<(), OutputManagerStorageError> { - let _output_sql = self.update( - UpdateOutput { - spending_key: Some(self.spending_key.clone()), - script_private_key: Some(self.script_private_key.clone()), - ..Default::default() - }, - conn, - )?; - Ok(()) - } -} - -/// Conversion from an DbUnblindedOutput to the Sql datatype form -impl TryFrom for DbUnblindedOutput { - type Error = OutputManagerStorageError; - #[allow(clippy::too_many_lines)] - fn try_from(o: OutputSql) -> Result { + pub fn to_db_unblinded_output( + mut self, + cipher: Option<&XChaCha20Poly1305>, + ) -> Result { + if let Some(cipher) = cipher { + self.decrypt(cipher).map_err(OutputManagerStorageError::AeadError)?; + } + let features: OutputFeatures = - serde_json::from_str(&o.features_json).map_err(|s| OutputManagerStorageError::ConversionError { + serde_json::from_str(&self.features_json).map_err(|s| OutputManagerStorageError::ConversionError { reason: format!("Could not convert json into OutputFeatures:{}", s), })?; - let encrypted_value = EncryptedValue::from_bytes(&o.encrypted_value)?; - let covenant = BorshDeserialize::deserialize(&mut o.covenant.as_bytes()).map_err(|e| { + let covenant = BorshDeserialize::deserialize(&mut self.covenant.as_bytes()).map_err(|e| { error!( target: LOG_TARGET, "Could not create Covenant from stored bytes ({}), They might be encrypted", e @@ -642,9 +632,11 @@ impl TryFrom for DbUnblindedOutput { reason: "Covenant could not be converted from bytes".to_string(), } })?; + + let encrypted_value = EncryptedValue::from_bytes(&self.encrypted_value)?; let unblinded_output = UnblindedOutput::new_current_version( - MicroTari::from(o.value as u64), - PrivateKey::from_vec(&o.spending_key).map_err(|_| { + MicroTari::from(self.value as u64), + PrivateKey::from_vec(&self.spending_key).map_err(|_| { error!( target: LOG_TARGET, "Could not create PrivateKey from stored bytes, They might be encrypted" @@ -654,9 +646,9 @@ impl TryFrom for DbUnblindedOutput { } })?, features, - TariScript::from_bytes(o.script.as_slice())?, - ExecutionStack::from_bytes(o.input_data.as_slice())?, - PrivateKey::from_vec(&o.script_private_key).map_err(|_| { + TariScript::from_bytes(self.script.as_slice())?, + ExecutionStack::from_bytes(self.input_data.as_slice())?, + PrivateKey::from_vec(&self.script_private_key).map_err(|_| { error!( target: LOG_TARGET, "Could not create PrivateKey from stored bytes, They might be encrypted" @@ -665,7 +657,7 @@ impl TryFrom for DbUnblindedOutput { reason: "PrivateKey could not be converted from bytes".to_string(), } })?, - PublicKey::from_vec(&o.sender_offset_public_key).map_err(|_| { + PublicKey::from_vec(&self.sender_offset_public_key).map_err(|_| { error!( target: LOG_TARGET, "Could not create PublicKey from stored bytes, They might be encrypted" @@ -675,7 +667,7 @@ impl TryFrom for DbUnblindedOutput { } })?, ComAndPubSignature::new( - Commitment::from_vec(&o.metadata_signature_ephemeral_commitment).map_err(|_| { + Commitment::from_vec(&self.metadata_signature_ephemeral_commitment).map_err(|_| { error!( target: LOG_TARGET, "Could not create Commitment from stored bytes, They might be encrypted" @@ -684,7 +676,7 @@ impl TryFrom for DbUnblindedOutput { reason: "Commitment could not be converted from bytes".to_string(), } })?, - PublicKey::from_vec(&o.metadata_signature_ephemeral_pubkey).map_err(|_| { + PublicKey::from_vec(&self.metadata_signature_ephemeral_pubkey).map_err(|_| { error!( target: LOG_TARGET, "Could not create PublicKey from stored bytes, They might be encrypted" @@ -693,7 +685,7 @@ impl TryFrom for DbUnblindedOutput { reason: "PublicKey could not be converted from bytes".to_string(), } })?, - PrivateKey::from_vec(&o.metadata_signature_u_a).map_err(|_| { + PrivateKey::from_vec(&self.metadata_signature_u_a).map_err(|_| { error!( target: LOG_TARGET, "Could not create PrivateKey from stored bytes, They might be encrypted" @@ -702,7 +694,7 @@ impl TryFrom for DbUnblindedOutput { reason: "PrivateKey could not be converted from bytes".to_string(), } })?, - PrivateKey::from_vec(&o.metadata_signature_u_x).map_err(|_| { + PrivateKey::from_vec(&self.metadata_signature_u_x).map_err(|_| { error!( target: LOG_TARGET, "Could not create PrivateKey from stored bytes, They might be encrypted" @@ -711,7 +703,7 @@ impl TryFrom for DbUnblindedOutput { reason: "PrivateKey could not be converted from bytes".to_string(), } })?, - PrivateKey::from_vec(&o.metadata_signature_u_y).map_err(|_| { + PrivateKey::from_vec(&self.metadata_signature_u_y).map_err(|_| { error!( target: LOG_TARGET, "Could not create PrivateKey from stored bytes, They might be encrypted" @@ -721,13 +713,17 @@ impl TryFrom for DbUnblindedOutput { } })?, ), - o.script_lock_height as u64, + self.script_lock_height as u64, covenant, encrypted_value, - MicroTari::from(o.minimum_value_promise as u64), + MicroTari::from(self.minimum_value_promise as u64), ); - let hash = match o.hash { + // we manually zeroize the sensitive data associated with OuptputSql, to avoid any leaks + self.spending_key.zeroize(); + self.script_private_key.zeroize(); + + let hash = match self.hash { None => { let factories = CryptoFactories::default(); unblinded_output.as_transaction_output(&factories)?.hash() @@ -742,7 +738,7 @@ impl TryFrom for DbUnblindedOutput { }, }, }; - let commitment = match o.commitment { + let commitment = match self.commitment { None => { let factories = CryptoFactories::default(); factories @@ -751,34 +747,34 @@ impl TryFrom for DbUnblindedOutput { }, Some(c) => Commitment::from_vec(&c)?, }; - let spending_priority = (o.spending_priority as u32).into(); - let mined_in_block = match o.mined_in_block { + let spending_priority = (self.spending_priority as u32).into(); + let mined_in_block = match self.mined_in_block { Some(v) => match v.try_into() { Ok(v) => Some(v), Err(_) => None, }, None => None, }; - let marked_deleted_in_block = match o.marked_deleted_in_block { + let marked_deleted_in_block = match self.marked_deleted_in_block { Some(v) => match v.try_into() { Ok(v) => Some(v), Err(_) => None, }, None => None, }; - Ok(Self { + Ok(DbUnblindedOutput { commitment, unblinded_output, hash, - status: o.status.try_into()?, - mined_height: o.mined_height.map(|mh| mh as u64), + status: self.status.try_into()?, + mined_height: self.mined_height.map(|mh| mh as u64), mined_in_block, - mined_mmr_position: o.mined_mmr_position.map(|mp| mp as u64), - mined_timestamp: o.mined_timestamp, - marked_deleted_at_height: o.marked_deleted_at_height.map(|d| d as u64), + mined_mmr_position: self.mined_mmr_position.map(|mp| mp as u64), + mined_timestamp: self.mined_timestamp, + marked_deleted_at_height: self.marked_deleted_at_height.map(|d| d as u64), marked_deleted_in_block, spending_priority, - source: o.source.try_into()?, + source: self.source.try_into()?, }) } } @@ -792,13 +788,16 @@ impl Encryptable for OutputSql { } fn encrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> { - self.spending_key = - encrypt_bytes_integral_nonce(cipher, self.domain("spending_key"), self.spending_key.clone())?; + self.spending_key = encrypt_bytes_integral_nonce( + cipher, + self.domain("spending_key"), + Hidden::hide(self.spending_key.clone()), + )?; self.script_private_key = encrypt_bytes_integral_nonce( cipher, self.domain("script_private_key"), - self.script_private_key.clone(), + Hidden::hide(self.script_private_key.clone()), )?; Ok(()) @@ -817,9 +816,3 @@ impl Encryptable for OutputSql { Ok(()) } } - -// impl PartialEq for OutputSql { -// fn eq(&self, other: &NewOutputSql) -> bool { -// &NewOutputSql::from(self.clone()) == other -// } -// } diff --git a/base_layer/wallet/src/storage/sqlite_db/wallet.rs b/base_layer/wallet/src/storage/sqlite_db/wallet.rs index f0fcfb36a3..1b6ebdf4d7 100644 --- a/base_layer/wallet/src/storage/sqlite_db/wallet.rs +++ b/base_layer/wallet/src/storage/sqlite_db/wallet.rs @@ -44,10 +44,11 @@ use tari_key_manager::cipher_seed::CipherSeed; use tari_utilities::{ hex::{from_hex, Hex}, message_format::MessageFormat, + Hidden, SafePassword, }; use tokio::time::Instant; -use zeroize::Zeroizing; +use zeroize::{Zeroize, Zeroizing}; use crate::{ error::WalletStorageError, @@ -93,7 +94,7 @@ impl WalletSqliteDatabase { WalletSettingSql::new(DbKey::WalletBirthday.to_string(), birthday.to_string()).set(conn)?; }, Some(cipher) => { - let seed_bytes = seed.encipher(None)?; + let seed_bytes = Hidden::hide(seed.encipher(None)?); let ciphertext_integral_nonce = encrypt_bytes_integral_nonce(cipher, b"wallet_setting_master_seed".to_vec(), seed_bytes) .map_err(|e| WalletStorageError::AeadError(format!("Encryption Error:{}", e)))?; @@ -110,13 +111,17 @@ impl WalletSqliteDatabase { let seed = match cipher.as_ref() { None => CipherSeed::from_enciphered_bytes(&from_hex(seed_str.as_str())?, None)?, Some(cipher) => { - let decrypted_key_bytes = decrypt_bytes_integral_nonce( - cipher, - b"wallet_setting_master_seed".to_vec(), - from_hex(seed_str.as_str())?, - ) - .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?; - CipherSeed::from_enciphered_bytes(&decrypted_key_bytes, None)? + // Decrypted_key_bytes contains sensitive data regarding decrypted + // seed words. For this reason, we should zeroize the underlying data buffer + let decrypted_key_bytes = Hidden::hide( + decrypt_bytes_integral_nonce( + cipher, + b"wallet_setting_master_seed".to_vec(), + from_hex(seed_str.as_str())?, + ) + .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?, + ); + CipherSeed::from_enciphered_bytes(decrypted_key_bytes.reveal(), None)? }, }; @@ -175,7 +180,9 @@ impl WalletSqliteDatabase { WalletSettingSql::new(DbKey::TorId.to_string(), tor_string).set(conn)?; }, Some(cipher) => { - let bytes = bincode::serialize(&tor).map_err(|e| WalletStorageError::ConversionError(e.to_string()))?; + let bytes = Hidden::hide( + bincode::serialize(&tor).map_err(|e| WalletStorageError::ConversionError(e.to_string()))?, + ); let ciphertext_integral_nonce = encrypt_bytes_integral_nonce(cipher, b"wallet_setting_tor_id".to_vec(), bytes) .map_err(|e| WalletStorageError::AeadError(format!("Encryption Error:{}", e)))?; @@ -195,11 +202,14 @@ impl WalletSqliteDatabase { TorIdentity::from_json(&key_str).map_err(|e| WalletStorageError::ConversionError(e.to_string()))? }, Some(cipher) => { - let decrypted_key_bytes = + // we must zeroize decrypted_key_bytes, as this contains sensitive data, + // including private key informations + let decrypted_key_bytes = Hidden::hide( decrypt_bytes_integral_nonce(cipher, b"wallet_setting_tor_id".to_vec(), from_hex(&key_str)?) - .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?; + .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?, + ); - bincode::deserialize(&decrypted_key_bytes) + bincode::deserialize(decrypted_key_bytes.reveal()) .map_err(|e| WalletStorageError::ConversionError(e.to_string()))? }, }; @@ -465,10 +475,10 @@ impl WalletBackend for WalletSqliteDatabase { Some(sk) => sk, }; - let master_seed_bytes = from_hex(master_seed_str.as_str())?; + let master_seed_bytes = Hidden::hide(from_hex(master_seed_str.as_str())?); // Sanity check that the decrypted bytes are a valid CipherSeed - let _master_seed = CipherSeed::from_enciphered_bytes(&master_seed_bytes, None)?; + let _master_seed = CipherSeed::from_enciphered_bytes(master_seed_bytes.reveal(), None)?; let ciphertext_integral_nonce = encrypt_bytes_integral_nonce(&cipher, b"wallet_setting_master_seed".to_vec(), master_seed_bytes) .map_err(|e| WalletStorageError::AeadError(format!("Encryption Error:{}", e)))?; @@ -486,7 +496,8 @@ impl WalletBackend for WalletSqliteDatabase { let tor_id = WalletSettingSql::get(DbKey::TorId.to_string(), &conn)?; if let Some(v) = tor_id { let tor = TorIdentity::from_json(&v).map_err(|e| WalletStorageError::ConversionError(e.to_string()))?; - let bytes = bincode::serialize(&tor).map_err(|e| WalletStorageError::ConversionError(e.to_string()))?; + let bytes = + Hidden::hide(bincode::serialize(&tor).map_err(|e| WalletStorageError::ConversionError(e.to_string()))?); let ciphertext_integral_nonce = encrypt_bytes_integral_nonce(&cipher, b"wallet_setting_tor_id".to_vec(), bytes) .map_err(|e| WalletStorageError::AeadError(format!("Encryption Error:{}", e)))?; @@ -522,16 +533,18 @@ impl WalletBackend for WalletSqliteDatabase { Some(sk) => sk, }; - let master_seed_bytes = decrypt_bytes_integral_nonce( - &cipher, - b"wallet_setting_master_seed".to_vec(), - from_hex(master_seed_str.as_str())?, - ) - .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?; + let master_seed_bytes = Hidden::hide( + decrypt_bytes_integral_nonce( + &cipher, + b"wallet_setting_master_seed".to_vec(), + from_hex(master_seed_str.as_str())?, + ) + .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?, + ); // Sanity check that the decrypted bytes are a valid CipherSeed - let _master_seed = CipherSeed::from_enciphered_bytes(&master_seed_bytes, None)?; - WalletSettingSql::new(DbKey::MasterSeed.to_string(), master_seed_bytes.to_hex()).set(&conn)?; + let _master_seed = CipherSeed::from_enciphered_bytes(master_seed_bytes.reveal(), None)?; + WalletSettingSql::new(DbKey::MasterSeed.to_string(), master_seed_bytes.reveal().to_hex()).set(&conn)?; let _ = WalletSettingSql::clear(DbKey::PassphraseHash.to_string(), &conn)?; let _ = WalletSettingSql::clear(DbKey::EncryptionSalt.to_string(), &conn)?; @@ -547,11 +560,14 @@ impl WalletBackend for WalletSqliteDatabase { // remove tor id encryption if present let key_str = WalletSettingSql::get(DbKey::TorId.to_string(), &conn)?; if let Some(v) = key_str { - let decrypted_key_bytes = + // decrypted_key_bytes contains sensitive information regarding private key, we thus + // make sure the data is appropriately zeroized when we leave the current scope + let decrypted_key_bytes = Hidden::hide( decrypt_bytes_integral_nonce(&cipher, b"wallet_setting_tor_id".to_vec(), from_hex(v.as_str())?) - .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?; + .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?, + ); - let tor_id: TorIdentity = bincode::deserialize(&decrypted_key_bytes) + let tor_id: TorIdentity = bincode::deserialize(decrypted_key_bytes.reveal()) .map_err(|e| WalletStorageError::ConversionError(e.to_string()))?; let tor_string = tor_id @@ -703,20 +719,24 @@ fn check_db_encryption_status( return Err(WalletStorageError::MissingNonce); } - let decrypted_key = + // decrypted key contains sensitive data, we make sure we appropriately zeroize + // the corresponding data buffer, when leaving the current scope + let decrypted_key = Hidden::hide( decrypt_bytes_integral_nonce(&cipher_inner, b"wallet_setting_master_seed".to_vec(), sk_bytes) .map_err(|e| { error!(target: LOG_TARGET, "Incorrect passphrase ({})", e); WalletStorageError::InvalidPassphrase - })?; + })?, + ); - let _cipher_seed = CipherSeed::from_enciphered_bytes(&decrypted_key, None).map_err(|_| { - error!( - target: LOG_TARGET, - "Decrypted Master Secret Key cannot be parsed into a Cipher Seed" - ); - WalletStorageError::InvalidEncryptionCipher - })?; + let _cipher_seed = + CipherSeed::from_enciphered_bytes(decrypted_key.reveal(), None).map_err(|_| { + error!( + target: LOG_TARGET, + "Decrypted Master Secret Key cannot be parsed into a Cipher Seed" + ); + WalletStorageError::InvalidEncryptionCipher + })?; } else { error!( target: LOG_TARGET, @@ -832,15 +852,19 @@ impl Encryptable for ClientKeyValueSql { #[allow(unused_assignments)] fn encrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> { - self.value = - encrypt_bytes_integral_nonce(cipher, self.domain("value"), self.value.as_bytes().to_vec())?.to_hex(); + self.value = encrypt_bytes_integral_nonce( + cipher, + self.domain("value"), + Hidden::hide(self.value.as_bytes().to_vec()), + )? + .to_hex(); Ok(()) } #[allow(unused_assignments)] fn decrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> { - let decrypted_value = decrypt_bytes_integral_nonce( + let mut decrypted_value = decrypt_bytes_integral_nonce( cipher, self.domain("value"), from_hex(self.value.as_str()).map_err(|e| e.to_string())?, @@ -850,6 +874,9 @@ impl Encryptable for ClientKeyValueSql { .map_err(|e| e.to_string())? .to_string(); + // we zeroize the decrypted value + decrypted_value.zeroize(); + Ok(()) } } diff --git a/base_layer/wallet/src/transaction_service/storage/sqlite_db.rs b/base_layer/wallet/src/transaction_service/storage/sqlite_db.rs index 1efb172da0..5366e8bcc6 100644 --- a/base_layer/wallet/src/transaction_service/storage/sqlite_db.rs +++ b/base_layer/wallet/src/transaction_service/storage/sqlite_db.rs @@ -46,9 +46,11 @@ use tari_core::transactions::tari_amount::MicroTari; use tari_utilities::{ hex::{from_hex, Hex}, ByteArray, + Hidden, }; use thiserror::Error; use tokio::time::Instant; +use zeroize::Zeroize; use crate::{ schema::{completed_transactions, inbound_transactions, outbound_transactions}, @@ -1537,7 +1539,7 @@ impl Encryptable for InboundTransactionSql { self.receiver_protocol = encrypt_bytes_integral_nonce( cipher, self.domain("receiver_protocol"), - self.receiver_protocol.as_bytes().to_vec(), + Hidden::hide(self.receiver_protocol.as_bytes().to_vec()), )? .to_hex(); @@ -1545,7 +1547,7 @@ impl Encryptable for InboundTransactionSql { } fn decrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> { - let decrypted_protocol = decrypt_bytes_integral_nonce( + let mut decrypted_protocol = decrypt_bytes_integral_nonce( cipher, self.domain("receiver_protocol"), from_hex(self.receiver_protocol.as_str()).map_err(|e| e.to_string())?, @@ -1555,6 +1557,9 @@ impl Encryptable for InboundTransactionSql { .map_err(|e| e.to_string())? .to_string(); + // zeroize the decrypted protocol data buffer + decrypted_protocol.zeroize(); + Ok(()) } } @@ -1788,7 +1793,7 @@ impl Encryptable for OutboundTransactionSql { self.sender_protocol = encrypt_bytes_integral_nonce( cipher, self.domain("sender_protocol"), - self.sender_protocol.as_bytes().to_vec(), + Hidden::hide(self.sender_protocol.as_bytes().to_vec()), )? .to_hex(); @@ -1796,7 +1801,7 @@ impl Encryptable for OutboundTransactionSql { } fn decrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> { - let decrypted_protocol = decrypt_bytes_integral_nonce( + let mut decrypted_protocol = decrypt_bytes_integral_nonce( cipher, self.domain("sender_protocol"), from_hex(self.sender_protocol.as_str()).map_err(|e| e.to_string())?, @@ -1806,6 +1811,9 @@ impl Encryptable for OutboundTransactionSql { .map_err(|e| e.to_string())? .to_string(); + // zeroize the decrypted protocol data buffer + decrypted_protocol.zeroize(); + Ok(()) } } @@ -2194,7 +2202,7 @@ impl Encryptable for CompletedTransactionSql { self.transaction_protocol = encrypt_bytes_integral_nonce( cipher, self.domain("transaction_protocol"), - self.transaction_protocol.as_bytes().to_vec(), + Hidden::hide(self.transaction_protocol.as_bytes().to_vec()), )? .to_hex(); @@ -2202,7 +2210,7 @@ impl Encryptable for CompletedTransactionSql { } fn decrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> { - let decrypted_protocol = decrypt_bytes_integral_nonce( + let mut decrypted_protocol = decrypt_bytes_integral_nonce( cipher, self.domain("transaction_protocol"), from_hex(self.transaction_protocol.as_str()).map_err(|e| e.to_string())?, @@ -2212,6 +2220,9 @@ impl Encryptable for CompletedTransactionSql { .map_err(|e| e.to_string())? .to_string(); + // zeroize the decrypted protocol data buffer + decrypted_protocol.zeroize(); + Ok(()) } } diff --git a/base_layer/wallet/src/util/encryption.rs b/base_layer/wallet/src/util/encryption.rs index 915fa163e1..3c933dffdd 100644 --- a/base_layer/wallet/src/util/encryption.rs +++ b/base_layer/wallet/src/util/encryption.rs @@ -29,7 +29,7 @@ use chacha20poly1305::{ XNonce, }; use rand::{rngs::OsRng, RngCore}; -use tari_utilities::ByteArray; +use tari_utilities::{ByteArray, Hidden}; pub trait Encryptable { const KEY_MANAGER: &'static [u8] = b"KEY_MANAGER"; @@ -77,7 +77,7 @@ pub fn decrypt_bytes_integral_nonce( pub fn encrypt_bytes_integral_nonce( cipher: &XChaCha20Poly1305, domain: Vec, - plaintext: Vec, + plaintext: Hidden>, ) -> Result, String> { // Produce a secure random nonce let mut nonce = [0u8; size_of::()]; @@ -86,8 +86,8 @@ pub fn encrypt_bytes_integral_nonce( // Bind the domain as additional data let payload = Payload { - msg: plaintext.as_bytes(), - aad: domain.as_bytes(), + msg: plaintext.reveal(), + aad: domain.as_slice(), }; // Attempt authenticated encryption @@ -106,7 +106,7 @@ mod test { use chacha20poly1305::{aead::NewAead, Key, Tag, XChaCha20Poly1305, XNonce}; use rand::{rngs::OsRng, RngCore}; - use tari_utilities::ByteArray; + use tari_utilities::{ByteArray, Hidden}; use crate::util::encryption::{decrypt_bytes_integral_nonce, encrypt_bytes_integral_nonce}; @@ -119,7 +119,8 @@ mod test { let key_ga = Key::from_slice(&key); let cipher = XChaCha20Poly1305::new(key_ga); - let ciphertext = encrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), plaintext.clone()).unwrap(); + let ciphertext = + encrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), Hidden::hide(plaintext.clone())).unwrap(); // Check the ciphertext size, which we rely on for later tests // It should extend the plaintext size by the nonce and tag sizes