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

Limit message sizes in multisig stages #1540

Closed
msgmaxim opened this issue Apr 19, 2022 · 2 comments · Fixed by #1704
Closed

Limit message sizes in multisig stages #1540

msgmaxim opened this issue Apr 19, 2022 · 2 comments · Fixed by #1704
Assignees
Labels
CFE discussion Discussions about proposed changes or ideas effort-1 Easy, but maybe tedious p2-somedaysoon

Comments

@msgmaxim
Copy link
Contributor

Description

Recent discoveries of high memory usage by CFE (causing crashes and ceremony failures) made me think about what's causing it, but also whether this is something that can be exploited by some adversary. First thing that comes to mind is how we store the all stage messages in memory before we are ready to process them (either when we get message from all peers or on timeout).

Based on my rough estimates in #966 (comment) it looks like we could be storing on the order of 100Mb for the "heaviest" stage in keygen (this does not include next stage messages that we delay processing of). Not sure how much this contributed to the high memory usage observed on testnet, but I realised that we can end up storing much larger messages in the presence of an adversary: the size of messages for some stages isn't bounded.

Every BroadcastVerificationMessage, for example, can contain millions of elements and still be parsed as valid by bincode (and thus be accepted initially):

pub struct BroadcastVerificationMessage<T: Clone> {
    pub data: HashMap<usize, Option<T>>,
}

Note that we only process the messages when we receive all of them (which is often necessary), so a party could send us 1GB+ messages (unless there is some limit in the p2p layer to prevent this) causing us to run out of memory before we even get a chance to discard them as invalid.

Proposed solution

We should be able to perform simple pre-validation of all received messages, for example, by comparing the number of elements in the HashMap/Vec with what we expect for the given ceremony.

Alternatives

Set a hard limit in bytes on the size of every message somewhere higher in the stack (i.e. p2p layer), so we don't need a separate pre-validation step for each type of message. I prefer not to do this as it is too crude and can lead to difficult to identify bugs, e.g. if we change the contents of stage messages in the protocol but forget to update the size limit in the p2p layer.

@msgmaxim msgmaxim added discussion Discussions about proposed changes or ideas CFE p2-somedaysoon effort-1 Easy, but maybe tedious labels Apr 19, 2022
@nakul-cf nakul-cf changed the title Limit message sizes in multisig stages [SC 3346] Limit message sizes in multisig stages Apr 19, 2022
@morelazers
Copy link

Would the pre-verification solve the issue entirely, or would it still be possible to send a message has a payload much larger than what we'd expect (the number of map keys might be ok but what about the size of the values)?

@msgmaxim
Copy link
Contributor Author

Would the pre-verification solve the issue entirely, or would it still be possible to send a message has a payload much larger than what we'd expect (the number of map keys might be ok but what about the size of the values)?

We will need to be careful if there are nested "collections" (which is the case for some "broadcast verification" stages), where we would need to check the inner values too.

@j4m1ef0rd j4m1ef0rd self-assigned this May 9, 2022
@morelazers morelazers changed the title [SC 3346] Limit message sizes in multisig stages Limit message sizes in multisig stages May 24, 2022
@morelazers morelazers added this to the 0.4.0 - Airplane Peanuts milestone May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CFE discussion Discussions about proposed changes or ideas effort-1 Easy, but maybe tedious p2-somedaysoon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants