Fill BlindingFactor with zeros on Drop (#2847)

* Implement simple zeroing of BlindingFactor in Drop

* rustfmt

* Make Debug implementation for BlindingFactor empty

* Implement BlindingFactor zeroing unit test

* mnemonic.rs: fix deprecated warning in test_bip39_random test

* Use zeroize crate to clear BlindingFactor

* Fix comment and implement dummy Debug trait for BlindingFactor

* Fix formatter

Cargo.lock

@@ -798,6 +798,7 @@ dependencies = [
  "serde_json 1.0.39 (registry+https://github.com/rust-lang/crates.io-index)",
  "sha2 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "uuid 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)",
+ "zeroize 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -2631,6 +2632,25 @@ name = "zeroize"
 version = "0.5.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 
+[[package]]
+name = "zeroize"
+version = "0.8.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+dependencies = [
+ "zeroize_derive 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)",
+]
+
+[[package]]
+name = "zeroize_derive"
+version = "0.8.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+dependencies = [
+ "proc-macro2 0.4.27 (registry+https://github.com/rust-lang/crates.io-index)",
+ "quote 0.6.12 (registry+https://github.com/rust-lang/crates.io-index)",
+ "syn 0.15.31 (registry+https://github.com/rust-lang/crates.io-index)",
+ "synstructure 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)",
+]
+
 [[package]]
 name = "zip"
 version = "0.4.2"
