From f813fd48ed6c302946690fb9c4da3dd41cdfe6f4 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 21 Oct 2021 09:38:43 +0200 Subject: [PATCH 1/6] Update to latest revault_tx (slight behaviour change) We pass around signatures instead of byte vectors from the database. We use the dedicated interface to add signatures to the Spend transaction. We now return `null` if the stored signatures are not valid for the Spend we are presented: ie if we are polled for another transaction spending the very same coins. Honest software would ~never do that. --- Cargo.lock | 8 ++++---- Cargo.toml | 10 ++++++++-- fuzz/Cargo.lock | 39 +++++++++++++++++++++++++++++++++------ src/database/mod.rs | 4 +++- src/database/schema.rs | 4 ++-- src/processing.rs | 34 ++++++++++++++++++++-------------- src/tests/builder.rs | 3 ++- 7 files changed, 72 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c55d0d7..83a3d28 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -364,9 +364,9 @@ dependencies = [ [[package]] name = "revault_net" -version = "0.1.0" +version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7d7581647856ce926493049324bbd0731fb67a34d6ecea676773fd7266617954" +checksum = "4245d1f8afc1ba8b8de3f65626998bd0511009dd86d7ce3fd066a51df62c4b7e" dependencies = [ "bitcoin", "log", @@ -379,9 +379,9 @@ dependencies = [ [[package]] name = "revault_tx" -version = "0.3.0" +version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c5342b21d7e70d39b01ab4fd9aa207849fca9d438f1930c67f5d9de27b21dfa4" +checksum = "9a3e08e97cd866f7f1e4a11ca71604f422d7d6d77ec923fd1c140fb19827e381" dependencies = [ "base64", "bitcoinconsensus", diff --git a/Cargo.toml b/Cargo.toml index a2e90e3..b6c06f5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,6 +3,12 @@ name = "cosignerd" version = "0.0.2" authors = ["JSwambo ", "Antoine Poinsot "] edition = "2018" +repository = "https://github.com/revault/revaultd" +license-file = "LICENCE" +keywords = ["revault", "bitcoin", "vault"] +description = "Revault cosigning server" +exclude = [".github/"] + [dependencies] # For the configuration file @@ -22,8 +28,8 @@ libc = "0.2" rusqlite = { version = "0.24.2", features = ["bundled"] } # Revault-specific libraries -revault_tx = { version = "0.3", features = ["use-serde"] } -revault_net = "0.1" +revault_tx = { version = "0.4", features = ["use-serde"] } +revault_net = "0.2" # For fuzz testing bitcoin = {version = "0.27", features = ["rand"], optional = true} diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index f36f1b7..6c2699b 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -98,14 +98,14 @@ dependencies = [ [[package]] name = "cosignerd" -version = "0.0.1" +version = "0.0.2" dependencies = [ "bitcoin", "fern", "libc", "log", - "revault_net", - "revault_tx", + "revault_net 0.2.0", + "revault_tx 0.4.0", "rusqlite", "serde", "serde_json", @@ -145,8 +145,8 @@ version = "0.0.1" dependencies = [ "cosignerd", "honggfuzz", - "revault_net", - "revault_tx", + "revault_net 0.1.0", + "revault_tx 0.3.0", "serde", "serde_json", ] @@ -407,7 +407,22 @@ checksum = "7d7581647856ce926493049324bbd0731fb67a34d6ecea676773fd7266617954" dependencies = [ "bitcoin", "log", - "revault_tx", + "revault_tx 0.3.0", + "serde", + "serde_json", + "snow", + "sodiumoxide", +] + +[[package]] +name = "revault_net" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4245d1f8afc1ba8b8de3f65626998bd0511009dd86d7ce3fd066a51df62c4b7e" +dependencies = [ + "bitcoin", + "log", + "revault_tx 0.4.0", "serde", "serde_json", "snow", @@ -426,6 +441,18 @@ dependencies = [ "serde", ] +[[package]] +name = "revault_tx" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a3e08e97cd866f7f1e4a11ca71604f422d7d6d77ec923fd1c140fb19827e381" +dependencies = [ + "base64", + "bitcoinconsensus", + "miniscript", + "serde", +] + [[package]] name = "rusqlite" version = "0.24.2" diff --git a/src/database/mod.rs b/src/database/mod.rs index 8053742..6048306 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -89,6 +89,8 @@ impl TryFrom<&Row<'_>> for DbSignedOutpoint { vout: row.get(1)?, }; let signature = row.get::<_, Vec>(2)?; + let signature = Signature::from_der(&signature) + .expect("We only ever store valid DER-encoded signatures"); Ok(DbSignedOutpoint { outpoint, @@ -251,7 +253,7 @@ mod test { .unwrap() .unwrap() .signature, - sig.serialize_der().to_vec() + sig ); } } diff --git a/src/database/schema.rs b/src/database/schema.rs index 9ce9b92..11547b8 100644 --- a/src/database/schema.rs +++ b/src/database/schema.rs @@ -1,4 +1,4 @@ -use revault_tx::miniscript::bitcoin::OutPoint; +use revault_tx::miniscript::bitcoin::{secp256k1::Signature, OutPoint}; pub const SCHEMA: &str = "\ @@ -21,5 +21,5 @@ pub struct DbSignedOutpoint { pub outpoint: OutPoint, // We don't even take care of parsing it as a Signature, as we only input it with // to_der() and use it to insert in partial_sigs (which takes raw bytes) - pub signature: Vec, + pub signature: Signature, } diff --git a/src/processing.rs b/src/processing.rs index daae8c0..9062c2d 100644 --- a/src/processing.rs +++ b/src/processing.rs @@ -44,7 +44,8 @@ pub fn process_sign_message( bitcoin_privkey: &secp256k1::SecretKey, ) -> Result { let db_path = config.db_file(); - let secp = secp256k1::Secp256k1::signing_only(); + // TODO: Cache it in the caller + let secp = secp256k1::Secp256k1::new(); let our_pubkey = BitcoinPubkey { compressed: true, key: secp256k1::PublicKey::from_secret_key(&secp, bitcoin_privkey), @@ -90,14 +91,20 @@ pub fn process_sign_message( // soon.. } - // If we had all the signatures for all these outpoints, send them. They'll figure out whether - // they are valid or not. + // If we had all the signatures for all these outpoints, send them if they are valid. if signatures.len() == n_inputs { - for (i, mut sig) in signatures.into_iter().enumerate() { - sig.push(SigHashType::All as u8); - spend_tx.psbt_mut().inputs[i] - .partial_sigs - .insert(our_pubkey, sig); + for (i, sig) in signatures.into_iter().enumerate() { + // Don't let them fool you! + if spend_tx + .add_signature(i, our_pubkey.key, sig, &secp) + .is_err() + { + log::error!( + "Invalid signature. Got a request for a modified Spend: '{}'", + spend_tx + ); + return Ok(null_signature()); + } } return Ok(SignResult { tx: Some(spend_tx) }); } @@ -108,8 +115,7 @@ pub fn process_sign_message( } // If we signed none of the input, append fresh signatures for each of them to the PSBT. - let mut psbtins = spend_tx.psbt().inputs.clone(); - for (i, psbtin) in psbtins.iter_mut().enumerate() { + for i in 0..spend_tx.psbt().inputs.len() { // FIXME: sighash cache upstream... let sighash = spend_tx .signature_hash(i, SigHashType::All) @@ -117,10 +123,11 @@ pub fn process_sign_message( let sighash = secp256k1::Message::from_slice(&sighash).expect("Sighash is 32 bytes"); let signature = secp.sign(&sighash, bitcoin_privkey); - let mut raw_sig = signature.serialize_der().to_vec(); - raw_sig.push(SigHashType::All as u8); + let res = spend_tx + .add_signature(i, our_pubkey.key, signature, &secp) + .expect("We must provide valid signatures"); assert!( - psbtin.partial_sigs.insert(our_pubkey, raw_sig).is_none(), + res.is_none(), "If there was a signature for our pubkey already and we didn't return \ above, we have big problems.." ); @@ -132,7 +139,6 @@ pub fn process_sign_message( ) .map_err(SignProcessingError::Database)?; } - spend_tx.psbt_mut().inputs = psbtins; // Belt-and-suspender: if it was not empty, we would have signed a prevout twice. assert!(signatures.is_empty()); diff --git a/src/tests/builder.rs b/src/tests/builder.rs index d323beb..20f974f 100644 --- a/src/tests/builder.rs +++ b/src/tests/builder.rs @@ -147,7 +147,8 @@ impl CosignerTestBuilder { SpendTransaction::new( unvault_txins, - vec![SpendTxOut::Destination(spend_txo.clone())], + vec![SpendTxOut::new(spend_txo.clone())], + None, &cpfp_descriptor.derive(0.into(), &secp), 0, true, From 4f21d97019df84341cb64b93a3f7efb0958b4fa2 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 21 Oct 2021 08:55:27 +0200 Subject: [PATCH 2/6] processing: remove the check for duplicated inputs It's now performed by revault_tx --- src/processing.rs | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/processing.rs b/src/processing.rs index 9062c2d..050748b 100644 --- a/src/processing.rs +++ b/src/processing.rs @@ -61,23 +61,6 @@ pub fn process_sign_message( // Gather what signatures we have for these prevouts let mut signatures = Vec::with_capacity(n_inputs); for txin in spend_tx.tx().input.iter() { - if spend_tx - .tx() - .input - .iter() - .filter_map(|curr| { - if curr.previous_output == txin.previous_output { - Some(curr) - } else { - None - } - }) - .count() - > 1 - { - return Err(SignProcessingError::Garbage); - } - if let Some(signed_op) = db_signed_outpoint(&db_path, &txin.previous_output) .map_err(SignProcessingError::Database)? { From 0477e31c93619a6a1cedc3f2d0f81134389fdc6a Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 21 Oct 2021 10:08:53 +0200 Subject: [PATCH 3/6] processing: use a signature hash cache --- src/processing.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/processing.rs b/src/processing.rs index 050748b..9c1982d 100644 --- a/src/processing.rs +++ b/src/processing.rs @@ -5,7 +5,7 @@ use crate::{ use revault_net::message::cosigner::{SignRequest, SignResult}; use revault_tx::{ - bitcoin::{secp256k1, PublicKey as BitcoinPubkey, SigHashType}, + bitcoin::{secp256k1, PublicKey as BitcoinPubkey, SigHashType, util::bip143::SigHashCache}, error::InputSatisfactionError, transactions::RevaultTransaction, }; @@ -98,10 +98,11 @@ pub fn process_sign_message( } // If we signed none of the input, append fresh signatures for each of them to the PSBT. + let unsigned_tx = spend_tx.tx().clone(); + let mut sighash_cache = SigHashCache::new(&unsigned_tx); for i in 0..spend_tx.psbt().inputs.len() { - // FIXME: sighash cache upstream... let sighash = spend_tx - .signature_hash(i, SigHashType::All) + .signature_hash_cached(i, SigHashType::All, &mut sighash_cache) .map_err(SignProcessingError::InsanePsbtMissingInput)?; let sighash = secp256k1::Message::from_slice(&sighash).expect("Sighash is 32 bytes"); From b572cea7c44b74051d8295c32c627e8f0706f9ab Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 21 Oct 2021 10:17:13 +0200 Subject: [PATCH 4/6] processing: cache the secp context --- fuzz/targets/process_sign_message.rs | 1 + src/bin/cosignerd.rs | 3 ++- src/processing.rs | 9 ++++++--- src/tests/builder.rs | 2 ++ 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/fuzz/targets/process_sign_message.rs b/fuzz/targets/process_sign_message.rs index 12127e9..b45e06c 100644 --- a/fuzz/targets/process_sign_message.rs +++ b/fuzz/targets/process_sign_message.rs @@ -34,6 +34,7 @@ fn main() { &builder.config, msg, &builder.bitcoin_privkey, + &builder.secp, ) { Ok(res) => res, Err(cosignerd::processing::SignProcessingError::Database(e)) => panic!("{}", e), diff --git a/src/bin/cosignerd.rs b/src/bin/cosignerd.rs index 0f9820d..4eeefed 100644 --- a/src/bin/cosignerd.rs +++ b/src/bin/cosignerd.rs @@ -65,6 +65,7 @@ fn daemon_main( }); let managers_noise_pubkeys: Vec = config.managers.iter().map(|m| m.noise_key).collect(); + let secp_ctx = secp256k1::Secp256k1::new(); // We expect a single connection once in a while, there is *no need* for complexity here so // just treat incoming connections sequentially. @@ -86,7 +87,7 @@ fn daemon_main( RequestParams::Sign(sign_req) => { log::trace!("Decoded request: {:#?}", sign_req); - let res = match process_sign_message(&config, sign_req, bitcoin_privkey) { + let res = match process_sign_message(&config, sign_req, bitcoin_privkey, &secp_ctx) { Ok(res) => res, Err(e) => { log::error!("Error when processing 'sign' message: '{}'", e); diff --git a/src/processing.rs b/src/processing.rs index 9c1982d..aa0cd49 100644 --- a/src/processing.rs +++ b/src/processing.rs @@ -5,7 +5,7 @@ use crate::{ use revault_net::message::cosigner::{SignRequest, SignResult}; use revault_tx::{ - bitcoin::{secp256k1, PublicKey as BitcoinPubkey, SigHashType, util::bip143::SigHashCache}, + bitcoin::{secp256k1, util::bip143::SigHashCache, PublicKey as BitcoinPubkey, SigHashType}, error::InputSatisfactionError, transactions::RevaultTransaction, }; @@ -42,10 +42,9 @@ pub fn process_sign_message( config: &Config, sign_msg: SignRequest, bitcoin_privkey: &secp256k1::SecretKey, + secp: &secp256k1::Secp256k1, ) -> Result { let db_path = config.db_file(); - // TODO: Cache it in the caller - let secp = secp256k1::Secp256k1::new(); let our_pubkey = BitcoinPubkey { compressed: true, key: secp256k1::PublicKey::from_secret_key(&secp, bitcoin_privkey), @@ -173,6 +172,7 @@ mod test { &test_framework.config, sign_a.clone(), &test_framework.bitcoin_privkey, + &test_framework.secp, ) .unwrap(); let tx = tx.unwrap(); @@ -190,6 +190,7 @@ mod test { &test_framework.config, sign_a, &test_framework.bitcoin_privkey, + &test_framework.secp, ) .unwrap(); assert_eq!(tx, second_psbt.unwrap()); @@ -211,6 +212,7 @@ mod test { &test_framework.config, sign_a, &test_framework.bitcoin_privkey, + &test_framework.secp, ) .unwrap(); assert!(tx.is_none(), "It contains a duplicated outpoint"); @@ -226,6 +228,7 @@ mod test { &test_framework.config, sign_msg, &test_framework.bitcoin_privkey, + &test_framework.secp, ) .unwrap_err(); } diff --git a/src/tests/builder.rs b/src/tests/builder.rs index 20f974f..03aad81 100644 --- a/src/tests/builder.rs +++ b/src/tests/builder.rs @@ -35,6 +35,7 @@ pub struct CosignerTestBuilder { pub noise_privkey: NoisePrivkey, pub bitcoin_privkey: secp256k1::SecretKey, pub managers_keys: Vec, + pub secp: secp256k1::Secp256k1, } impl CosignerTestBuilder { @@ -95,6 +96,7 @@ impl CosignerTestBuilder { noise_privkey, bitcoin_privkey, managers_keys, + secp, } } From 444630ceff24387c61bc5fdd4171d6d3aa1c524b Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 21 Oct 2021 10:35:07 +0200 Subject: [PATCH 5/6] main: process all messages of a connection Connections should be short-lived, but still it's cleaner. --- src/bin/cosignerd.rs | 63 +++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 27 deletions(-) diff --git a/src/bin/cosignerd.rs b/src/bin/cosignerd.rs index 4eeefed..dce6b9c 100644 --- a/src/bin/cosignerd.rs +++ b/src/bin/cosignerd.rs @@ -52,6 +52,35 @@ fn setup_logger(log_level: log::LevelFilter) -> Result<(), fern::InitError> { Ok(()) } +fn process_message( + secp_ctx: &secp256k1::Secp256k1, + config: &Config, + bitcoin_privkey: &secp256k1::SecretKey, + message: RequestParams, +) -> Option { + match message { + RequestParams::Sign(sign_req) => { + log::trace!("Decoded request: {:#?}", sign_req); + + let res = match process_sign_message(&config, sign_req, bitcoin_privkey, &secp_ctx) { + Ok(res) => res, + Err(e) => { + log::error!("Error when processing 'sign' message: '{}'", e); + return None; + } + }; + log::trace!("Decoded response: {:#?}", res); + + Some(ResponseResult::SignResult(res)) + } + _ => { + // FIXME: This should probably be fatal, they are violating the protocol + log::error!("Unexpected message: '{:?}'", message); + None + } + } +} + // Wait for connections from managers on the configured interface and process `sign` messages. fn daemon_main( config: Config, @@ -82,37 +111,17 @@ fn daemon_main( } }; - match kk_stream.read_req(|req_params| { - match req_params { - RequestParams::Sign(sign_req) => { - log::trace!("Decoded request: {:#?}", sign_req); - - let res = match process_sign_message(&config, sign_req, bitcoin_privkey, &secp_ctx) { - Ok(res) => res, - Err(e) => { - log::error!("Error when processing 'sign' message: '{}'", e); - return None; - } - }; - log::trace!("Decoded response: {:#?}", res); - - Some(ResponseResult::SignResult(res)) - } - _ => { - // FIXME: This should probably be fatal, they are violating the protocol - log::error!("Unexpected message: '{:?}'", req_params); - None - } - } - }) { - Ok(buf) => buf, - Err(e) => { + // Process all messages from this connection. + loop { + if let Err(e) = + kk_stream.read_req(|msg| process_message(&secp_ctx, &config, &bitcoin_privkey, msg)) + { log::error!( - "Error handling request from stream '{:?}': '{}'", + "Error handling request from stream '{:?}': '{}'. Dropping connection.", kk_stream, e ); - continue; + break; } } } From 1561833455916adefd902d629574761673f2f070 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 22 Oct 2021 11:38:32 +0200 Subject: [PATCH 6/6] ci: workaround for LLVM pass API break --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2bfca14..7e90ce1 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -43,7 +43,7 @@ jobs: override: true profile: minimal - name: Run fuzz testing script - run: sudo apt install build-essential binutils-dev libunwind-dev libblocksruntime-dev liblzma-dev && cargo install --force honggfuzz && cd fuzz && git clone https://github.com/revault/cosignerd_fuzz_corpus && HFUZZ_RUN_ARGS="--exit_upon_crash --iterations 10000 -v --timeout 2 --input cosignerd_fuzz_corpus" cargo hfuzz run process_sign_message && cd .. + run: sudo apt install build-essential binutils-dev libunwind-dev libblocksruntime-dev liblzma-dev && cargo install --force honggfuzz && cd fuzz && git clone https://github.com/revault/cosignerd_fuzz_corpus && RUSTFLAGS="-Znew-llvm-pass-manager=no" HFUZZ_RUN_ARGS="--exit_upon_crash --iterations 10000 -v --timeout 2 --input cosignerd_fuzz_corpus" cargo hfuzz run process_sign_message rustfmt_check: runs-on: ubuntu-latest