Skip to content

Commit

Permalink
fix: minimize potential memory leaks of sensitive data on the wallet …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@jorgeantonio21
jorgeantonio21 committed Nov 28, 2022
1 parent baa196f commit e364994
Show file tree
Hide file tree
Showing 7 changed files with 322 additions and 473 deletions.
15 changes: 11 additions & 4 deletions base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -158,8 +159,11 @@ impl Encryptable<XChaCha20Poly1305> 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(())
}
Expand All @@ -180,8 +184,11 @@ impl Encryptable<XChaCha20Poly1305> 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(())
}
Expand Down
510 changes: 154 additions & 356 deletions base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs
View file

Large diffs are not rendered by default.

24 changes: 18 additions & 6 deletions base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -77,10 +77,12 @@ impl NewOutputSql {
status: OutputStatus,
received_in_tx_id: Option<TxId>,
coinbase_block_height: Option<u64>,
cipher: Option<&XChaCha20Poly1305>,
) -> Result<Self, OutputManagerStorageError> {
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,
Expand Down Expand Up @@ -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
Expand All @@ -132,13 +141,16 @@ impl Encryptable<XChaCha20Poly1305> 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(())
Expand Down
105 changes: 49 additions & 56 deletions base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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<OutputSql> for DbUnblindedOutput {
type Error = OutputManagerStorageError;

#[allow(clippy::too_many_lines)]
fn try_from(o: OutputSql) -> Result<Self, Self::Error> {
pub fn to_db_unblinded_output(
mut self,
cipher: Option<&XChaCha20Poly1305>,
) -> Result<DbUnblindedOutput, OutputManagerStorageError> {
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
Expand All @@ -642,9 +632,11 @@ impl TryFrom<OutputSql> 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"
Expand All @@ -654,9 +646,9 @@ impl TryFrom<OutputSql> 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"
Expand All @@ -665,7 +657,7 @@ impl TryFrom<OutputSql> 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"
Expand All @@ -675,7 +667,7 @@ impl TryFrom<OutputSql> 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"
Expand All @@ -684,7 +676,7 @@ impl TryFrom<OutputSql> 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"
Expand All @@ -693,7 +685,7 @@ impl TryFrom<OutputSql> 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"
Expand All @@ -702,7 +694,7 @@ impl TryFrom<OutputSql> 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"
Expand All @@ -711,7 +703,7 @@ impl TryFrom<OutputSql> 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"
Expand All @@ -721,13 +713,17 @@ impl TryFrom<OutputSql> 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()
Expand All @@ -742,7 +738,7 @@ impl TryFrom<OutputSql> for DbUnblindedOutput {
},
},
};
let commitment = match o.commitment {
let commitment = match self.commitment {
None => {
let factories = CryptoFactories::default();
factories
Expand All @@ -751,34 +747,34 @@ impl TryFrom<OutputSql> 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()?,
})
}
}
Expand All @@ -792,13 +788,16 @@ impl Encryptable<XChaCha20Poly1305> 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(())
Expand All @@ -817,9 +816,3 @@ impl Encryptable<XChaCha20Poly1305> for OutputSql {
Ok(())
}
}

// impl PartialEq<NewOutputSql> for OutputSql {
// fn eq(&self, other: &NewOutputSql) -> bool {
// &NewOutputSql::from(self.clone()) == other
// }
// }
Loading

0 comments on commit e364994

Please sign in to comment.