@@ -2916,4 +2936,6 @@ dependencies = [
 "checksum yaml-rust 0.3.5 (registry+https://github.com/rust-lang/crates.io-index)" = "e66366e18dc58b46801afbf2ca7661a9f59cc8c5962c29892b6039b4f86fa992"
 "checksum yaml-rust 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)" = "65923dd1784f44da1d2c3dbbc5e822045628c590ba72123e1c73d3c230c4434d"
 "checksum zeroize 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)" = "8ddfeb6eee2fb3b262ef6e0898a52b7563bb8e0d5955a313b3cf2f808246ea14"
+"checksum zeroize 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "b60a6c572b91d8ecb0a460950d84fe5b40699edd07d65f73789b31237afc8f66"
+"checksum zeroize_derive 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "9dac4b660d969bff9c3fe1847a891cacaa8b21dd5f2aae6e0a3e0975aea96431"
 "checksum zip 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)" = "36b9e08fb518a65cf7e08a1e482573eb87a2f4f8c6619316612a3c1f162fe822"

core/src/core/block.rs

@@ -384,7 +384,7 @@ impl BlockHeader {
 
 	/// Total kernel offset for the chain state up to and including this block.
 	pub fn total_kernel_offset(&self) -> BlindingFactor {
-		self.total_kernel_offset
+		self.total_kernel_offset.clone()
 	}
 }
 
@@ -560,8 +560,10 @@ impl Block {
 			.with_kernel(reward_kern);
 
 		// Now add the kernel offset of the previous block for a total
-		let total_kernel_offset =
-			committed::sum_kernel_offsets(vec![agg_tx.offset, prev.total_kernel_offset], vec![])?;
+		let total_kernel_offset = committed::sum_kernel_offsets(
+			vec![agg_tx.offset.clone(), prev.total_kernel_offset.clone()],
+			vec![],
+		)?;
 
 		let now = Utc::now().timestamp();
 		let timestamp = DateTime::<Utc>::from_utc(NaiveDateTime::from_timestamp(now, 0), Utc);
@@ -698,7 +700,7 @@ impl Block {
 		// verify.body.outputs and kernel sums
 		let (_utxo_sum, kernel_sum) = self.verify_kernel_sums(
 			self.header.overage(),
-			self.block_kernel_offset(*prev_kernel_offset)?,
+			self.block_kernel_offset(prev_kernel_offset.clone())?,
 		)?;
 
 		Ok(kernel_sum)

core/src/core/transaction.rs

@@ -957,7 +957,7 @@ impl Transaction {
 	) -> Result<(), Error> {
 		self.body.validate(weighting, verifier)?;
 		self.body.verify_features()?;
-		self.verify_kernel_sums(self.overage(), self.offset)?;
+		self.verify_kernel_sums(self.overage(), self.offset.clone())?;
 		Ok(())
 	}
 

core/src/libtx/build.rs

@@ -158,7 +158,7 @@ where
 {
 	Box::new(
 		move |_build, (tx, kern, sum)| -> (Transaction, TxKernel, BlindSum) {
-			(tx.with_offset(offset), kern, sum)
+			(tx.with_offset(offset.clone()), kern, sum)
 		},
 	)
 }

keychain/Cargo.toml

@@ -19,6 +19,7 @@ serde_derive = "1"
 serde_json = "1"
 uuid = { version = "0.6", features = ["serde", "v4"] }
 lazy_static = "1"
+zeroize = "0.8.0"
 
 digest = "0.7"
 hmac = "0.6"

keychain/src/mnemonic.rs

@@ -324,10 +324,11 @@ mod tests {
 
 	#[test]
 	fn test_bip39_random() {
+		use rand::seq::SliceRandom;
 		let sizes: [usize; 5] = [16, 20, 24, 28, 32];
 
 		let mut rng = thread_rng();
-		let size = *rng.choose(&sizes).unwrap();
+		let size = *sizes.choose(&mut rng).unwrap();
 		let mut entropy: Vec<u8> = Vec::with_capacity(size);
 
 		for _ in 0..size {

keychain/src/types.rs

@@ -32,6 +32,7 @@ use crate::util::secp::key::{PublicKey, SecretKey};
 use crate::util::secp::pedersen::Commitment;
 use crate::util::secp::{self, Message, Secp256k1, Signature};
 use crate::util::static_secp_instance;
+use zeroize::Zeroize;
 
 use byteorder::{BigEndian, ReadBytesExt, WriteBytesExt};
 
@@ -227,12 +228,13 @@ impl fmt::Display for Identifier {
 	}
 }
 
-#[derive(Clone, Copy, PartialEq, Serialize, Deserialize)]
+#[derive(Default, Clone, PartialEq, Serialize, Deserialize, Zeroize)]
 pub struct BlindingFactor([u8; SECRET_KEY_SIZE]);
 
+// Dummy `Debug` implementation that prevents secret leakage.
 impl fmt::Debug for BlindingFactor {
 	fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> fmt::Result {
-		write!(f, "{}", self.to_hex())
+		write!(f, "BlindingFactor(<secret key hidden>)")
 	}
 }
 
@@ -480,8 +482,38 @@ mod test {
 	use rand::thread_rng;
 
 	use crate::types::{BlindingFactor, ExtKeychainPath, Identifier};
+	use crate::util::secp::constants::SECRET_KEY_SIZE;
 	use crate::util::secp::key::{SecretKey, ZERO_KEY};
 	use crate::util::secp::Secp256k1;
+	use std::slice::from_raw_parts;
+
+	// This tests cleaning of BlindingFactor (e.g. secret key) on Drop.
+	// To make this test fail, just remove `Zeroize` derive from `BlindingFactor` definition.
+	#[test]
+	fn blinding_factor_clear_on_drop() {
+		// Create buffer for blinding factor filled with non-zero bytes.
+		let bf_bytes = [0xAA; SECRET_KEY_SIZE];
+		let ptr = {
+			// Fill blinding factor with some "sensitive" data
+			let bf = BlindingFactor::from_slice(&bf_bytes[..]);
+			bf.0.as_ptr()
+
+			// -- after this line BlindingFactor should be zeroed
+		};
+
+		// Unsafely get data from where BlindingFactor was in memory. Should be all zeros.
+		let bf_bytes = unsafe { from_raw_parts(ptr, SECRET_KEY_SIZE) };
+
+		// There should be all zeroes.
+		let mut all_zeros = true;
+		for b in bf_bytes {
+			if *b != 0x00 {
+				all_zeros = false;
+			}
+		}
+
+		assert!(all_zeros)
+	}
 
 	#[test]
 	fn split_blinding_factor() {

pool/src/pool.rs

@@ -268,7 +268,7 @@ impl Pool {
 		header: &BlockHeader,
 	) -> Result<BlockSums, PoolError> {
 		let overage = tx.overage();
-		let offset = (header.total_kernel_offset() + tx.offset)?;
+		let offset = (header.total_kernel_offset() + tx.offset.clone())?;
 
 		let block_sums = self.blockchain.get_block_sums(&header.hash())?;