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.

Sign up for GitHub

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
merged 11 commits into from
Nov 28, 2022
Merged

fix: minimize potential memory leaks of sensitive data on the wallet code #4953

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
041d74f
first commit
jorgeantonio21 Nov 24, 2022
105d659
Merge branch 'development' into ja-hide-wallet-sql-outputs
jorgeantonio21 Nov 24, 2022
9e1385f
remove encryption
jorgeantonio21 Nov 24, 2022
416225d
remove all instances of decryption/encryption of output sql
jorgeantonio21 Nov 24, 2022
d60aa6e
compiler checks
jorgeantonio21 Nov 24, 2022
61a0a5a
add zeroize to plaintext in encrypt_bytes_integral_nonce
jorgeantonio21 Nov 24, 2022
37bd39a
encrypt/decrypt with hidden/zeroize data guarantees
jorgeantonio21 Nov 24, 2022
e8436a1
remove unnecessary zeroize
jorgeantonio21 Nov 24, 2022
ee84521
merge development and address conflicts + cargo clippy
jorgeantonio21 Nov 25, 2022
411c55e
Merge branch 'development' into ja-hide-wallet-sql-outputs
jorgeantonio21 Nov 25, 2022
e7dba7e
Merge branch 'development' into ja-hide-wallet-sql-outputs
jorgeantonio21 Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs
View file
Open in desktop
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
513 changes: 157 additions & 356 deletions base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs
View file
Open in desktop

Large diffs are not rendered by default.

23 changes: 17 additions & 6 deletions base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,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 @@ -75,8 +75,9 @@ impl NewOutputSql {
status: OutputStatus,
received_in_tx_id: Option<TxId>,
coinbase_block_height: Option<u64>,
cipher: Option<&XChaCha20Poly1305>,
) -> Result<Self, OutputManagerStorageError> {
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 @@ -109,7 +110,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 @@ -128,13 +136,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ use tari_core::{
};
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 @@ -609,35 +611,25 @@ 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(|e| OutputManagerStorageError::AeadError(e))?;
}

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 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 @@ -647,9 +639,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 @@ -658,7 +650,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 @@ -668,7 +660,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 @@ -677,7 +669,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 @@ -686,7 +678,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 @@ -695,7 +687,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 @@ -704,7 +696,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 @@ -714,8 +706,8 @@ impl TryFrom<OutputSql> for DbUnblindedOutput {
}
})?,
),
o.script_lock_height as u64,
Covenant::from_bytes(&o.covenant).map_err(|e| {
self.script_lock_height as u64,
Covenant::from_bytes(&self.covenant).map_err(|e| {
error!(
target: LOG_TARGET,
"Could not create Covenant from stored bytes ({}), They might be encrypted", e
Expand All @@ -725,10 +717,14 @@ impl TryFrom<OutputSql> for DbUnblindedOutput {
}
})?,
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 @@ -743,7 +739,7 @@ impl TryFrom<OutputSql> for DbUnblindedOutput {
},
},
};
let commitment = match o.commitment {
let commitment = match self.commitment {
None => {
let factories = CryptoFactories::default();
factories
Expand All @@ -752,34 +748,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 @@ -793,13 +789,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 @@ -818,9 +817,3 @@ impl Encryptable<XChaCha20Poly1305> for OutputSql {
Ok(())
}
}

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