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

Remove TOCTOU hazards by moving all checks into execute and renaming the Component methods #3960

Closed
4 tasks
hdevalence opened this issue Mar 5, 2024 · 1 comment
Closed
4 tasks

Remove TOCTOU hazards by moving all checks into execute and renaming the Component methods #3960

hdevalence opened this issue Mar 5, 2024 · 1 comment
Assignees
@hdevalence
Labels
zellic-component-needs-remediation Bug reports by zellic

Comments

@hdevalence
Copy link
Member

Describe the bug

Zellic observed many TOCTOU bugs in our ActionHandler impls. These all have a common cause: the aggressive use of check_stateful, which does parallel checks against a snapshot of chain state prior to any execution. The problem is that this separates an action's checks from its execution.

We can eliminate this bug class by simply not doing this.

Expected behavior

  • All code currently in check_stateful impls shall be moved into the execute methods, unless there is a simple AND LOCAL explanation for why it is safe to perform the check against historical state.
  • All intra-action consistency checks shall be removed. For instance, we will not be checking that a transaction has duplicate nullifiers, because we will handle each action in isolation.
  • The current execute method shall be renamed check_and_execute.
  • The current check_stateful method shall be renamed check_historical, and the docs shall be updated to note that it is run in parallel with a snapshot of the chain state some time prior to execution of the action, that it presents a TOCTOU hazard, and that it should be avoided in favor of check_and_execute unless it is known to be safe.
@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra Mar 5, 2024
@hdevalence hdevalence mentioned this issue Mar 5, 2024
Closed
@cratelyn
Copy link
Contributor

cratelyn commented Mar 7, 2024

this was fixed in #3962! i believe this can be closed as completed now?

@github-project-automation github-project-automation bot moved this from 🗄️ Backlog to Done in Penumbra Mar 10, 2024
@erwanor erwanor added the zellic-component-needs-remediation Bug reports by zellic label Mar 25, 2024
@SuperFluffy SuperFluffy mentioned this issue Aug 1, 2024
Merged
github-merge-queue bot pushed a commit to astriaorg/astria that referenced this issue Aug 20, 2024
@SuperFluffy @Fraser999
fix(sequencer)!: fix TOCTOU issues by merging check and execution (#1332
9f959f4 
)

## Summary
Introduces `ActionHandler::check_and_execute`, replacing
`ActionHandler::check_stateful` and `ActionHandler::execute`.

## Background
Zelic found that separating execution into `check_stateful` and
`execute` lead to time-of-check-vs-time-of-use risks: while
`check_stateful` for two actions `A` and `B` might each pass, the
execution of action `A` might change the state such that a subsequent
execution of action `B` would now fail - or worse, lead to invalid
state.

This patch follows Penumbra (see issues linked at the bottom) in merging
them into one atomic operation (atomic in the sense that
`<Action>::check_and_execute` are run sequentially).

There is also a `ActionHandler::check_historic` trait method, which
however is currently not used. It is left for future work to go through
the individual checks and move them to `check_historic`, where
applicable (for example, checking address prefixes as long as changing
these is not possible after chain init).

## Changes
- change `ActionHandler` trait to merge `check_stateful` and `execute`
into `check_and_execute`.
- inject a transaction's signer into the ephemeral object store, setting
before and after a transaction is executed. Necessary because this
follows the `cnidarium_component::ActionHandler` convention, but also
allows simplifying
- remove the notion of bech32m addresses from many state writes and
reads: the prefix only matters at the boundary, not inside the system

## Testing
All tests were updated and pass.

NOTE: a useful test would be to craft a problematic transaction that
would be rejected with the newest change. However, crafting and
executing such a transaction so that it is rejected by the current
sequencer but leads to incorrect is left to a follow-up.

## Breaking Changelist
While no snapshots guarding against breaking app state were triggered,
this is still a breaking change: if supplied with a problematic payload,
a sequencer node without this patch will reach a different (and invalid)
state compared a node with the present patch.

## Related Issues

Original Penumbra issues and fix:
penumbra-zone/penumbra#3960
penumbra-zone/penumbra#3960

Closes #1318

---------

Co-authored-by: Fraser Hutchison <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hdevalence hdevalence

Labels
zellic-component-needs-remediation Bug reports by zellic
Projects
Penumbra
Archived in project
Milestone
No milestone
Development

No branches or pull requests
3 participants
@hdevalence @erwanor @cratelyn