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

Verify that all commits are produced by finalizer from finalizer set #901

Open
kostyantyn opened this issue Apr 5, 2019 · 0 comments
Open
Labels
consensus p2p security A security related issue
Milestone

Comments

@kostyantyn
Copy link
Member

kostyantyn commented Apr 5, 2019

Is your feature request related to a problem? Please describe
Initial discussion started here: #852 (comment)
When node syncs, it receives headers+commits. At this step, we verify that commits are correct, but we don't check that they are produced by finalizers which are in finalizer set. For instance, after the deposit, finalizer will be able to vote in current_dynasty+3. However, if we don't check that finalizer is part of the active set, that finalizer can vote right after the deposit and potentially create its own finalization.

During the full sync, the node can be fooled and moved to the wrong fork but once it starts downloading blocks, the issue will be discovered, and blocks will be rejected as we perform the following check in ContextualCheckFinalizerCommit.

However, we don't have full blocks when we run fast sync and it can lead to constructing malicious finalization state and downloading a wrong snapshot.

Describe the solution you'd like
When we process finalizer commits (regardless of how we sync), we should always check that commits we receive are from the eligible finalizers. In this case, we can secure the fast sync as it would require 2/3 of finalizers to misbehave to attack the system.

Additional context
Currently, we have the following checks:
ContextualCheckFinalizerCommit -> CheckFinalizerCommit.

CheckFinalizerCommit checks the format of commits.
ContextualCheckFinalizerCommit checks that signatures are correct (but not tx signature), finalizers are part of finalizer set, and inputs spend UTXO.

Would be better to introduce another level of check:
FullCheckFinalizerCommit -> ContextualCheckFinalizerCommit -> CheckFinalizerCommit.

CheckFinalizerCommit stays unchanged.
From ContextualCheckFinalizerCommit we remove input check as this function will be used for both, full/fast sync. But we extend it and check the full signature of the transaction.

FullCheckFinalizerCommit calls two other functions + check the input, that it is part of UTXO.

@kostyantyn kostyantyn added consensus p2p security A security related issue labels Apr 5, 2019
@thothd thothd added this to the 1.0 milestone Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus p2p security A security related issue
Projects
None yet
Development

No branches or pull requests

2 participants