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: minimize potential memory leaks of sensitive data on the wallet code #4953

Merged
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

Large diffs are not rendered by default.

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
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