bonsaidb icon indicating copy to clipboard operation
bonsaidb copied to clipboard

Prevent memory exhaustion attacks via serialization

Open ecton opened this issue 4 years ago • 1 comments
trafficstars

We shouldn't ever use bincode::deserialize directly. The preferred method is to use the Options trait. The DefaultOptions struct talks about what options are set up by default, and the important part for us is the byte limit.

Technically since PliantDb relies on Fabruic, to truly be preventing it, this issue should also be fixed before closing this one out fully: GitHub issue/pull request detail

For cbor, the situation is more complicated. Here's a devlog describing my experiment of writing my own serialization format. As of this edit (July 13), my thoughts are now that we should:

  • [x] Adopt pbor. And... rename it for goodness sake. (done, now named Pot)
  • [x] Add max_size configuration options to pot
  • [ ] Switch wire protocol to Pot (Blocked by khonsulabs/fabruic#28)
  • [ ] Add a configuration option to the database, and use this single setting for bincode and pot. CBOR isn't an attack vector when it's export-only.

ecton avatar May 21 '21 14:05 ecton

Made some progress on this today, but mostly just familiarized myself with what needs to be done. I realized that we're currently relying on Fabruic to encode the payload being sent across the wire, and it's using bincode. Currently, the CustomApi Request/Response types are being serialized directly onto this structure. Either we need to switch the Fabruic protocol to Pot (my proposal of allowing this is in khonsulabs/fabruic#28) or we need to wrap the end-user data structure in a pre-serialized Vec that is encoded using Pot.

Outside of that, the plan should be able to be implemented as it was before. To save my future-self some time from pondering the same thing again: The documents are encoded on the client, which means that view map decoding must consider document decoding potentially malicious. Thus, even document deserialization should have a max-size option, but perhaps it should be independent from the settings used for the wire protocol. This is possible because the document is serialized as a Vec before being placed into the wire protocol's structure.

ecton avatar Oct 27 '21 21:10 ecton