Skip to content

Murisi/fix tx malleability #1607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brentstone merged 10 commits into from
Jul 6, 2023
Merged

Murisi/fix tx malleability #1607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
65cc5bb
Now sign over all sections in transactions.
Jun 16, 2023
676a5ed
Fixed the tests involving transaction signing.
Jun 20, 2023
1274d1a
Fixed clippy and formatting issues.
Jun 21, 2023
7737db5
Merged in v0.17.4.
murisi Jun 23, 2023
ef34267
VPs now check that code and data are signed together.
murisi Jun 25, 2023
1819d16
Make the signature section unmalleable.
murisi Jun 25, 2023
a3a8bbc
Expanded validate_header to check for signature over all sections, an…
murisi Jun 27, 2023
d52a137
[fix]: Fix failing test-wasm by adding code section and also signing …
murisi Jul 2, 2023
b20c0b2
Added changelog entry.
murisi Jul 3, 2023
56a0863
Merged in 0.18.0.
murisi Jul 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changelog/unreleased/bug-fixes/1607-fix-tx-malleability.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Fixed bug that allowed transactions to be modified without invalidating
transaction hash ([\#1607](https://github.com/anoma/namada/pull/1607))
6 changes: 1 addition & 5 deletions apps/src/lib/client/tx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,13 @@ use namada::types::dec::Dec;
use namada::types::governance::{
OfflineProposal, OfflineVote, Proposal, ProposalVote, VoteType,
};
use namada::types::hash::Hash;
use namada::types::key::*;
use namada::types::storage::{Epoch, Key};
use namada::types::token;
use namada::types::transaction::governance::{
InitProposalData, ProposalType, VoteProposalData,
};
use namada::types::transaction::{InitValidator, TxType};
use sha2::{Digest as Sha2Digest, Sha256};

use super::rpc;
use crate::cli::context::WalletAddress;
Expand Down Expand Up @@ -213,16 +211,14 @@ pub async fn submit_init_validator<
let extra = tx.add_section(Section::ExtraData(Code::from_hash(
validator_vp_code_hash,
)));
let extra_hash =
Hash(extra.hash(&mut Sha256::new()).finalize_reset().into());
let data = InitValidator {
account_key,
consensus_key: consensus_key.ref_to(),
protocol_key,
dkg_key,
commission_rate,
max_commission_rate_change,
validator_vp_code_hash: extra_hash,
validator_vp_code_hash: extra.get_hash(),
};
let data = data.try_to_vec().expect("Encoding tx data shouldn't fail");
tx.header.chain_id = tx_args.chain_id.clone().unwrap();
Expand Down
15 changes: 9 additions & 6 deletions apps/src/lib/node/ledger/shell/finalize_block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ where
continue;
}

let tx = if let Ok(()) = tx.validate_header() {
let tx = if tx.validate_tx().is_ok() {
tx
} else {
tracing::error!(
Expand Down Expand Up @@ -311,7 +311,7 @@ where
DecryptedTx::Decrypted { has_valid_pow: _ } => {
if let Some(code_sec) = tx
.get_section(tx.code_sechash())
.and_then(Section::code_sec)
.and_then(|x| Section::code_sec(x.as_ref()))
{
stats.increment_tx_type(
code_sec.code.hash().to_string(),
Expand Down Expand Up @@ -991,11 +991,11 @@ mod test_finalize_block {
wrapper.set_code(Code::new(
format!("transaction data: {}", i).as_bytes().to_owned(),
));
wrapper.encrypt(&Default::default());
wrapper.add_section(Section::Signature(Signature::new(
&wrapper.header_hash(),
vec![wrapper.header_hash(), wrapper.sections[0].get_hash()],
&keypair,
)));
wrapper.encrypt(&Default::default());
if i > 1 {
processed_txs.push(ProcessedTx {
tx: wrapper.to_bytes(),
Expand Down Expand Up @@ -1229,11 +1229,14 @@ mod test_finalize_block {
.as_bytes()
.to_owned(),
));
wrapper_tx.encrypt(&Default::default());
wrapper_tx.add_section(Section::Signature(Signature::new(
&wrapper_tx.header_hash(),
vec![
wrapper_tx.header_hash(),
wrapper_tx.sections[0].get_hash(),
],
&keypair,
)));
wrapper_tx.encrypt(&Default::default());
valid_txs.push(wrapper_tx.clone());
processed_txs.push(ProcessedTx {
tx: wrapper_tx.to_bytes(),
Expand Down
27 changes: 19 additions & 8 deletions apps/src/lib/node/ledger/shell/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,8 +793,8 @@ where
}

// Tx signature check
let tx_type = match tx.validate_header() {
Ok(()) => tx.header(),
let tx_type = match tx.validate_tx() {
Ok(_) => tx.header(),
Err(msg) => {
response.code = ErrorCodes::InvalidSig.into();
response.log = msg.to_string();
Expand Down Expand Up @@ -1378,11 +1378,14 @@ mod test_mempool_validate {
invalid_wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
invalid_wrapper
.set_data(Data::new("transaction data".as_bytes().to_owned()));
invalid_wrapper.encrypt(&Default::default());
invalid_wrapper.add_section(Section::Signature(Signature::new(
&invalid_wrapper.header_hash(),
vec![
invalid_wrapper.header_hash(),
invalid_wrapper.sections[0].get_hash(),
],
&keypair,
)));
invalid_wrapper.encrypt(&Default::default());

// we mount a malleability attack to try and remove the fee
let mut new_wrapper =
Expand Down Expand Up @@ -1443,11 +1446,11 @@ mod test_mempool_validate {
wrapper.header.chain_id = shell.chain_id.clone();
wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
wrapper.set_data(Data::new("transaction data".as_bytes().to_owned()));
wrapper.encrypt(&Default::default());
wrapper.add_section(Section::Signature(Signature::new(
&wrapper.header_hash(),
vec![wrapper.header_hash(), wrapper.sections[0].get_hash()],
&keypair,
)));
wrapper.encrypt(&Default::default());

// Write wrapper hash to storage
let wrapper_hash = wrapper.header_hash();
Expand Down Expand Up @@ -1539,7 +1542,11 @@ mod test_mempool_validate {
tx.set_code(Code::new("wasm_code".as_bytes().to_owned()));
tx.set_data(Data::new("transaction data".as_bytes().to_owned()));
tx.add_section(Section::Signature(Signature::new(
&tx.header_hash(),
vec![
tx.header_hash(),
tx.sections[0].get_hash(),
tx.sections[1].get_hash(),
],
&keypair,
)));

Expand Down Expand Up @@ -1570,7 +1577,11 @@ mod test_mempool_validate {
tx.set_code(Code::new("wasm_code".as_bytes().to_owned()));
tx.set_data(Data::new("transaction data".as_bytes().to_owned()));
tx.add_section(Section::Signature(Signature::new(
&tx.header_hash(),
vec![
tx.header_hash(),
tx.sections[0].get_hash(),
tx.sections[1].get_hash(),
],
&keypair,
)));

Expand Down
33 changes: 18 additions & 15 deletions apps/src/lib/node/ledger/shell/prepare_proposal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ where
if let (Some(block_time), Some(exp)) = (block_time.as_ref(), &tx.header.expiration) {
if block_time > exp { return None }
}
if tx.validate_header().is_ok() && tx.header().wrapper().is_some() && self.replay_protection_checks(&tx, tx_bytes.as_slice(), &mut temp_wl_storage).is_ok() {
if tx.validate_tx().is_ok() && tx.header().wrapper().is_some() && self.replay_protection_checks(&tx, tx_bytes.as_slice(), &mut temp_wl_storage).is_ok() {
return Some(tx_bytes.clone());
}
}
Expand Down Expand Up @@ -370,11 +370,11 @@ mod test_prepare_proposal {
tx.set_data(Data::new(
format!("transaction data: {}", i).as_bytes().to_owned(),
));
tx.encrypt(&Default::default());
tx.add_section(Section::Signature(Signature::new(
&tx.header_hash(),
vec![tx.header_hash(), tx.sections[0].get_hash()],
&keypair,
)));
tx.encrypt(&Default::default());

shell.enqueue_tx(tx.clone());
expected_wrapper.push(tx.clone());
Expand Down Expand Up @@ -437,11 +437,11 @@ mod test_prepare_proposal {
wrapper.header.chain_id = shell.chain_id.clone();
wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
wrapper.set_data(Data::new("transaction data".as_bytes().to_owned()));
wrapper.encrypt(&Default::default());
wrapper.add_section(Section::Signature(Signature::new(
&wrapper.header_hash(),
vec![wrapper.header_hash(), wrapper.sections[0].get_hash()],
&keypair,
)));
wrapper.encrypt(&Default::default());

// Write wrapper hash to storage
let wrapper_unsigned_hash = wrapper.header_hash();
Expand Down Expand Up @@ -489,11 +489,11 @@ mod test_prepare_proposal {
wrapper.header.chain_id = shell.chain_id.clone();
wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
wrapper.set_data(Data::new("transaction data".as_bytes().to_owned()));
wrapper.encrypt(&Default::default());
wrapper.add_section(Section::Signature(Signature::new(
&wrapper.header_hash(),
vec![wrapper.header_hash(), wrapper.sections[0].get_hash()],
&keypair,
)));
wrapper.encrypt(&Default::default());

let req = RequestPrepareProposal {
txs: vec![wrapper.to_bytes(); 2],
Expand Down Expand Up @@ -530,11 +530,11 @@ mod test_prepare_proposal {
wrapper.header.chain_id = shell.chain_id.clone();
wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
wrapper.set_data(Data::new("transaction data".as_bytes().to_owned()));
wrapper.encrypt(&Default::default());
wrapper.add_section(Section::Signature(Signature::new(
&wrapper.header_hash(),
vec![wrapper.header_hash(), wrapper.sections[0].get_hash()],
&keypair,
)));
wrapper.encrypt(&Default::default());
let inner_unsigned_hash =
wrapper.clone().update_header(TxType::Raw).header_hash();

Expand Down Expand Up @@ -585,11 +585,11 @@ mod test_prepare_proposal {
wrapper.set_code(tx_code.clone());
let tx_data = Data::new("transaction data".as_bytes().to_owned());
wrapper.set_data(tx_data.clone());
wrapper.encrypt(&Default::default());
wrapper.add_section(Section::Signature(Signature::new(
&wrapper.header_hash(),
vec![wrapper.header_hash(), wrapper.sections[0].get_hash()],
&keypair,
)));
wrapper.encrypt(&Default::default());

let mut new_wrapper =
Tx::new(TxType::Wrapper(Box::new(WrapperTx::new(
Expand All @@ -607,11 +607,14 @@ mod test_prepare_proposal {
new_wrapper.header.timestamp = wrapper.header.timestamp;
new_wrapper.set_code(tx_code);
new_wrapper.set_data(tx_data);
new_wrapper.encrypt(&Default::default());
new_wrapper.add_section(Section::Signature(Signature::new(
&new_wrapper.header_hash(),
vec![
new_wrapper.header_hash(),
new_wrapper.sections[0].get_hash(),
],
&keypair,
)));
new_wrapper.encrypt(&Default::default());

let req = RequestPrepareProposal {
txs: vec![wrapper.to_bytes(), new_wrapper.to_bytes()],
Expand Down Expand Up @@ -650,11 +653,11 @@ mod test_prepare_proposal {
wrapper_tx.set_code(Code::new("wasm_code".as_bytes().to_owned()));
wrapper_tx
.set_data(Data::new("transaction data".as_bytes().to_owned()));
wrapper_tx.encrypt(&Default::default());
wrapper_tx.add_section(Section::Signature(Signature::new(
&wrapper_tx.header_hash(),
vec![wrapper_tx.header_hash(), wrapper_tx.sections[0].get_hash()],
&keypair,
)));
wrapper_tx.encrypt(&Default::default());

let time = DateTimeUtc::now();
let block_time =
Expand Down
Loading