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 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/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..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, @@ -65,6 +94,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. @@ -81,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) { - 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; } } } 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..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}, + bitcoin::{secp256k1, util::bip143::SigHashCache, PublicKey as BitcoinPubkey, SigHashType}, error::InputSatisfactionError, transactions::RevaultTransaction, }; @@ -42,9 +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(); - let secp = secp256k1::Secp256k1::signing_only(); let our_pubkey = BitcoinPubkey { compressed: true, key: secp256k1::PublicKey::from_secret_key(&secp, bitcoin_privkey), @@ -60,23 +60,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)? { @@ -90,14 +73,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,19 +97,20 @@ 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() { - // FIXME: sighash cache upstream... + let unsigned_tx = spend_tx.tx().clone(); + let mut sighash_cache = SigHashCache::new(&unsigned_tx); + for i in 0..spend_tx.psbt().inputs.len() { 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"); 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 +122,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()); @@ -183,6 +172,7 @@ mod test { &test_framework.config, sign_a.clone(), &test_framework.bitcoin_privkey, + &test_framework.secp, ) .unwrap(); let tx = tx.unwrap(); @@ -200,6 +190,7 @@ mod test { &test_framework.config, sign_a, &test_framework.bitcoin_privkey, + &test_framework.secp, ) .unwrap(); assert_eq!(tx, second_psbt.unwrap()); @@ -221,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"); @@ -236,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 d323beb..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, } } @@ -147,7 +149,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,