From 705228ecc2e6339b750ea685cf147dac9bb3dbf4 Mon Sep 17 00:00:00 2001 From: Greg Fitzgerald Date: Wed, 11 Apr 2018 22:05:40 -0600 Subject: [PATCH 1/2] Remove redundant signs --- src/transaction.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/transaction.rs b/src/transaction.rs index dda9581e2..25ee6c4f2 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -93,9 +93,7 @@ pub fn test_tx() -> Transaction { let keypair1 = KeyPair::new(); let pubkey1 = keypair1.pubkey(); let zero = Hash::default(); - let mut tr = Transaction::new(&keypair1, pubkey1, 42, zero); - tr.sign(&keypair1); - return tr; + Transaction::new(&keypair1, pubkey1, 42, zero) } #[cfg(test)] @@ -174,7 +172,6 @@ mod tests { let keypair = KeyPair::new(); let pubkey = keypair.pubkey(); let mut tr = Transaction::new(&keypair, pubkey, 42, zero); - tr.sign(&keypair); tr.data.tokens = 1_000_000; // <-- attack! assert!(!tr.verify_plan()); } @@ -187,7 +184,6 @@ mod tests { let pubkey1 = keypair1.pubkey(); let zero = Hash::default(); let mut tr = Transaction::new(&keypair0, pubkey1, 42, zero); - tr.sign(&keypair0); if let Plan::Pay(ref mut payment) = tr.data.plan { payment.to = thief_keypair.pubkey(); // <-- attack! }; From 51633f509d3df6053361855ce7e0d69c5a27759c Mon Sep 17 00:00:00 2001 From: Greg Fitzgerald Date: Wed, 11 Apr 2018 22:11:01 -0600 Subject: [PATCH 2/2] Fix test The test was meant to ensure the signature covered the 'tokens' field, but then when the 'plan' field was rolled in, Transaction::verify() started failing because Plan::verify() failed. When Transaction::verify() was split into two, the unexpected failure was exposed but went unnoticed. This patch brings it back to its original intent, to ensure signature verification fails if the network attempts to change the client's payment. --- src/transaction.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/transaction.rs b/src/transaction.rs index 25ee6c4f2..4080d11be 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -167,13 +167,17 @@ mod tests { } #[test] - fn test_bad_event_signature() { + fn test_token_attack() { let zero = Hash::default(); let keypair = KeyPair::new(); let pubkey = keypair.pubkey(); let mut tr = Transaction::new(&keypair, pubkey, 42, zero); - tr.data.tokens = 1_000_000; // <-- attack! - assert!(!tr.verify_plan()); + tr.data.tokens = 1_000_000; // <-- attack, part 1! + if let Plan::Pay(ref mut payment) = tr.data.plan { + payment.tokens = tr.data.tokens; // <-- attack, part 2! + }; + assert!(tr.verify_plan()); + assert!(!tr.verify_sig()); } #[test]