Skip to content
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

Jump to bottom

Outsource masp signature verification #3372

Merged
merged 10 commits into from
Jul 24, 2024
Merged

Outsource masp signature verification #3372

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
c6f11bc
Stop requiring authorization signatures for non-MASP related balance …
murisi Jul 16, 2024
17b9fcf
Added a changelog entry.
murisi Jul 16, 2024
3369502
Moves signatures verification from masp vp to the affected vps
grarco Jun 5, 2024
5f70e2c
Masp vp checks for signer actions
grarco Jul 4, 2024
ffd5cfd
Refactors masp action checks
grarco Jul 5, 2024
01484b3
Renames masp signers to authorizers
grarco Jul 12, 2024
77b164d
Transfer transaction pushes masp actions
grarco Jul 12, 2024
14d70f3
Masp vp checks that no unneeded actions are pushed
grarco Jul 19, 2024
dab10a6
Changelog #3312
grarco Jun 5, 2024
fc63450
Transfer transaction fails if masp transparent inputs are not debited
grarco Jul 19, 2024
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/improvements/3312-outsource-masp-sig-verification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Moved the signature verifications out of the masp vp and into the affected
addresses' vps. ([\#3312](https://github.com/anoma/namada/issues/3312))
2 changes: 2 additions & 0 deletions .changelog/unreleased/improvements/3516-reduce-masp-vp-signatures.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Eliminates the MASP VPs requirement for all debited accounts to sign a Tx.
([\#3516](https://github.com/anoma/namada/pull/3516))
169 changes: 101 additions & 68 deletions crates/namada/src/ledger/native_vp/masp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ use namada_core::ibc::apps::transfer::types::msgs::transfer::MsgTransfer as IbcM
use namada_core::ibc::apps::transfer::types::packet::PacketData;
use namada_core::masp::{addr_taddr, encode_asset_type, ibc_taddr, MaspEpoch};
use namada_core::storage::Key;
use namada_gas::GasMetering;
use namada_governance::storage::is_proposal_accepted;
use namada_ibc::core::channel::types::commitment::{
compute_packet_commitment, PacketCommitment,
Expand All @@ -43,6 +42,7 @@ use namada_sdk::masp::TAddrData;
use namada_state::{ConversionState, OptionExt, ResultExt, StateRead};
use namada_token::read_denom;
use namada_token::validation::verify_shielded_tx;
use namada_tx::action::Read;
use namada_tx::BatchedTxRef;
use namada_vp_env::VpEnv;
use thiserror::Error;
Expand Down Expand Up @@ -687,33 +687,26 @@ where
.read_post(&counterpart_balance_key)?
.unwrap_or_default();
// Public keys must be the hash of the sources/targets
let address_hash = addr_taddr(counterpart.clone());
let addr_hash = addr_taddr(counterpart.clone());
// Enable the decoding of these counterpart addresses
result
.decoder
.insert(address_hash, TAddrData::Addr(counterpart.clone()));
.insert(addr_hash, TAddrData::Addr(counterpart.clone()));
let zero = ValueSum::zero();
// Finally record the actual balance change starting with the initial
// state
let pre_entry = result
.pre
.get(&address_hash)
.cloned()
.unwrap_or(ValueSum::zero());
let pre_entry = result.pre.get(&addr_hash).unwrap_or(&zero).clone();
result.pre.insert(
address_hash,
addr_hash,
checked!(
pre_entry + &ValueSum::from_pair((*token).clone(), pre_balance)
)
.map_err(native_vp::Error::new)?,
);
// And then record thee final state
let post_entry = result
.post
.get(&address_hash)
.cloned()
.unwrap_or(ValueSum::zero());
// And then record the final state
let post_entry = result.post.get(&addr_hash).cloned().unwrap_or(zero);
result.post.insert(
address_hash,
addr_hash,
checked!(
post_entry
+ &ValueSum::from_pair((*token).clone(), post_balance)
Expand Down Expand Up @@ -757,6 +750,7 @@ where
&self,
tx_data: &BatchedTxRef<'_>,
keys_changed: &BTreeSet<Key>,
verifiers: &BTreeSet<Address>,
) -> Result<()> {
let masp_epoch_multiplier =
namada_parameters::read_masp_epoch_multiplier_parameter(
Expand All @@ -771,6 +765,7 @@ where
})?;
let conversion_state = self.ctx.state.in_mem().get_conversion_state();
let ibc_msg = self.ctx.get_ibc_message(tx_data).ok();
let actions = self.ctx.read_actions()?;
let shielded_tx =
if let Some(IbcMessage::Envelope(ref envelope)) = ibc_msg {
extract_masp_tx_from_envelope(envelope).ok_or_else(|| {
Expand All @@ -781,7 +776,7 @@ where
} else {
// Get the Transaction object from the actions
let masp_section_ref =
namada_tx::action::get_masp_section_ref(&self.ctx)?
namada_tx::action::get_masp_section_ref(&actions)
.ok_or_else(|| {
native_vp::Error::new_const(
"Missing MASP section reference in action",
Expand Down Expand Up @@ -809,27 +804,29 @@ where
}

// Check the validity of the keys and get the transfer data
let mut changed_balances =
let changed_balances =
self.validate_state_and_get_transfer_data(keys_changed, ibc_msg)?;

// Some constants that will be used repeatedly
let zero = ValueSum::zero();
let masp_address_hash = addr_taddr(MASP);
verify_sapling_balancing_value(
changed_balances
.pre
.get(&masp_address_hash)
.unwrap_or(&ValueSum::zero()),
.unwrap_or(&zero),
changed_balances
.post
.get(&masp_address_hash)
.unwrap_or(&ValueSum::zero()),
.unwrap_or(&zero),
&shielded_tx.sapling_value_balance(),
masp_epoch,
&changed_balances.tokens,
conversion_state,
)?;

// The set of addresses that are required to authorize this transaction
let mut signers = BTreeSet::new();
let mut authorizers = BTreeSet::new();

// Checks on the sapling bundle
// 1. The spend descriptions' anchors are valid
Expand All @@ -845,32 +842,56 @@ where
self.valid_note_commitment_update(&shielded_tx)?;

// Checks on the transparent bundle, if present
let mut changed_bals_minus_txn = changed_balances.clone();
validate_transparent_bundle(
&shielded_tx,
&mut changed_balances,
&mut changed_bals_minus_txn,
masp_epoch,
conversion_state,
&mut signers,
&mut authorizers,
)?;

// Ensure that every account for which balance has gone down has
// authorized this transaction
for (addr, pre) in changed_balances.pre {
if changed_balances
.post
.get(&addr)
.unwrap_or(&ValueSum::zero())
< &pre
&& addr != masp_address_hash
// Ensure that every account for which balance has gone down as a result
// of the Transaction has authorized this transaction
for (addr, minus_txn_pre) in changed_bals_minus_txn.pre {
// The pre-balance seen by all VPs including this one
let pre = changed_balances.pre.get(&addr).unwrap_or(&zero);
// The post-balance seen by all VPs including this one
let post = changed_balances.post.get(&addr).unwrap_or(&zero);
// The post-balance if the effects of the Transaction are removed
let minus_txn_post =
changed_bals_minus_txn.post.get(&addr).unwrap_or(&zero);
// Never require a signature from the MASP VP
if addr != masp_address_hash &&
// Only require further authorization if without the Transaction,
// this Tx would decrease the balance of this address
minus_txn_post < &minus_txn_pre &&
// Only require further authorization from this address if the
// Transaction alters its balance
(minus_txn_pre, minus_txn_post) != (pre.clone(), post)
{
signers.insert(addr);
// This address will need to provide further authorization
authorizers.insert(addr);
}
}

let mut actions_authorizers: HashSet<&Address> = actions
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actions are actually a vector of Action, so it's possible to push the same action more than once. I believe from the perspective of the masp vp this is of no importance: we just want to check that the action for a specific authorizer has been written, it doesn't matter if it's duplicated

.iter()
.filter_map(|action| {
if let namada_tx::action::Action::Masp(
namada_tx::action::MaspAction::MaspAuthorizer(addr),
) = action
{
Some(addr)
} else {
None
}
})
.collect();
// Ensure that this transaction is authorized by all involved parties
for signer in signers {
for authorizer in authorizers {
if let Some(TAddrData::Addr(IBC)) =
changed_balances.decoder.get(&signer)
changed_bals_minus_txn.decoder.get(&authorizer)
{
// If the IBC address is a signatory, then it means that either
// Tx - Transaction(s) causes a decrease in the IBC balance or
Expand All @@ -887,7 +908,7 @@ where
if let Some(transp_bundle) = shielded_tx.transparent_bundle() {
for vout in transp_bundle.vout.iter() {
if let Some(TAddrData::Ibc(_)) =
changed_balances.decoder.get(&vout.address)
changed_bals_minus_txn.decoder.get(&vout.address)
{
let error = native_vp::Error::new_const(
"Simultaneous credit and debit of IBC account \
Expand All @@ -900,39 +921,53 @@ where
}
}
} else if let Some(TAddrData::Addr(signer)) =
changed_balances.decoder.get(&signer)
changed_bals_minus_txn.decoder.get(&authorizer)
{
// Otherwise the signer must be decodable so that we can
// manually check the signatures
let public_keys_index_map =
crate::account::public_keys_index_map(
&self.ctx.pre(),
signer,
)?;
let threshold =
crate::account::threshold(&self.ctx.pre(), signer)?
.unwrap_or(1);
let mut gas_meter = self.ctx.gas_meter.borrow_mut();
tx_data
.tx
.verify_signatures(
&[tx_data.tx.raw_header_hash()],
public_keys_index_map,
&Some(signer.clone()),
threshold,
|| gas_meter.consume(crate::gas::VERIFY_TX_SIG_GAS),
)
.map_err(native_vp::Error::new)?;
// Otherwise the owner's vp must have been triggered and the
// relative action must have been written
if !verifiers.contains(signer) {
Copy link
Contributor

@murisi murisi Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would almost certainly agree with this logic if signer's VP was being executed on the state change being checked by this MASP VP minus the state changes induced by the inner Transaction. My concern here is as follows: imagine Alice constructs an unshielding Transaction of 10 BTC to Bertha. Now imagine that Christel intercepts this Transaction and embeds it inside a Transfer Tx that increases the balance of Christel by 10 BTC (instead of Bertha who is the rightful recipient). In this case, since Tx is effectively two transfers (an unshielding Transaction of 10 BTC to Bertha followed by a transparent Transfer of 10 BTC from Bertha to Christel), the signers map would contain Bertha's address. My specific concern here is that Bertha's VP doesn't see this foul play (of rehousing the Transaction in a different Tx) and always accepts the state change because the net change to Bertha's balance as a result of the entire state change is zero (after all, Bertha is only mentioned in the transparent output of the Transaction). To me this approach would only make sense if the verifiers could be made to specifically validate just the "effective" Transfer of 10 BTC from Bertha to Christel (which would be the result of doing the above "subtraction"), in which case they would certainly demand an authorization signature from Bertha (as was being done before in an admittedly restrictive way). Am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this PR only removes the signature verification and requests the corresponding VP to be triggered. As you are saying though, Bertha's VP would not reject the tx in the attack scenario you described because the balance change is 0. To make this work I've introduced a new masp Action (i.e. a temporary write to write log that is not committed to storage) with the address of the verifier whose vp we want to check this tx (in this case Bertha). Again, the 0 balance change would lead to the tx being accepted, so on top of that, I've updated the implicit and user VPs to handle this Action with a signature check (but custom vps could add more checks if desired): so the signature check has been simply moved from the masp vp to the user and implicit vps.

For this logic to work we need some cooperation from the tx which must write the temporary action key(s) required (the ones carrying the addresses whose extra signatures have been attached to the tx).

I believe this logic is ok (but please double check again) but I think that there are other flaws in this this implementation of mine:

  1. Even though the Action carries the address of the signer this does not trigger its vp, since it's not the leading part of the key and we don't trigger VPs anymore if their address appear in non-leading locations of a key (@tzemanovic please correct me if I'm wrong). In this case we need a way for the tx to trigger the required VPs
  2. Checking that the VPs have been triggered is not enough. Someone could craft a tx that does not write the required actions but for which the relevant VPs are still triggered. In this case we could end up in the scenario you envisioned. On top of checking that VPs have been triggered we should also check that the corresponding Action has been written to storage to ensure that the VP will run the correct validation

Note that, as I've already answered Tomas before, at the moment the transactions do not support this Action logic cause with the previous design of vectorized transfers these scenario could not happen. It will become relevant after the new vectorized strategy has been merged

Copy link
Member

@tzemanovic tzemanovic Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a single transfer I think we would need do something like this:

  1. If a VP of an address who must authorize the tx is not going to be triggered from the changed keys (yes, it is only the leading part) then the tx has to insert_verifier
  2. The MASP VP should require an Action to be written so a tx is not allowed to skip it (i.e. actions must not be empty)

For these to work together the MASP VP should then ensure that when the action is MaspSigner then the address in this action is contained in the verifiers set.

However, with the scenario with multiple transfers @murisi described I think this may not be sufficient. A tx could only attach a single action despite performing multiple actions. We might then need to add more information to the actions to describe the transfers in more detail and the MASP and user/implicit VPs will have to enforce that the storage changes match these actions. In this example, for the unshielding action from Alice we could have e.g. Action::Unshield { dest, amount } where dest == Bertha`` and if Christel` was to maliciously embed this transfer in another tx, either:

  • Only with the original action, an increase in Christel's balance would not be matching the action (the MASP VP has to check the transparent bundle's vout.address against the actions to enforce this)
  • With the original action and added Action::Transparent { src, dest, amount } where src == Bertha, dest == Christel and amount == 10, the user/implicit VP must require signature from Bertha (even if the overall balance change of Bertha is 0)
  • Without the original action, but with an Action::Unshield where dest == Christel, the side-effects of the storage changes would not match the actions (the actual unshielding dest is Bertha)

Copy link
Contributor

@murisi murisi Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this PR only removes the signature verification and requests the corresponding VP to be triggered.

Ah, I completely misread the approach taken by this PR. My bad.

but custom vps could add more checks if desired

I see. And at the bare minimum, custom VPs must not forget to handle MASP Actions.

In this case we need a way for the tx to trigger the required VPs

Nice, I see you've now covered this case.

On top of checking that VPs have been triggered we should also check that the corresponding Action has been written to storage to ensure that the VP will run the correct validation

Nice, I see you've also covered this case.

Copy link
Contributor

@murisi murisi Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a single transfer I think we would need do something like this:

1. If a VP of an address who must authorize the tx is not going to be triggered from the changed keys (yes, it is only the leading part) then the tx has to `insert_verifier`

2. The MASP VP should require an `Action` to be written so a tx is not allowed to skip it (i.e. actions must not be empty)

For these to work together the MASP VP should then ensure that when the action is MaspSigner then the address in this action is contained in the verifiers set.

At least in the current PR, this is all covered right? The current conditional forces the presence of the signer in the verifiers list. The next conditional forces the presence of the corresponding MaspSigner Action.

However, with the scenario with multiple transfers @murisi described I think this may not be sufficient.

The signer variable in this loop takes on as its value each of the Addresses whose balances are debited during processing (even if their net change is zero), so if this verifier check and the following MaspSigner check is correct then this VP should prevent Christel from committing thievery (since the Tx would be forced to trigger the signature checks elsewhere). Right?

let error = native_vp::Error::new_alloc(format!(
"The required vp of address {signer} was not triggered"
))
.into();
tracing::debug!("{error}");
return Err(error);
}

// The action is required becuse the target vp might have been
// triggered for other reasons but we need to signal it that it
// is required to validate a discrepancy in its balance change
// because of a masp transaction, which might require a
// different validation than a normal balance change
if !actions_authorizers.swap_remove(signer) {
let error = native_vp::Error::new_alloc(format!(
"The required masp authorizer action for address \
{signer} is missing"
))
.into();
tracing::debug!("{error}");
return Err(error);
}
} else {
// We are not able to decode the signer, so just fail
// We are not able to decode the authorizer, so just fail
let error = native_vp::Error::new_const(
"Unable to decode a transaction signer",
"Unable to decode a transaction authorizer",
)
.into();
tracing::debug!("{error}");
return Err(error);
}
}
// The transaction shall not push masp authorizer actions that are not
// needed cause this might lead vps to run a wrong validation logic
if !actions_authorizers.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transaction shall not push masp authorizer actions that are not needed

This is fine, though it means that transactions need to precisely compute/maintain the required set of authorizers even when the logic at

namada/crates/namada/src/ledger/native_vp/masp.rs

Line 874 in fc63450

authorizers.insert(addr);
is triggered. Theoretically this set might also contain addresses that are not in the Transaction transparent inputs and hence may be inconvenient to compute and push in transactions.

Copy link
Contributor Author

@grarco grarco Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, there's some burden on the transaction side to match the set of verifiers. But eventually the transaction has to either match the set of authorizers required by the vps exactly or match a superset of it (in case we wanted to relax this condition). The only thing I can think of to ease the computation on the transaction side would be to push actions for all the involved parties (which need to be computed anyway since, as you are saying, they might not be limited to the transparent inputs). This would lead to an increase in gas cost given by the extra (unneeded) signatures attached to the transaction and by the cost of their verification: at the end this might outweigh the added gas cost for the logic to be placed in the transfer transaction. Another issue is that collecting some of these signatures could be complicated and make the ux more unpleasant

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might lead vps to run a wrong validation logic

I'd assumed that redundant MASP authorizer actions only imply signature checks for redundant signatures. In a correctly implemented VP, is this not necessarily the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For our VPs, that's correct, we'd just recheck the signature (which actually doesn't even happen because of the Gadget that caches the result of the signature check and ensures that we run that verification at most once). I'm not sure this holds for incorrectly VPs though: I believe the worst that could happen was the rejection of a valid transaction but not the acceptance of an invalid one. But some really broken VPs could actually end up accepting invalid transactions because of masp actions. So my reasoning here was to be very conservative, also because there's really no reason why someone should push a masp action which isn't required

let error = native_vp::Error::new_const(
"Found masp authorizer actions that are not required",
)
.into();
tracing::debug!("{error}");
return Err(error);
}

// Verify the proofs
verify_shielded_tx(&shielded_tx, |gas| self.ctx.charge_gas(gas))
Expand All @@ -957,18 +992,17 @@ fn unepoched_tokens(
Ok(())
}

// Handle transparent input
fn validate_transparent_input<A: Authorization>(
vin: &TxIn<A>,
changed_balances: &mut ChangedBalances,
transparent_tx_pool: &mut I128Sum,
epoch: MaspEpoch,
conversion_state: &ConversionState,
signers: &mut BTreeSet<TransparentAddress>,
authorizers: &mut BTreeSet<TransparentAddress>,
) -> Result<()> {
// A decrease in the balance of an account needs to be
// authorized by the account of this transparent input
signers.insert(vin.address);
authorizers.insert(vin.address);
// Non-masp sources add to the transparent tx pool
*transparent_tx_pool = transparent_tx_pool
.checked_add(
Expand Down Expand Up @@ -1044,7 +1078,6 @@ fn validate_transparent_input<A: Authorization>(
Ok(())
}

// Handle transparent output
fn validate_transparent_output(
out: &TxOut,
changed_balances: &mut ChangedBalances,
Expand Down Expand Up @@ -1116,7 +1149,7 @@ fn validate_transparent_bundle(
changed_balances: &mut ChangedBalances,
epoch: MaspEpoch,
conversion_state: &ConversionState,
signers: &mut BTreeSet<TransparentAddress>,
authorizers: &mut BTreeSet<TransparentAddress>,
) -> Result<()> {
// The Sapling value balance adds to the transparent tx pool
let mut transparent_tx_pool = shielded_tx.sapling_value_balance();
Expand All @@ -1129,7 +1162,7 @@ fn validate_transparent_bundle(
&mut transparent_tx_pool,
epoch,
conversion_state,
signers,
authorizers,
)?;
}

Expand Down Expand Up @@ -1253,7 +1286,7 @@ where
&self,
tx_data: &BatchedTxRef<'_>,
keys_changed: &BTreeSet<Key>,
_verifiers: &BTreeSet<Address>,
verifiers: &BTreeSet<Address>,
) -> Result<()> {
let masp_keys_changed: Vec<&Key> =
keys_changed.iter().filter(|key| is_masp_key(key)).collect();
Expand Down Expand Up @@ -1283,7 +1316,7 @@ where
self.is_valid_parameter_change(tx_data)
} else if masp_transfer_changes {
// The MASP transfer keys can only be changed by a valid Transaction
self.is_valid_masp_transfer(tx_data, keys_changed)
self.is_valid_masp_transfer(tx_data, keys_changed, verifiers)
} else {
// Changing no MASP keys at all is also fine
Ok(())
Expand Down
11 changes: 7 additions & 4 deletions crates/namada/src/ledger/protocol/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,9 +415,10 @@ where
if is_accepted {
// If the transaction was a masp one append the
// transaction refs for the events
let actions =
state.read_actions().map_err(Error::StateError)?;
if let Some(masp_section_ref) =
namada_tx::action::get_masp_section_ref(state)
.map_err(Error::StateError)?
namada_tx::action::get_masp_section_ref(&actions)
{
extended_tx_result
.masp_tx_refs
Expand Down Expand Up @@ -740,8 +741,10 @@ where
);
}

let masp_ref = namada_tx::action::get_masp_section_ref(*state)
.map_err(Error::StateError)?;
let actions =
state.read_actions().map_err(Error::StateError)?;
let masp_ref =
namada_tx::action::get_masp_section_ref(&actions);
// Ensure that the transaction is actually a masp one, otherwise
// reject
(is_masp_transfer(&result.changed_keys) && result.is_accepted())
Expand Down
Loading
Loading