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

Prevent addition overflow #1584

Closed
wants to merge 7 commits into from
Closed

Prevent addition overflow #1584

wants to merge 7 commits into from

Conversation

vicsn
Copy link
Contributor

@vicsn vicsn commented May 24, 2023

Motivation

We should avoid panics, this PR adds checking if additions are allowed in snarkVM. I could not find problematic subtraction cases.

For this, we should agree on the attack surface. I can imagine the following attack vectors:

  • an attacker lets different nodes misinterpret a serialized object, which can happen if our flags are serialized last. Fortunately I haven't seen this anywhere.
  • an attacker's frontend lets an honest user serialize a giant object so their local node panics, I don't think this is a huge issue
  • an attacker lets an honest node deserialize a giant object so it panics. This might kill the network so we should prevent this. Nodes already seem to have a maximum message size, but as defense in depth I use saturating_add in serialized_size. Using checked_add there is tricky as it will change the serialized_size trait function signature all over the codebase, including in macros and for types which we know have a fixed size.

Test Plan

Our usual test suite

Related PRs

Merging into https://github.com/AleoHQ/snarkVM/pull/1499

@vicsn vicsn requested a review from howardwu May 24, 2023 20:14
@ljedrz
Copy link
Collaborator

ljedrz commented May 25, 2023

A note on the maximum network message size: that size is stored in a length prefix, and only as many bytes can be read from the network in order to form a single inbound message. The implications for deserialization of individual objects are as follows:

  • no message with a size greater than MAXIMUM_MESSAGE_SIZE can be accepted from the network
  • if the length prefix indicates that the message is within bounds, but some object inside it suggests that there is a lot more bytes to read, it will just run out of bytes to process while deserializing, and the operation will fail

The main potential issue I can see (other than the potential overflows that this PR tackles) is pre-allocating too many bytes (if the collection length is stored as a u64 or a lower value, but with large items).

@vicsn
Copy link
Contributor Author

vicsn commented May 25, 2023

The main potential issue I can see (other than the potential overflows that this PR tackles) is pre-allocating too many bytes (if the collection length is stored as a u64 or a lower value, but with large items).

Hmm good point! Continuing discussion of this elsewhere, as it ties into how to handle usize/u64/u32 in the codebase which I'll tackle further in a different PR.

@vicsn vicsn force-pushed the catch_usize_and_err branch 3 times, most recently from 2500e08 to 5239635 Compare June 6, 2023 17:41
@vicsn
Copy link
Contributor Author

vicsn commented Sep 13, 2023

Closing as severely outdated

@vicsn vicsn closed this Sep 13, 2023
@vicsn vicsn deleted the checked_add branch September 13, 2023 14:45
@vicsn vicsn mentioned this pull request Sep 13, 2023
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 this pull request may close these issues.

2 participants