From 790e89902a845b5e3f3a8694a365b2579294760e Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sun, 22 May 2022 01:56:17 -0400 Subject: [PATCH] Override Monero's random function with a Rust-seedable random Closes https://github.com/serai-dex/serai/issues/2. Also finishes the implementation of https://github.com/monero-project/research-lab/issues/103. --- coins/monero/build.rs | 4 +- coins/monero/c/{wrapper.c => wrapper.cpp} | 43 +++++++- coins/monero/src/bulletproofs.rs | 31 ++++-- coins/monero/src/wallet/mod.rs | 4 + coins/monero/src/wallet/scan.rs | 2 - coins/monero/src/wallet/send/mod.rs | 37 ++----- coins/monero/src/wallet/send/multisig.rs | 128 ++++++++++++---------- 7 files changed, 150 insertions(+), 99 deletions(-) rename coins/monero/c/{wrapper.c => wrapper.cpp} (60%) diff --git a/coins/monero/build.rs b/coins/monero/build.rs index ddb83fb7..807e80ef 100644 --- a/coins/monero/build.rs +++ b/coins/monero/build.rs @@ -65,11 +65,11 @@ fn main() { } } - println!("cargo:rerun-if-changed=c/wrapper.c"); + println!("cargo:rerun-if-changed=c/wrapper.cpp"); if !Command::new("g++").args(&[ "-O3", "-Wall", "-shared", "-std=c++14", "-fPIC", "-Imonero/contrib/epee/include", "-Imonero/src", - "wrapper.c", "-o", &format!( + "wrapper.cpp", "-o", &format!( "{}/{}wrapper.{}", out_dir, &env::consts::DLL_PREFIX, diff --git a/coins/monero/c/wrapper.c b/coins/monero/c/wrapper.cpp similarity index 60% rename from coins/monero/c/wrapper.c rename to coins/monero/c/wrapper.cpp index 02392bb8..69b534a0 100644 --- a/coins/monero/c/wrapper.c +++ b/coins/monero/c/wrapper.cpp @@ -1,9 +1,37 @@ +#include + #include "device/device_default.hpp" #include "ringct/bulletproofs.h" #include "ringct/rctSigs.h" +std::mutex rng_mutex; +char rng_entropy[64]; +void rng(uint8_t* seed) { + memcpy(rng_entropy, seed, 32); + memset(&rng_entropy[32], 0, 32); +} + extern "C" { + void generate_random_bytes_not_thread_safe(size_t n, uint8_t* value) { + size_t written = 0; + while (written != n) { + uint8_t hash[32]; + crypto::cn_fast_hash(rng_entropy, 64, (char*) hash); + // Step the RNG by setting the latter half to the most recent result + // Does not leak the RNG, even if the values are leaked (which they are expected to be) due to + // the first half remaining constant and undisclosed + memcpy(&rng_entropy[32], hash, 32); + + size_t next = n - written; + if (next > 32) { + next = 32; + } + memcpy(&value[written], hash, next); + written += next; + } + } + void c_hash_to_point(uint8_t* point) { rct::key key_point; ge_p3 e_p3; @@ -12,7 +40,10 @@ extern "C" { ge_p3_tobytes(point, &e_p3); } - uint8_t* c_generate_bp(uint8_t len, uint64_t* a, uint8_t* m) { + uint8_t* c_generate_bp(uint8_t* seed, uint8_t len, uint64_t* a, uint8_t* m) { + std::lock_guard guard(rng_mutex); + rng(seed); + rct::keyV masks; std::vector amounts; masks.resize(len); @@ -21,6 +52,7 @@ extern "C" { memcpy(masks[i].bytes, m + (i * 32), 32); amounts[i] = a[i]; } + rct::Bulletproof bp = rct::bulletproof_PROVE(amounts, masks); std::stringstream ss; @@ -33,7 +65,14 @@ extern "C" { return res; } - bool c_verify_bp(uint s_len, uint8_t* s, uint8_t c_len, uint8_t* c) { + bool c_verify_bp(uint8_t* seed, uint s_len, uint8_t* s, uint8_t c_len, uint8_t* c) { + // BPs are batch verified which use RNG based challenges to ensure individual integrity + // That's why this must also have control over RNG, to prevent interrupting multisig signing + // while not using known seeds. Considering this doesn't actually define a batch, + // and it's only verifying a single BP, it'd probably be fine, but... + std::lock_guard guard(rng_mutex); + rng(seed); + rct::Bulletproof bp; std::stringstream ss; std::string str; diff --git a/coins/monero/src/bulletproofs.rs b/coins/monero/src/bulletproofs.rs index caf96534..300f91fc 100644 --- a/coins/monero/src/bulletproofs.rs +++ b/coins/monero/src/bulletproofs.rs @@ -1,5 +1,7 @@ #![allow(non_snake_case)] +use rand_core::{RngCore, CryptoRng}; + use curve25519_dalek::{scalar::Scalar, edwards::EdwardsPoint}; use crate::{Commitment, wallet::TransactionError, serialize::*}; @@ -20,47 +22,56 @@ pub struct Bulletproofs { } impl Bulletproofs { - pub fn new(outputs: &[Commitment]) -> Result { + pub fn new(rng: &mut R, outputs: &[Commitment]) -> Result { if outputs.len() > 16 { return Err(TransactionError::TooManyOutputs)?; } - let masks: Vec<[u8; 32]> = outputs.iter().map(|commitment| commitment.mask.to_bytes()).collect(); - let amounts: Vec = outputs.iter().map(|commitment| commitment.amount).collect(); - let res: Bulletproofs; + let mut seed = [0; 32]; + rng.fill_bytes(&mut seed); + + let masks = outputs.iter().map(|commitment| commitment.mask.to_bytes()).collect::>(); + let amounts = outputs.iter().map(|commitment| commitment.amount).collect::>(); + + let res; unsafe { #[link(name = "wrapper")] extern "C" { fn free(ptr: *const u8); - fn c_generate_bp(len: u8, amounts: *const u64, masks: *const [u8; 32]) -> *const u8; + fn c_generate_bp(seed: *const u8, len: u8, amounts: *const u64, masks: *const [u8; 32]) -> *const u8; } - let ptr = c_generate_bp(outputs.len() as u8, amounts.as_ptr(), masks.as_ptr()); + let ptr = c_generate_bp(seed.as_ptr(), outputs.len() as u8, amounts.as_ptr(), masks.as_ptr()); let len = ((ptr.read() as usize) << 8) + (ptr.add(1).read() as usize); res = Bulletproofs::deserialize( // Wrap in a cursor to provide a mutable Reader &mut std::io::Cursor::new(std::slice::from_raw_parts(ptr.add(2), len)) ).expect("Couldn't deserialize Bulletproofs from Monero"); free(ptr); - } + }; - Ok(res.into()) + Ok(res) } - pub fn verify(&self, commitments: &[EdwardsPoint]) -> bool { + pub fn verify(&self, rng: &mut R, commitments: &[EdwardsPoint]) -> bool { if commitments.len() > 16 { return false; } + let mut seed = [0; 32]; + rng.fill_bytes(&mut seed); + let mut serialized = Vec::with_capacity((9 + (2 * self.L.len())) * 32); self.serialize(&mut serialized).unwrap(); let commitments: Vec<[u8; 32]> = commitments.iter().map( |commitment| (commitment * Scalar::from(8 as u8).invert()).compress().to_bytes() ).collect(); + unsafe { #[link(name = "wrapper")] extern "C" { fn c_verify_bp( + seed: *const u8, serialized_len: usize, serialized: *const u8, commitments_len: u8, @@ -68,7 +79,7 @@ impl Bulletproofs { ) -> bool; } - c_verify_bp(serialized.len(), serialized.as_ptr(), commitments.len() as u8, commitments.as_ptr()) + c_verify_bp(seed.as_ptr(), serialized.len(), serialized.as_ptr(), commitments.len() as u8, commitments.as_ptr()) } } diff --git a/coins/monero/src/wallet/mod.rs b/coins/monero/src/wallet/mod.rs index 8992ad91..88d4ff61 100644 --- a/coins/monero/src/wallet/mod.rs +++ b/coins/monero/src/wallet/mod.rs @@ -15,6 +15,10 @@ pub(crate) use decoys::Decoys; mod send; pub use send::{TransactionError, SignableTransaction}; +fn key_image_sort(x: &EdwardsPoint, y: &EdwardsPoint) -> std::cmp::Ordering { + x.compress().to_bytes().cmp(&y.compress().to_bytes()).reverse() +} + // https://github.com/monero-project/research-lab/issues/103 pub(crate) fn uniqueness(inputs: &[Input]) -> [u8; 32] { let mut u = b"domain_separator".to_vec(); diff --git a/coins/monero/src/wallet/scan.rs b/coins/monero/src/wallet/scan.rs index 0cfcfb52..a4e36748 100644 --- a/coins/monero/src/wallet/scan.rs +++ b/coins/monero/src/wallet/scan.rs @@ -24,8 +24,6 @@ pub struct SpendableOutput { pub commitment: Commitment } -// TODO: Enable disabling one of the shared key derivations and solely using one -// Change outputs currently always use unique derivations, so that must also be corrected impl Transaction { pub fn scan( &self, diff --git a/coins/monero/src/wallet/send/mod.rs b/coins/monero/src/wallet/send/mod.rs index e1462c4e..bddb7117 100644 --- a/coins/monero/src/wallet/send/mod.rs +++ b/coins/monero/src/wallet/send/mod.rs @@ -24,7 +24,7 @@ use crate::{ generate_key_image, bulletproofs::Bulletproofs, clsag::{ClsagError, ClsagInput, Clsag}, rpc::{Rpc, RpcError}, transaction::*, - wallet::{uniqueness, shared_key, commitment_mask, amount_encryption, SpendableOutput, Decoys} + wallet::{SpendableOutput, Decoys, key_image_sort, uniqueness, shared_key, commitment_mask, amount_encryption} }; #[cfg(feature = "multisig")] use crate::frost::MultisigError; @@ -185,7 +185,7 @@ impl SignableTransaction { fn prepare_outputs( &mut self, rng: &mut R, - uniqueness: Option<[u8; 32]> + uniqueness: [u8; 32] ) -> Result<(Vec, Scalar), TransactionError> { self.fee = self.fee_per_byte * 2000; // TODO @@ -203,20 +203,7 @@ impl SignableTransaction { for payment in &self.payments { temp_outputs.push((None, (payment.0, payment.1))); } - // Ideally, the change output would always have uniqueness, as we control this wallet software - // Unfortunately, if this is used with multisig, doing so would add an extra round due to the - // fact Bulletproofs use a leader protocol reliant on this shared key before the first round of - // communication. Making the change output unique would require Bulletproofs not be a leader - // protocol, using a seeded random - // There is a vector where the multisig participants leak the output key they're about to send - // to, and someone could use that key, forcing some funds to be burnt accordingly if they win - // the race. Any multisig wallet, with this current setup, must only keep change keys in context - // accordingly, preferably as soon as they are proposed, even before they appear as confirmed - // Using another source of uniqueness would also be possible, yet it'd make scanning a tri-key - // system (currently dual for the simpler API, yet would be dual even with a more complex API - // under this decision) - // TODO after https://github.com/serai-dex/serai/issues/2 - temp_outputs.push((uniqueness, (self.change, in_amount - out_amount))); + temp_outputs.push((Some(uniqueness), (self.change, in_amount - out_amount))); // Shuffle the outputs temp_outputs.shuffle(rng); @@ -293,22 +280,20 @@ impl SignableTransaction { for input in &self.inputs { images.push(generate_key_image(&(spend + input.key_offset))); } - images.sort_by(|x, y| x.compress().to_bytes().cmp(&y.compress().to_bytes()).reverse()); + images.sort_by(key_image_sort); let (commitments, mask_sum) = self.prepare_outputs( rng, - Some( - uniqueness( - &images.iter().map(|image| Input::ToKey { - amount: 0, - key_offsets: vec![], - key_image: *image - }).collect::>() - ) + uniqueness( + &images.iter().map(|image| Input::ToKey { + amount: 0, + key_offsets: vec![], + key_image: *image + }).collect::>() ) )?; - let mut tx = self.prepare_transaction(&commitments, Bulletproofs::new(&commitments)?); + let mut tx = self.prepare_transaction(&commitments, Bulletproofs::new(rng, &commitments)?); let signable = prepare_inputs(rng, rpc, &self.inputs, spend, &mut tx).await?; diff --git a/coins/monero/src/wallet/send/multisig.rs b/coins/monero/src/wallet/send/multisig.rs index 3479f12e..6e2d57f3 100644 --- a/coins/monero/src/wallet/send/multisig.rs +++ b/coins/monero/src/wallet/send/multisig.rs @@ -13,16 +13,18 @@ use crate::{ random_scalar, bulletproofs::Bulletproofs, clsag::{ClsagInput, ClsagDetails, ClsagMultisig}, rpc::Rpc, transaction::{Input, RctPrunable, Transaction}, - wallet::{TransactionError, SignableTransaction, Decoys} + wallet::{TransactionError, SignableTransaction, Decoys, key_image_sort, uniqueness} }; pub struct TransactionMachine { - leader: bool, signable: SignableTransaction, + i: usize, transcript: Transcript, decoys: Vec, + our_preprocess: Vec, + images: Vec, output_masks: Option, inputs: Vec>>>, @@ -55,6 +57,10 @@ impl SignableTransaction { // depending on how these transactions are coordinated let mut transcript = Transcript::new(label); + // Include the height we're using for our data + // The data itself will be included, making this unnecessary, yet a lot of this is technically + // unnecessary. Anything which further increases security at almost no cost should be followed + transcript.append_message(b"height", &u64::try_from(height).unwrap().to_le_bytes()); // Also include the spend_key as below only the key offset is included, so this confirms the sum product // Useful as confirming the sum product confirms the key image, further guaranteeing the one time // properties noted below @@ -76,10 +82,12 @@ impl SignableTransaction { // Select decoys // Ideally, this would be done post entropy, instead of now, yet doing so would require sign - // to be async which isn't feasible. This should be suitably competent though + // to be async which isn't preferable. This should be suitably competent though // While this inability means we can immediately create the input, moving it out of the // Rc RefCell, keeping it within an Rc RefCell keeps our options flexible let decoys = Decoys::select( + // Using a seeded RNG with a specific height, committed to above, should make these decoys + // committed to. They'll also be committed to later via the TX message as a whole &mut ChaCha12Rng::from_seed(transcript.rng_seed(b"decoys", None)), rpc, height, @@ -100,15 +108,17 @@ impl SignableTransaction { } // Verify these outputs by a dummy prep - self.prepare_outputs(rng, None)?; + self.prepare_outputs(rng, [0; 32])?; Ok(TransactionMachine { - leader: keys.params().i() == included[0], signable: self, + i: keys.params().i(), transcript, decoys, + our_preprocess: vec![], + images, output_masks: None, inputs, @@ -135,26 +145,19 @@ impl StateMachine for TransactionMachine { for (i, clsag) in self.clsags.iter_mut().enumerate() { let preprocess = clsag.preprocess(rng)?; // First 64 bytes are FROST's commitments - self.images[i] += CompressedEdwardsY(preprocess[64 .. 96].try_into().unwrap()).decompress().unwrap(); + self.images[i] = CompressedEdwardsY(preprocess[64 .. 96].try_into().unwrap()).decompress().unwrap(); serialized.extend(&preprocess); } + self.our_preprocess = serialized.clone(); - if self.leader { - let mut entropy = [0; 32]; - rng.fill_bytes(&mut entropy); - serialized.extend(&entropy); - - let mut rng = ChaCha12Rng::from_seed(self.transcript.rng_seed(b"tx_keys", Some(entropy))); - // Safe to unwrap thanks to the dummy prepare - let (commitments, output_masks) = self.signable.prepare_outputs(&mut rng, None).unwrap(); - self.output_masks = Some(output_masks); - - let bp = Bulletproofs::new(&commitments).unwrap(); - bp.serialize(&mut serialized).unwrap(); - - let tx = self.signable.prepare_transaction(&commitments, bp); - self.tx = Some(tx); - } + // We could add further entropy here, and previous versions of this library did so + // As of right now, the multisig's key, the inputs being spent, and the FROST data itself + // will be used for RNG seeds. In order to recreate these RNG seeds, breaking privacy, + // counterparties must have knowledge of the multisig, either the view key or access to the + // coordination layer, and then access to the actual FROST signing process + // If the commitments are sent in plain text, then entropy here also would be, making it not + // increase privacy. If they're not sent in plain text, or are otherwise inaccessible, they + // already offer sufficient entropy. That's why further entropy is not included Ok(serialized) } @@ -162,52 +165,35 @@ impl StateMachine for TransactionMachine { fn sign( &mut self, commitments: &[Option>], + // Drop FROST's 'msg' since we calculate the actual message in this function _: &[u8] ) -> Result, FrostError> { if self.state() != State::Preprocessed { Err(FrostError::InvalidSignTransition(State::Preprocessed, self.state()))?; } - // FROST commitments, image, commitments, and their proofs - let clsag_len = 64 + ClsagMultisig::serialized_len(); - let clsag_lens = clsag_len * self.clsags.len(); - - // Split out the prep and update the TX - let mut tx; - if self.leader { - tx = self.tx.take().unwrap(); - } else { - let (l, prep) = commitments.iter().enumerate().filter(|(_, prep)| prep.is_some()).next() - .ok_or(FrostError::InternalError("no participants".to_string()))?; - let prep = prep.as_ref().unwrap(); - - // Not invalid outputs due to doing a dummy prep as leader - let (commitments, output_masks) = self.signable.prepare_outputs( - &mut ChaCha12Rng::from_seed( - self.transcript.rng_seed( - b"tx_keys", - Some(prep[clsag_lens .. (clsag_lens + 32)].try_into().map_err(|_| FrostError::InvalidShare(l))?) - ) - ), - None - ).map_err(|_| FrostError::InvalidShare(l))?; - self.output_masks.replace(output_masks); - - // Verify the provided bulletproofs if not leader - let bp = Bulletproofs::deserialize( - &mut std::io::Cursor::new(&prep[(clsag_lens + 32) .. prep.len()]) - ).map_err(|_| FrostError::InvalidShare(l))?; - if !bp.verify(&commitments.iter().map(|c| c.calculate()).collect::>()) { - Err(FrostError::InvalidShare(l))?; + // Add all commitments to the transcript for their entropy + // While each CLSAG will do this as they need to for security, they have their own transcripts + // cloned from this TX's initial premise's transcript. For our TX transcript to have the CLSAG + // data for entropy, it'll have to be added ourselves + for c in 0 .. commitments.len() { + self.transcript.append_message(b"participant", &u16::try_from(c).unwrap().to_le_bytes()); + if c == self.i { + self.transcript.append_message(b"preprocess", &self.our_preprocess); + } else if let Some(commitments) = commitments[c].as_ref() { + self.transcript.append_message(b"preprocess", commitments); } - - tx = self.signable.prepare_transaction(&commitments, bp); } + // FROST commitments, image, H commitments, and their proofs + let clsag_len = 64 + ClsagMultisig::serialized_len(); + for c in 0 .. self.clsags.len() { - // Calculate the key images in order to update the TX + // Calculate the key images // Multisig will parse/calculate/validate this as needed, yet doing so here as well provides - // the easiest API overall + // the easiest API overall, as this is where the TX is (which needs the key images in its + // message), along with where the outputs are determined (where our change output needs these + // to be unique) for (l, serialized) in commitments.iter().enumerate().filter(|(_, s)| s.is_some()) { self.images[c] += CompressedEdwardsY( serialized.as_ref().unwrap()[((c * clsag_len) + 64) .. ((c * clsag_len) + 96)] @@ -216,6 +202,34 @@ impl StateMachine for TransactionMachine { } } + // Create the actual transaction + let mut tx = { + // Calculate uniqueness + let mut images = self.images.clone(); + images.sort_by(key_image_sort); + + // Not invalid outputs due to already doing a dummy prep + let (commitments, output_masks) = self.signable.prepare_outputs( + &mut ChaCha12Rng::from_seed(self.transcript.rng_seed(b"tx_keys", None)), + uniqueness( + &images.iter().map(|image| Input::ToKey { + amount: 0, + key_offsets: vec![], + key_image: *image + }).collect::>() + ) + ).expect("Couldn't prepare outputs despite already doing a dummy prep"); + self.output_masks = Some(output_masks); + + self.signable.prepare_transaction( + &commitments, + Bulletproofs::new( + &mut ChaCha12Rng::from_seed(self.transcript.rng_seed(b"bulletproofs", None)), + &commitments + ).unwrap() + ) + }; + let mut commitments = (0 .. self.inputs.len()).map(|c| commitments.iter().map( |commitments| commitments.clone().map( |commitments| commitments[(c * clsag_len) .. ((c * clsag_len) + clsag_len)].to_vec()