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

Fix storage DOS attack in block proposals #2199

Open
Tracked by #1981
ma2bd opened this issue Jun 28, 2024 · 5 comments · Fixed by #2784 · May be fixed by #2813
Open
Tracked by #1981

Fix storage DOS attack in block proposals #2199

ma2bd opened this issue Jun 28, 2024 · 5 comments · Fixed by #2784 · May be fixed by #2813
Assignees
Labels
bug Something isn't working security
Milestone

Comments

@ma2bd
Copy link
Contributor

ma2bd commented Jun 28, 2024

We should not persist any certificate value (or blob) in storage before a block is confirmed.

Staged execution should behave silently differently from real execution by accepting to read certain certificate value (soon to be blobs) from a local in-memory map instead of storage.

Incidentally, we should tag the content of such map as used and verify that all of them are used after the fact, instead of making a pass where system operations are introspected. (This won't work for user blobs)

Also, when handing a block proposal, we shouldn't put the blobs into storage yet. And when handling a confirmed block certificate, we should only put them into storage after the execution.

@ma2bd ma2bd added the security label Jun 28, 2024
@ma2bd ma2bd added this to the Testnet #1 milestone Jun 28, 2024
@ma2bd ma2bd assigned ndr-ds and afck Jun 28, 2024
@ma2bd ma2bd added the bug Something isn't working label Jul 18, 2024
ndr-ds pushed a commit that referenced this issue Aug 27, 2024
## Motivation

Now that we have blobs, there are several follow ups we can do.

## Proposal

This PR replaces Bytecode with Blobs. This will clean up a lot of code and also make our blocks smaller when publishing Bytecodes. Fixes #2149 #2199

## Test Plan

CI
Some tests had to be altered since we don't have to subscribe to get bytecodes anymore, and also the `ApplicationId` format has changed with this.
@ma2bd ma2bd closed this as completed Aug 27, 2024
@afck
Copy link
Contributor

afck commented Sep 3, 2024

I don't think this is 100% completed yet:

We add to ChainManager::pending_blobs whenever we vote for a proposal. But a malicious client could make us vote for any number of block proposals without having them confirmed.

We need a rule for the pending blobs in the chain manager that guarantees liveness without allowing clients to add an arbitrary number of pending blobs.

The only rule I can think of is:

  • The chain manager should always contain exactly those pending blobs that belong to the locked block.

All other pending blobs should go into a limited LRU cache.

@afck afck reopened this Sep 3, 2024
@afck
Copy link
Contributor

afck commented Oct 28, 2024

Currently a lot of invalid block proposals with new blobs can fill up validators' value caches.

Maybe there should be a separate per-chain cache for the blobs that haven't been confirmed yet.

@ndr-ds
Copy link
Contributor

ndr-ds commented Nov 5, 2024

Should be done after #2704 is done

@ndr-ds
Copy link
Contributor

ndr-ds commented Nov 7, 2024

Actually, we probably need to finish #2186 also

@ndr-ds
Copy link
Contributor

ndr-ds commented Nov 12, 2024

We still need to add a limit on the total size of a block proposal (including blobs). Talked with @afck, we were thinking maybe 13 MB? Ideally once we can stream from the client to the server (after gRPC Web adds support to it), we'll be able to just stream the blobs in the proposal over and hopefully the blob limits would be enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security
Projects
Status: Done
3 participants