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

component: eliminate TOCTOU bug factory #3962

Merged
merged 3 commits into from
Mar 7, 2024
Merged

component: eliminate TOCTOU bug factory #3962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f08418
component: eliminate TOCTOU bug factory
hdevalence Mar 6, 2024
2949f8f
component: remove TOCTOU from AppActionHandler
hdevalence Mar 6, 2024
6f59134
app: remove redundant cross-action checks
hdevalence Mar 7, 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
38 changes: 25 additions & 13 deletions crates/cnidarium-component/src/action_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,33 +44,45 @@ pub trait ActionHandler {
/// be run in parallel across all transactions in a block.
async fn check_stateless(&self, context: Self::CheckStatelessContext) -> Result<()>;

/// Performs all of this action's stateful validity checks.
/// Performs those stateful validity checks that can be performed against a
/// historical state snapshot.
///
/// This method provides read access to a snapshot of the `State` prior to
/// transaction execution. Because it does not give write access, it's safe
/// to run the `ActionHandler` implementations for all actions in a given
/// transaction in parallel.
/// transaction execution. It is intended to be run in parallel across all
/// actions within a transaction.
///
/// As much work as possible should be done in `check_stateful`, as it can
/// be run in parallel across all actions within a transaction.
/// # Warning
///
/// Misuse of this method creates TOCTOU vulnerabilities. Checks performed
/// in this method must be valid if they are performed against a _prior_
/// state, as another action in the same transaction may execute first and
/// change the state.
///
/// Checks performed in this phase should have a justification for why they
/// are safe to run in parallel with other actions in the same transaction,
/// and the default behavior should be to perform checks in
/// [`ActionHandler::check_and_execute`].
cratelyn marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Invariants
///
/// This method should only be called on data that has been checked
/// with [`ActionHandler::check_stateless`]. This method can be called
/// before [`Component::begin_block`](crate::Component::begin_block).
async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()>;
async fn check_historical<S: StateRead + 'static>(&self, _state: Arc<S>) -> Result<()> {
// Default behavior: no-op
Ok(())
}

/// Attempts to execute this action against the provided `state`.
///
/// This method provides read and write access to the `state`. It is
/// fallible, so it's possible to perform checks within the `execute`
/// fallible, so it's possible to perform checks within the `check_and_execute`
/// implementation and abort execution on error; the [`StateTransaction`]
/// mechanism ensures that all writes are correctly discarded.
///
/// Because `execute` must run sequentially, whenever possible, checks
/// should be performed in [`ActionHandler::check_stateful`] or
/// [`ActionHandler::check_stateless`]. One example of where this is not
/// should be performed in [`ActionHandler::check_stateless`], or (more carefully) in
/// [`ActionHandler::check_historical`]. One example of where this is not
/// possible (in fact, the motivating example) is for IBC, where a
/// transaction may (1) submit a client update and then (2) relay messages
/// valid relative to the newly updated state. In this case, the checks for
Expand All @@ -81,8 +93,8 @@ pub trait ActionHandler {
///
/// # Invariants
///
/// This method should only be called immediately after an invocation of
/// [`ActionHandler::check_stateful`] on the same transaction. This method
/// This method should only be called after an invocation of
/// [`ActionHandler::check_historical`] on the same transaction. This method
/// can be called before [`Component::begin_block`](crate::Component::begin_block).
async fn execute<S: StateWrite>(&self, state: S) -> Result<()>;
async fn check_and_execute<S: StateWrite>(&self, state: S) -> Result<()>;
}
24 changes: 16 additions & 8 deletions crates/core/app/src/action_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,26 +16,34 @@ mod transaction;
/// duplicate the trait here and there, and provide a generic impl of this trait
/// for anything implementing the copy of the trait in the other crate. Later,
/// we can delete this trait entirely.
///
/// Currently, there are only three impls, all of which are entangled with app-level data:
///
/// - ProposalSubmit (which is entangled with the whole-application state)
/// - Action (which needs to slot in the PenumbraHost for IBC action handling)
/// - Transaction (which depends on the above)
#[async_trait]
pub trait ActionHandler {
pub trait AppActionHandler {
cratelyn marked this conversation as resolved.
Show resolved Hide resolved
type CheckStatelessContext: Clone + Send + Sync + 'static;
async fn check_stateless(&self, context: Self::CheckStatelessContext) -> Result<()>;
async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()>;
async fn execute<S: StateWrite>(&self, state: S) -> Result<()>;
async fn check_historical<S: StateRead + 'static>(&self, _state: Arc<S>) -> Result<()> {
return Ok(());
}
cratelyn marked this conversation as resolved.
Show resolved Hide resolved
async fn check_and_execute<S: StateWrite>(&self, state: S) -> Result<()>;
}

