From 3079bdb2273df54d7b6937a2067b3164e38bb292 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Fri, 4 Oct 2019 15:34:25 +0900 Subject: [PATCH 1/5] Reject TXs when there is a mismatch Reject transactions when there is a mismatch between tx.signatures.len() and tx.message().num_required_signatures. Fixes #3568 --- core/src/sigverify.rs | 256 ++++++++++++++++++++++++++++++++--------- sdk/src/message.rs | 2 + sdk/src/transaction.rs | 1 + 3 files changed, 202 insertions(+), 57 deletions(-) diff --git a/core/src/sigverify.rs b/core/src/sigverify.rs index 1aa3dbaaa168a5..04cd68698923a8 100644 --- a/core/src/sigverify.rs +++ b/core/src/sigverify.rs @@ -33,6 +33,54 @@ pub type TxOffset = PinnedVec; type TxOffsets = (TxOffset, TxOffset, TxOffset, TxOffset, Vec>); +#[derive(Debug, PartialEq, Eq)] +struct PacketOffsets { + pub sig_len: u32, + pub sig_start: u32, + pub msg_start: u32, + pub pubkey_start: u32, +} + +impl PacketOffsets { + pub fn new(sig_len: u32, sig_start: u32, msg_start: u32, pubkey_start: u32) -> Self { + Self { + sig_len, + sig_start, + msg_start, + pubkey_start, + } + } + + pub fn new_with_packet(packet_offsets: PacketOffsets) -> Self { + Self::new( + packet_offsets.sig_len, + packet_offsets.sig_start, + packet_offsets.msg_start, + packet_offsets.pubkey_start, + ) + } +} + +struct UnsanitizedPacketOffsets { + pub correct: bool, + pub packet_offsets: PacketOffsets, +} + +impl UnsanitizedPacketOffsets { + pub fn new( + correct: bool, + sig_len: u32, + sig_start: u32, + msg_start: u32, + pubkey_start: u32, + ) -> Self { + Self { + correct, + packet_offsets: PacketOffsets::new(sig_len, sig_start, msg_start, pubkey_start), + } + } +} + pub fn init() { if let Some(api) = perf_libs::api() { unsafe { @@ -46,17 +94,21 @@ pub fn init() { } fn verify_packet(packet: &Packet) -> u8 { - let (sig_len, sig_start, msg_start, pubkey_start) = get_packet_offsets(packet, 0); - let mut sig_start = sig_start as usize; - let mut pubkey_start = pubkey_start as usize; - let msg_start = msg_start as usize; + let packet_offsets = get_packet_offsets(packet, 0); + let mut sig_start = packet_offsets.sig_start as usize; + let mut pubkey_start = packet_offsets.pubkey_start as usize; + let msg_start = packet_offsets.msg_start as usize; + + if packet_offsets.sig_len == 0 { + return 0; + } if packet.meta.size <= msg_start { return 0; } let msg_end = packet.meta.size; - for _ in 0..sig_len { + for _ in 0..packet_offsets.sig_len { let pubkey_end = pubkey_start as usize + size_of::(); let sig_end = sig_start as usize + size_of::(); @@ -81,24 +133,55 @@ fn batch_size(batches: &[Packets]) -> usize { batches.iter().map(|p| p.packets.len()).sum() } -pub fn get_packet_offsets(packet: &Packet, current_offset: u32) -> (u32, u32, u32, u32) { - let (sig_len, sig_size) = decode_len(&packet.data); - let msg_start_offset = sig_size + sig_len * size_of::(); +// internal function to be unit-tested; should be used only by get_packet_offsets +fn do_get_packet_offsets(packet: &Packet, current_offset: u32) -> UnsanitizedPacketOffsets { + // This directly reads the length of Transaction.signatures (serialized with short_vec) + let (sig_len_untrusted, sig_size) = decode_len(&packet.data); + + // This directly reads MessageHeader.num_required_signatures (serialized with u8) + let msg_start_offset = sig_size + sig_len_untrusted * size_of::(); + // Using msg_start_offset which is based on sig_len_untrusted introduces uncertainty. + // Ultimately, the actual sigverify will determine the uncertainty. + let sig_len_maybe_trusted = packet.data[msg_start_offset] as usize; + let msg_header_size = serialized_size(&MessageHeader::default()).unwrap() as usize; - let (_pubkey_len, pubkey_size) = + // This directly reads the length of Message.account_keys (serialized with short_vec) + let (_pubkey_len, pubkey_len_size) = decode_len(&packet.data[(msg_start_offset + msg_header_size)..]); let sig_start = current_offset as usize + sig_size; let msg_start = current_offset as usize + msg_start_offset; - let pubkey_start = msg_start + msg_header_size + pubkey_size; - - ( - sig_len as u32, - sig_start as u32, - msg_start as u32, - pubkey_start as u32, - ) + let pubkey_start = msg_start + msg_header_size + pubkey_len_size; + + if sig_len_maybe_trusted == sig_len_untrusted { + UnsanitizedPacketOffsets::new( + true, + sig_len_maybe_trusted as u32, + sig_start as u32, + msg_start as u32, + pubkey_start as u32, + ) + } else { + // a malformed packet is detected!! + UnsanitizedPacketOffsets::new( + false, + sig_len_untrusted as u32, + sig_start as u32, + msg_start as u32, + pubkey_start as u32, + ) + } +} + +fn get_packet_offsets(packet: &Packet, current_offset: u32) -> PacketOffsets { + let unsanitized_packet_offsets = do_get_packet_offsets(packet, current_offset); + if unsanitized_packet_offsets.correct { + PacketOffsets::new_with_packet(unsanitized_packet_offsets.packet_offsets) + } else { + // force sigverify to fail by returning zeros + PacketOffsets::new(0, 0, 0, 0) + } } pub fn generate_offsets(batches: &[Packets], recycler: &Recycler) -> Result { @@ -118,24 +201,27 @@ pub fn generate_offsets(batches: &[Packets], recycler: &Recycler) -> R p.packets.iter().for_each(|packet| { let current_offset = current_packet as u32 * size_of::() as u32; - let (sig_len, sig_start, msg_start_offset, pubkey_offset) = - get_packet_offsets(packet, current_offset); - let mut pubkey_offset = pubkey_offset; + let packet_offsets = get_packet_offsets(packet, current_offset); - sig_lens.push(sig_len); + sig_lens.push(packet_offsets.sig_len); - trace!("pubkey_offset: {}", pubkey_offset); - let mut sig_offset = sig_start; - for _ in 0..sig_len { - signature_offsets.push(sig_offset); - sig_offset += size_of::() as u32; + trace!("pubkey_offset: {}", packet_offsets.pubkey_start); + if packet_offsets.sig_len > 0 { + let mut pubkey_offset = packet_offsets.pubkey_start; + let mut sig_offset = packet_offsets.sig_start; + for _ in 0..packet_offsets.sig_len { + signature_offsets.push(sig_offset); + sig_offset += size_of::() as u32; - pubkey_offsets.push(pubkey_offset); - pubkey_offset += size_of::() as u32; + pubkey_offsets.push(pubkey_offset); + pubkey_offset += size_of::() as u32; - msg_start_offsets.push(msg_start_offset); + msg_start_offsets.push(packet_offsets.msg_start); - msg_sizes.push(current_offset + (packet.meta.size as u32) - msg_start_offset); + msg_sizes.push( + current_offset + (packet.meta.size as u32) - packet_offsets.msg_start, + ); + } } current_packet += 1; }); @@ -250,14 +336,18 @@ pub fn ed25519_verify( let mut num = 0; for (vs, sig_vs) in rvs.iter_mut().zip(sig_lens.iter()) { for (v, sig_v) in vs.iter_mut().zip(sig_vs.iter()) { - let mut vout = 1; - for _ in 0..*sig_v { - if 0 == out[num] { - vout = 0; + if *sig_v == 0 { + *v = 0; + } else { + let mut vout = 1; + for _ in 0..*sig_v { + if 0 == out[num] { + vout = 0; + } + num += 1; } - num += 1; + *v = vout; } - *v = vout; if *v != 0 { trace!("VERIFIED PACKET!!!!!"); } @@ -288,6 +378,7 @@ mod tests { use crate::packet::{Packet, Packets}; use crate::recycler::Recycler; use crate::sigverify; + use crate::sigverify::PacketOffsets; use crate::test_tx::{test_multisig_tx, test_tx}; use bincode::{deserialize, serialize, serialized_size}; use solana_sdk::hash::Hash; @@ -325,8 +416,7 @@ mod tests { let message_data = tx.message_data(); let packet = sigverify::make_packet_from_transaction(tx.clone()); - let (sig_len, sig_start, msg_start_offset, pubkey_offset) = - sigverify::get_packet_offsets(&packet, 0); + let packet_offsets = sigverify::get_packet_offsets(&packet, 0); assert_eq!( memfind(&tx_bytes, &tx.signatures[0].as_ref()), @@ -334,17 +424,45 @@ mod tests { ); assert_eq!( memfind(&tx_bytes, &tx.message().account_keys[0].as_ref()), - Some(pubkey_offset as usize) + Some(packet_offsets.pubkey_start as usize) ); assert_eq!( memfind(&tx_bytes, &message_data), - Some(msg_start_offset as usize) + Some(packet_offsets.msg_start as usize) ); assert_eq!( memfind(&tx_bytes, &tx.signatures[0].as_ref()), - Some(sig_start as usize) + Some(packet_offsets.sig_start as usize) + ); + assert_eq!(packet_offsets.sig_len, 1); + } + + #[test] + fn test_untrustworthy_sigs() { + let required_num_sigs = 14; + let actual_num_sigs = 5 as u32; + + let message = Message { + header: MessageHeader { + num_required_signatures: required_num_sigs as u8, + num_credit_only_signed_accounts: 12, + num_credit_only_unsigned_accounts: 11, + }, + account_keys: vec![], + recent_blockhash: Hash::default(), + instructions: vec![], + }; + let mut tx = Transaction::new_unsigned(message); + tx.signatures = vec![Signature::default(); actual_num_sigs as usize]; + let packet = sigverify::make_packet_from_transaction(tx.clone()); + + let unsanitized_packet_offsets = sigverify::do_get_packet_offsets(&packet, 0); + + assert_eq!(unsanitized_packet_offsets.correct, false); + assert_eq!( + unsanitized_packet_offsets.packet_offsets.sig_len, + actual_num_sigs ); - assert_eq!(sig_len, 1); } #[test] @@ -368,8 +486,7 @@ mod tests { tx.signatures = vec![Signature::default(); actual_num_sigs]; let packet = sigverify::make_packet_from_transaction(tx.clone()); - let (_sig_len, _sig_start, _msg_start_offset, pubkey_offset) = - sigverify::get_packet_offsets(&packet, 0); + let unsanitized_packet_offsets = sigverify::do_get_packet_offsets(&packet, 0); let expected_sig_size = 1; let expected_sigs_size = actual_num_sigs * size_of::(); @@ -380,7 +497,10 @@ mod tests { + expected_msg_header_size + expected_pubkey_size; - assert_eq!(expected_pubkey_start, pubkey_offset as usize); + assert_eq!( + expected_pubkey_start, + unsanitized_packet_offsets.packet_offsets.pubkey_start as usize + ); } #[test] @@ -405,33 +525,38 @@ mod tests { } // Just like get_packet_offsets, but not returning redundant information. - fn get_packet_offsets_from_tx(tx: Transaction, current_offset: u32) -> (u32, u32, u32, u32) { + fn get_packet_offsets_from_tx(tx: Transaction, current_offset: u32) -> PacketOffsets { let packet = sigverify::make_packet_from_transaction(tx); - let (sig_len, sig_start, msg_start_offset, pubkey_offset) = - sigverify::get_packet_offsets(&packet, current_offset); - ( - sig_len, - sig_start - current_offset, - msg_start_offset - sig_start, - pubkey_offset - msg_start_offset, + let packet_offsets = sigverify::get_packet_offsets(&packet, current_offset); + PacketOffsets::new( + packet_offsets.sig_len - packet_offsets.sig_len, + packet_offsets.sig_start - current_offset, + packet_offsets.msg_start - packet_offsets.sig_start, + packet_offsets.pubkey_start - packet_offsets.msg_start, ) } #[test] fn test_get_packet_offsets() { - assert_eq!(get_packet_offsets_from_tx(test_tx(), 0), (1, 1, 64, 4)); - assert_eq!(get_packet_offsets_from_tx(test_tx(), 100), (1, 1, 64, 4)); + assert_eq!( + get_packet_offsets_from_tx(test_tx(), 0), + PacketOffsets::new(1, 1, 64, 4) + ); + assert_eq!( + get_packet_offsets_from_tx(test_tx(), 100), + PacketOffsets::new(1, 1, 64, 4) + ); // Ensure we're not indexing packet by the `current_offset` parameter. assert_eq!( get_packet_offsets_from_tx(test_tx(), 1_000_000), - (1, 1, 64, 4) + PacketOffsets::new(1, 1, 64, 4) ); // Ensure we're returning sig_len, not sig_size. assert_eq!( get_packet_offsets_from_tx(test_multisig_tx(), 0), - (2, 1, 128, 4) + PacketOffsets::new(2, 1, 128, 4) ); } @@ -478,6 +603,23 @@ mod tests { assert_eq!(ans, vec![vec![ref_ans; n], vec![ref_ans; n]]); } + #[test] + fn test_verify_tampered_sig_len() { + let mut tx = test_tx().clone(); + // pretend malicious leader dropped a signature... + tx.signatures.pop(); + let packet = sigverify::make_packet_from_transaction(tx); + + let batches = generate_packet_vec(&packet, 1, 1); + + let recycler = Recycler::default(); + let recycler_out = Recycler::default(); + // verify packets + let ans = sigverify::ed25519_verify(&batches, &recycler, &recycler_out); + + assert_eq!(ans, vec![vec![0u8; 1]]); + } + #[test] fn test_verify_zero() { test_verify_n(0, false); diff --git a/sdk/src/message.rs b/sdk/src/message.rs index 46a93e7954252f..7d090a720ec4fd 100644 --- a/sdk/src/message.rs +++ b/sdk/src/message.rs @@ -127,6 +127,7 @@ fn get_program_ids(instructions: &[Instruction]) -> Vec { pub struct MessageHeader { /// The number of signatures required for this message to be considered valid. The /// signatures must match the first `num_required_signatures` of `account_keys`. + /// NOTE: Serialization-related changes must be paired with the direct read at sigverify. pub num_required_signatures: u8, /// The last num_credit_only_signed_accounts of the signed keys are credit-only accounts. @@ -142,6 +143,7 @@ pub struct MessageHeader { #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)] pub struct Message { /// The message header, identifying signed and credit-only `account_keys` + /// NOTE: Serialization-related changes must be paired with the direct read at sigverify. pub header: MessageHeader, /// All the account keys used by this transaction diff --git a/sdk/src/transaction.rs b/sdk/src/transaction.rs index 9cec326be98d4a..bc4ab4474959fb 100644 --- a/sdk/src/transaction.rs +++ b/sdk/src/transaction.rs @@ -60,6 +60,7 @@ pub type Result = result::Result; pub struct Transaction { /// A set of digital signatures of `account_keys`, `program_ids`, `recent_blockhash`, and `instructions`, signed by the first /// signatures.len() keys of account_keys + /// NOTE: Serialization-related changes must be paired with the direct read at sigverify. #[serde(with = "short_vec")] pub signatures: Vec, From 819ab30e5d878299a0be3e34faa35f8269624888 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Thu, 17 Oct 2019 07:41:36 +0900 Subject: [PATCH 2/5] Dont substract by itself... --- core/src/sigverify.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/sigverify.rs b/core/src/sigverify.rs index 04cd68698923a8..c279453f3ed786 100644 --- a/core/src/sigverify.rs +++ b/core/src/sigverify.rs @@ -529,7 +529,7 @@ mod tests { let packet = sigverify::make_packet_from_transaction(tx); let packet_offsets = sigverify::get_packet_offsets(&packet, current_offset); PacketOffsets::new( - packet_offsets.sig_len - packet_offsets.sig_len, + packet_offsets.sig_len, packet_offsets.sig_start - current_offset, packet_offsets.msg_start - packet_offsets.sig_start, packet_offsets.pubkey_start - packet_offsets.msg_start, From 81e022a4ceb6517c055c6a051f6a8be5234291bc Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Thu, 17 Oct 2019 07:49:48 +0900 Subject: [PATCH 3/5] Unify too similar condition clauses --- core/src/sigverify.rs | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/core/src/sigverify.rs b/core/src/sigverify.rs index c279453f3ed786..ecb0b539e8c888 100644 --- a/core/src/sigverify.rs +++ b/core/src/sigverify.rs @@ -154,24 +154,13 @@ fn do_get_packet_offsets(packet: &Packet, current_offset: u32) -> UnsanitizedPac let msg_start = current_offset as usize + msg_start_offset; let pubkey_start = msg_start + msg_header_size + pubkey_len_size; - if sig_len_maybe_trusted == sig_len_untrusted { - UnsanitizedPacketOffsets::new( - true, - sig_len_maybe_trusted as u32, - sig_start as u32, - msg_start as u32, - pubkey_start as u32, - ) - } else { - // a malformed packet is detected!! - UnsanitizedPacketOffsets::new( - false, - sig_len_untrusted as u32, - sig_start as u32, - msg_start as u32, - pubkey_start as u32, - ) - } + UnsanitizedPacketOffsets::new( + sig_len_maybe_trusted == sig_len_untrusted, + sig_len_untrusted as u32, + sig_start as u32, + msg_start as u32, + pubkey_start as u32, + ) } fn get_packet_offsets(packet: &Packet, current_offset: u32) -> PacketOffsets { From b7f518d44fa0a4f3c33d3284ade4eae91f6190f7 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Thu, 17 Oct 2019 07:51:47 +0900 Subject: [PATCH 4/5] Just pass packet_offsets... --- core/src/sigverify.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/core/src/sigverify.rs b/core/src/sigverify.rs index ecb0b539e8c888..ab25185ee113ed 100644 --- a/core/src/sigverify.rs +++ b/core/src/sigverify.rs @@ -50,15 +50,6 @@ impl PacketOffsets { pubkey_start, } } - - pub fn new_with_packet(packet_offsets: PacketOffsets) -> Self { - Self::new( - packet_offsets.sig_len, - packet_offsets.sig_start, - packet_offsets.msg_start, - packet_offsets.pubkey_start, - ) - } } struct UnsanitizedPacketOffsets { @@ -166,7 +157,7 @@ fn do_get_packet_offsets(packet: &Packet, current_offset: u32) -> UnsanitizedPac fn get_packet_offsets(packet: &Packet, current_offset: u32) -> PacketOffsets { let unsanitized_packet_offsets = do_get_packet_offsets(packet, current_offset); if unsanitized_packet_offsets.correct { - PacketOffsets::new_with_packet(unsanitized_packet_offsets.packet_offsets) + unsanitized_packet_offsets.packet_offsets } else { // force sigverify to fail by returning zeros PacketOffsets::new(0, 0, 0, 0) From f8379a7ee5a1767835fcec2deaec2f4dd53ee0a6 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Sat, 19 Oct 2019 01:14:28 +0900 Subject: [PATCH 5/5] Address review comments --- core/src/sigverify.rs | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/core/src/sigverify.rs b/core/src/sigverify.rs index ab25185ee113ed..145e1c164b90ed 100644 --- a/core/src/sigverify.rs +++ b/core/src/sigverify.rs @@ -186,22 +186,20 @@ pub fn generate_offsets(batches: &[Packets], recycler: &Recycler) -> R sig_lens.push(packet_offsets.sig_len); trace!("pubkey_offset: {}", packet_offsets.pubkey_start); - if packet_offsets.sig_len > 0 { - let mut pubkey_offset = packet_offsets.pubkey_start; - let mut sig_offset = packet_offsets.sig_start; - for _ in 0..packet_offsets.sig_len { - signature_offsets.push(sig_offset); - sig_offset += size_of::() as u32; - pubkey_offsets.push(pubkey_offset); - pubkey_offset += size_of::() as u32; + let mut pubkey_offset = packet_offsets.pubkey_start; + let mut sig_offset = packet_offsets.sig_start; + for _ in 0..packet_offsets.sig_len { + signature_offsets.push(sig_offset); + sig_offset += size_of::() as u32; - msg_start_offsets.push(packet_offsets.msg_start); + pubkey_offsets.push(pubkey_offset); + pubkey_offset += size_of::() as u32; - msg_sizes.push( - current_offset + (packet.meta.size as u32) - packet_offsets.msg_start, - ); - } + msg_start_offsets.push(packet_offsets.msg_start); + + msg_sizes + .push(current_offset + (packet.meta.size as u32) - packet_offsets.msg_start); } current_packet += 1; }); @@ -420,11 +418,11 @@ mod tests { #[test] fn test_untrustworthy_sigs() { let required_num_sigs = 14; - let actual_num_sigs = 5 as u32; + let actual_num_sigs = 5; let message = Message { header: MessageHeader { - num_required_signatures: required_num_sigs as u8, + num_required_signatures: required_num_sigs, num_credit_only_signed_accounts: 12, num_credit_only_unsigned_accounts: 11, }, @@ -440,7 +438,7 @@ mod tests { assert_eq!(unsanitized_packet_offsets.correct, false); assert_eq!( - unsanitized_packet_offsets.packet_offsets.sig_len, + unsanitized_packet_offsets.packet_offsets.sig_len as usize, actual_num_sigs ); }