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
[Fix] Perform block advancement after quorum check #3232
Conversation
@ghostant-1017 Could you confirm that this addresses your finding - #3226? |
Of course! I will test it out. |
Hey @raychu86 , However, during my review process, I identified another potential DOS attack vector. It only requires a minor modification on top of the existing changes, I haven't conducted practical tests yet. The main logic is as follows:
I can probably confirm the effectiveness of this attack. Please recheck whether this attack is valid.I won't submit another report for this, I hope you consider this potential DOS attack when assessing the severity of this report. |
Thanks for flagging this @ghostant-1017! I think the attack described may be possible, there's one thing to consider: the sync logic only takes blocks from If my understanding is correct, this suggests we could have an attacker send this block once, but then it no longer works. Perhaps one thing we could do is have // Return early if this block has already been processed.
if self.ledger.contains_block_height(block.height()) {
return Ok(());
} |
Hey, @howardwu |
Thanks for the feedback. Wouldn't it require at least |
[Fix] Add `is_linked` checks in validator syncing
It seems that the @howardwu Correct me if I'm wrong. |
Yes, the idea is that the check in update_dag and subsequently here both enforce availability or quorum threshold. So wouldn't the attacker need to compromise at least f private keys from the committee to allow for the node to sync the block? (Sorry if I missed the point) |
I think Ghostant is saying we are calling I believe our mitigation for this is in the @ghostant-1017 let us know if our understanding of your concern is correct. |
@raychu86 Yes, you get me, that's what I mean. |
@raychu86 @howardwu Do you agree? |
@ghostant-1017 I agree that this could be an issue, and we should have some more mitigating checks to prevent this. However this attack existed prior to this PR, so I'm of the opinion we try to mitigate it in a separate PR after this is merged. I don't believe it is quite as straightforward to address in this PR, because we need to perform In fact, I believe that this is a more fundamental issue with receiving blocks, as this attack using malicious blocks could be done to clients and validators performing the non-BFT sync. Bitcoin has this issue as well, but i believe they get around this by doing chain-reorgs and rollbacks. Couple ideas for mitigation:
Let us know if you agree or have other insights. |
@raychu86 And I'm trying finding a way to simplify the syncing process. I'm not quite sure, but I will share with you if I'm ready. |
Motivation
This PR updates the final stage of validator syncing by adding an additional quorum check. Instead of blindly adding the block to ledger, we now check that each block's leader certificate reached quorum based on the certificates in the subsequent block.
This solves 2 problems:
Naive validator sync
X
) without performing any quorum checks.X
through the standard non-sync procedure.X
.The change now skips adding the latest block because we can't guarantee the block meets the quorum. So we will process the block through consensus instead of the sync process.
Malicious validator sync
The malicious sync case is defined in a hackerOne finding: #3226
The new quorum checks should ensure that we are not processing blocks blindly.