use cnidarium_component::ActionHandler as ComponentActionHandler;

#[async_trait]
impl<'a, T: ComponentActionHandler + Sync> ActionHandler for crate::Compat<'a, T> {
impl<'a, T: ComponentActionHandler + Sync> AppActionHandler for crate::Compat<'a, T> {
type CheckStatelessContext = T::CheckStatelessContext;
async fn check_stateless(&self, context: Self::CheckStatelessContext) -> Result<()> {
ComponentActionHandler::check_stateless(self.0, context).await
}
async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()> {
ComponentActionHandler::check_stateful(self.0, state).await
async fn check_historical<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()> {
ComponentActionHandler::check_historical(self.0, state).await
}
async fn execute<S: StateWrite>(&self, state: S) -> Result<()> {
ComponentActionHandler::execute(self.0, state).await
async fn check_and_execute<S: StateWrite>(&self, state: S) -> Result<()> {
ComponentActionHandler::check_and_execute(self.0, state).await
}
}
90 changes: 46 additions & 44 deletions crates/core/app/src/action_handler/actions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ mod submit;

use crate::PenumbraHost;

use super::ActionHandler;
use super::AppActionHandler;
use cnidarium_component::ActionHandler as _;

#[async_trait]
impl ActionHandler for Action {
impl AppActionHandler for Action {
type CheckStatelessContext = TransactionContext;

async fn check_stateless(&self, context: TransactionContext) -> Result<()> {
Expand Down Expand Up @@ -53,25 +53,27 @@ impl ActionHandler for Action {
}
}

async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()> {
async fn check_historical<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()> {
match self {
Action::Delegate(action) => action.check_stateful(state).await,
Action::Undelegate(action) => action.check_stateful(state).await,
Action::UndelegateClaim(action) => action.check_stateful(state).await,
Action::ValidatorDefinition(action) => action.check_stateful(state).await,
Action::DelegatorVote(action) => action.check_stateful(state).await,
Action::ValidatorVote(action) => action.check_stateful(state).await,
Action::PositionClose(action) => action.check_stateful(state).await,
Action::PositionOpen(action) => action.check_stateful(state).await,
Action::PositionWithdraw(action) => action.check_stateful(state).await,
Action::ProposalSubmit(action) => action.check_stateful(state).await,
Action::ProposalWithdraw(action) => action.check_stateful(state).await,
Action::ProposalDepositClaim(action) => action.check_stateful(state).await,
Action::Swap(action) => action.check_stateful(state).await,
Action::SwapClaim(action) => action.check_stateful(state).await,
Action::Spend(action) => action.check_stateful(state).await,
Action::Output(action) => action.check_stateful(state).await,
Action::Delegate(action) => action.check_historical(state).await,
Action::Undelegate(action) => action.check_historical(state).await,
Action::UndelegateClaim(action) => action.check_historical(state).await,
Action::ValidatorDefinition(action) => action.check_historical(state).await,
Action::DelegatorVote(action) => action.check_historical(state).await,
Action::ValidatorVote(action) => action.check_historical(state).await,
Action::PositionClose(action) => action.check_historical(state).await,
Action::PositionOpen(action) => action.check_historical(state).await,
Action::PositionWithdraw(action) => action.check_historical(state).await,
Action::ProposalSubmit(action) => action.check_historical(state).await,
Action::ProposalWithdraw(action) => action.check_historical(state).await,
Action::ProposalDepositClaim(action) => action.check_historical(state).await,
Action::Swap(action) => action.check_historical(state).await,
Action::SwapClaim(action) => action.check_historical(state).await,
Action::Spend(action) => action.check_historical(state).await,
Action::Output(action) => action.check_historical(state).await,
Action::IbcRelay(action) => {
// SAFETY: this is safe to check in parallel because IBC enablement cannot
// change during transaction execution.
if !state.get_ibc_params().await?.ibc_enabled {
anyhow::bail!("transaction contains IBC actions, but IBC is not enabled");
}
Expand All @@ -82,42 +84,42 @@ impl ActionHandler for Action {
.check_stateful(state)
.await
}
Action::Ics20Withdrawal(action) => action.check_stateful(state).await,
Action::CommunityPoolSpend(action) => action.check_stateful(state).await,
Action::CommunityPoolOutput(action) => action.check_stateful(state).await,
Action::CommunityPoolDeposit(action) => action.check_stateful(state).await,
Action::Ics20Withdrawal(action) => action.check_historical(state).await,
Action::CommunityPoolSpend(action) => action.check_historical(state).await,
Action::CommunityPoolOutput(action) => action.check_historical(state).await,
Action::CommunityPoolDeposit(action) => action.check_historical(state).await,
}
}

async fn execute<S: StateWrite>(&self, state: S) -> Result<()> {
async fn check_and_execute<S: StateWrite>(&self, state: S) -> Result<()> {
match self {
Action::Delegate(action) => action.execute(state).await,
Action::Undelegate(action) => action.execute(state).await,
Action::UndelegateClaim(action) => action.execute(state).await,
Action::ValidatorDefinition(action) => action.execute(state).await,
Action::DelegatorVote(action) => action.execute(state).await,
Action::ValidatorVote(action) => action.execute(state).await,
Action::PositionClose(action) => action.execute(state).await,
Action::PositionOpen(action) => action.execute(state).await,
Action::PositionWithdraw(action) => action.execute(state).await,
Action::ProposalSubmit(action) => action.execute(state).await,
Action::ProposalWithdraw(action) => action.execute(state).await,
Action::ProposalDepositClaim(action) => action.execute(state).await,
Action::Swap(action) => action.execute(state).await,
Action::SwapClaim(action) => action.execute(state).await,
Action::Spend(action) => action.execute(state).await,
Action::Output(action) => action.execute(state).await,
Action::Delegate(action) => action.check_and_execute(state).await,
Action::Undelegate(action) => action.check_and_execute(state).await,
Action::UndelegateClaim(action) => action.check_and_execute(state).await,
Action::ValidatorDefinition(action) => action.check_and_execute(state).await,
Action::DelegatorVote(action) => action.check_and_execute(state).await,
Action::ValidatorVote(action) => action.check_and_execute(state).await,
Action::PositionClose(action) => action.check_and_execute(state).await,
Action::PositionOpen(action) => action.check_and_execute(state).await,
Action::PositionWithdraw(action) => action.check_and_execute(state).await,
Action::ProposalSubmit(action) => action.check_and_execute(state).await,
Action::ProposalWithdraw(action) => action.check_and_execute(state).await,
Action::ProposalDepositClaim(action) => action.check_and_execute(state).await,
Action::Swap(action) => action.check_and_execute(state).await,
Action::SwapClaim(action) => action.check_and_execute(state).await,
Action::Spend(action) => action.check_and_execute(state).await,
Action::Output(action) => action.check_and_execute(state).await,
Action::IbcRelay(action) => {
action
.clone()
.with_handler::<Ics20Transfer, PenumbraHost>()
.execute(state)
.await
}
Action::Ics20Withdrawal(action) => action.execute(state).await,
Action::CommunityPoolSpend(action) => action.execute(state).await,
Action::CommunityPoolOutput(action) => action.execute(state).await,
Action::CommunityPoolDeposit(action) => action.execute(state).await,
Action::Ics20Withdrawal(action) => action.check_and_execute(state).await,
Action::CommunityPoolSpend(action) => action.check_and_execute(state).await,
Action::CommunityPoolOutput(action) => action.check_and_execute(state).await,
Action::CommunityPoolDeposit(action) => action.check_and_execute(state).await,
}
}
}
30 changes: 20 additions & 10 deletions crates/core/app/src/action_handler/actions/submit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::str::FromStr;
use std::sync::Arc;

use anyhow::{Context, Result};
use ark_ff::PrimeField;
Expand All @@ -9,7 +8,7 @@ use decaf377_rdsa::{VerificationKey, VerificationKeyBytes};
use ibc_types::core::client::ClientId;
use once_cell::sync::Lazy;

use cnidarium::{StateDelta, StateRead, StateWrite};
use cnidarium::StateWrite;
use penumbra_asset::STAKING_TOKEN_DENOM;
use penumbra_community_pool::component::StateReadExt as _;
use penumbra_governance::{
Expand All @@ -27,7 +26,7 @@ use penumbra_sct::component::tree::SctRead;
use penumbra_shielded_pool::component::SupplyWrite;
use penumbra_transaction::{AuthorizationData, Transaction, TransactionPlan, WitnessData};

use crate::action_handler::ActionHandler;
use crate::action_handler::AppActionHandler;
use crate::community_pool_ext::CommunityPoolStateWriteExt;
use crate::params::AppParameters;

Expand All @@ -45,7 +44,7 @@ pub const PROPOSAL_TITLE_LIMIT: usize = 80; // ⚠️ DON'T CHANGE THIS (see abo
pub const PROPOSAL_DESCRIPTION_LIMIT: usize = 10_000; // ⚠️ DON'T CHANGE THIS (see above)!

#[async_trait]
impl ActionHandler for ProposalSubmit {
impl AppActionHandler for ProposalSubmit {
type CheckStatelessContext = ();
async fn check_stateless(&self, _context: ()) -> Result<()> {
let ProposalSubmit {
Expand Down Expand Up @@ -147,7 +146,11 @@ impl ActionHandler for ProposalSubmit {
Ok(())
}

async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()> {
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
// These checks all formerly happened in the `check_historical` method,
// if profiling shows that they cause a bottleneck we could (CAREFULLY)
// move some of them back.

let ProposalSubmit {
deposit_amount,
proposal, // statelessly verified
Expand Down Expand Up @@ -194,6 +197,8 @@ impl ActionHandler for ProposalSubmit {
// current chain state. This doesn't guarantee that it will execute successfully at
// the time when the proposal passes, but we don't want to allow proposals that are
// obviously going to fail to execute.
//
// NOTE: we do not do stateful checks, see below
let parsed_transaction_plan = TransactionPlan::decode(&transaction_plan[..])
.context("transaction plan was malformed")?;
let tx = build_community_pool_transaction(parsed_transaction_plan.clone())
Expand All @@ -202,12 +207,19 @@ impl ActionHandler for ProposalSubmit {
tx.check_stateless(()).await.context(
"submitted Community Pool spend transaction failed stateless checks",
)?;
tx.check_stateful(state.clone())
/*
// We skip stateful checks rather than doing them in simulation. Partly this is
// because it's easier to not check, but also it avoids having to reason about whether
// there are any cases where a transaction could be invalid when submitted but become
// valid when voting finishes (e.g., an undelegation?)

tx.check_historical(state.clone())
.await
.context("submitted Community Pool spend transaction failed stateful checks")?;
tx.execute(StateDelta::new(state)).await.context(
tx.check_and_execute(StateDelta::new(state)).await.context(
"submitted Community Pool spend transaction failed to execute in current chain state",
)?;
*/
}
ProposalPayload::UpgradePlan { .. } => {
// TODO(erwan): no stateful checks for upgrade plan.
Expand All @@ -230,10 +242,8 @@ impl ActionHandler for ProposalSubmit {
}
}

Ok(())
}
// (end of former check_stateful checks)

async fn execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
let ProposalSubmit {
proposal,
deposit_amount,
Expand Down
Loading
Loading