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.

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

Attacks possible if we always accept non-adjacent headers #79

Closed
Wondertan opened this issue Jul 10, 2023 · 5 comments · Fixed by #90
Closed

Attacks possible if we always accept non-adjacent headers #79

Wondertan opened this issue Jul 10, 2023 · 5 comments · Fixed by #90

Comments

@Wondertan
Copy link
Member

Wondertan commented Jul 10, 2023

We were talking about Rollkit implications with @nashqueue and found a few attacks with the current solution:

  • Message amplification. We accept all non-adjacent headers as valid, and everyone gossips them.
  • DoS. We don't have any top limit on height for accepted non-adjacent headers, so attackers can fill up the whole network through message amplification until OOM
  • Message amplification. Nodes will retry indefinitely to reach the non-adjacent header(sync target) by requesting connected peers.
@liamsi
Copy link
Member

liamsi commented Jul 10, 2023

We accept all non-adjacent headers as valid, and everyone gossips them.

Can we not wait to gossip them until verified via adjacent verification (or if that does not check out, they won't be gossiped at all)?

We don't have any top limit on height for accepted non-adjacent headers, so attackers can fill up the whole network through message amplification until OOM

Couldn't that be fixed by estimating a time/height for the time that has past from genesis?

@Wondertan
Copy link
Member Author

Wondertan commented Jul 11, 2023

Can we not wait to gossip them until verified via adjacent verification (or if that does not check out, they won't be gossiped at all)?

This is the simplest thing we can do. When we discussed this with @renaynay, we had a concern that the headers won't gossip because no one will regossip them, but in fact, all well-connected nodes will be synced up to the tip and will do adjacent verification to submit it further. Only nodes who didn't catch up to the tip won't reshare them, which I think is not bad and even intuitively feels right.

Couldn't that be fixed by estimating a time/height for the time that has past from genesis?

I think so, but I remember @musalbas had some concerns about block time assumptions not being realistic/precise. It might be that we don't need perfect precision in this case, and would like to hear your thoughts.

github-merge-queue bot pushed a commit to celestiaorg/celestia-node that referenced this issue Jul 11, 2023
This PR disables skipping verification and accepts all the incoming
non-adjacent headers. These headers will later be verified via adjacent
verification and rejected if invalid.

Test that proves that Syncer rejects and continues to work with a fault
header: celestiaorg/go-header#76

Potential outcomes of doing so:
celestiaorg/go-header#79

Additionally, the PR reverts the genesis hash change to the hash
pointing to height 1.

Supersedes #2449, which became zombie

---------

Co-authored-by: rene <[email protected]>
github-merge-queue bot pushed a commit to celestiaorg/celestia-node that referenced this issue Jul 12, 2023
This PR disables skipping verification and accepts all the incoming
non-adjacent headers. These headers will later be verified via adjacent
verification and rejected if invalid.

Test that proves that Syncer rejects and continues to work with a fault
header: celestiaorg/go-header#76

Potential outcomes of doing so:
celestiaorg/go-header#79

Additionally, the PR reverts the genesis hash change to the hash
pointing to height 1.

Supersedes #2449, which became zombie

---------

Co-authored-by: rene <[email protected]>
@renaynay
Copy link
Member

renaynay commented Jul 12, 2023

TODO (outcomes from meeting on 12.7.23)

  • Add a sanity check that compares untrusted incoming height against subjective head's height based on timestamp estimation (does this height seem like a valid sync target height) -- this check can be added inside the syncer as it can be used generically since syncer has concept of blocktime + trusting Period
  • Change trustingPeriod default to two weeks instead of 1 (recommendation but doesn't have to do w security)
  • Consider backwards sync as a prevention-mechanism for following a fork on sync (FUTURE)
  • Set subjective head as the canonical trusted "sync target" of the initial sync and DO NOT discard it if it cannot be applied to the "forked" chain that was synced -- this will prevent fork following until backwards sync is implemented.
  • Message amplification prevention inside headersub: do not ValidateAccept if it doesn't pass VerifyLightCommitTrusting, just ValidateIgnore, but if it does pass VLCT then it can ValidateAccept

@Wondertan
Copy link
Member Author

The default trusting period in celestia-core https://github.com/celestiaorg/celestia-core/blob/v0.34.x-celestia/cmd/cometbft/commands/light.go#L90.

@liamsi
Copy link
Member

liamsi commented Jul 12, 2023

  1. currently, if nodes start following a fork (outside of the unbonding period), they will discard the subjective head (which was given by a subjectively chosen trusted peer or the default baked in trusted peer in node). This is an issue that can easily be fixed by:

    • either by backwards-sync (which would be very time consuming), or

    • a simpler fix: once your fork passed the time or the height of the subjective header (inside of the unbonding period), you realize you are actually on a fork and you (store the fork as evidence and) halt; it’s important that the fork headers aren’t gossiped. Recovering from this requires you to restart the node, ideally blacklisting the peers that fooled you

  2. The concern about timing assumption are a non-issue as you would always reject headers that are subjective header + too far in the future (outside of the unbonding period but forward looking); no precision around block times is necessary here.

Wondertan added a commit that referenced this issue Aug 24, 2023
We realized that non-adjacent verification might produce false negatives in Tendermint consensus. Additionally, not all chains have non-adjacent verification, and before this change, we dropped all headers that failed the non-adjacency verification. However, we should give them a second chance and keep them as sync targets.

Additionally:
* Introduces typical verification flow. Users had to verify time, height, and chain-id themselves, but not anymore.
* We reject known headers, and it's a hard failure now.
* Non-adjacent verification failure is considered a soft failure.

Closes #79
0semter0 pushed a commit to 0semter0/celestia-node that referenced this issue Aug 12, 2024
This PR disables skipping verification and accepts all the incoming
non-adjacent headers. These headers will later be verified via adjacent
verification and rejected if invalid.

Test that proves that Syncer rejects and continues to work with a fault
header: celestiaorg/go-header#76

Potential outcomes of doing so:
celestiaorg/go-header#79

Additionally, the PR reverts the genesis hash change to the hash
pointing to height 1.

Supersedes #2449, which became zombie

---------

Co-authored-by: rene <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants