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

liveslots/marshal option to reject oversized inbound messages #5956

Open
warner opened this issue Aug 13, 2022 · 1 comment
Open

liveslots/marshal option to reject oversized inbound messages #5956

warner opened this issue Aug 13, 2022 · 1 comment
Assignees
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes marshal package: marshal SwingSet package: SwingSet vaults_triage DO NOT USE

Comments

@warner
Copy link
Member

warner commented Aug 13, 2022

What is the Problem Being Solved?

There's a simpler version of #5955 that might buy us some useful protection. The idea would be that liveslots checks on the size of the capdata before deserialization. If the .body is too large, or if there are too many .slots, then dispatch.deliver rejects the result promise before it even attempts to deserialize anything.

That would protect the memory footprint of the target vat. We'd incur the cost of copying the VatDeliveryObject around, but we'd avoid any expansion of the body (e.g. [],[],[], expanding into N Objects costing a couple of words each). If we had a way to pre-check the slots for new vpids, we could avoid the expense of all the promise-tracking code (as well as the subscribe calls we'd normally do during deserialization).

And refusing a message at that (early) point would let us cleanly report the failure to the sender (who might also be a victim, if an attacker submitted extra properties in some other message to them, with the intent of getting that inflation pushed into their message to us).

Description of the Design

The liveslots dispatch.deliver() code would inspect the capdata before calling unserialize.

We'd need some configuration mechanism. It could be hard-coded, like the SYSCALL_CAPDATA_BODY_SIZE_LIMIT = 10*MB we have now, or we could add some more-visible scheme (vatPowers.setLimit() from within the vat, dynamicVatOptions passed to E(vatAdminSvc).createVat() from the outside.. same as #5954)

Security Considerations

This would not protect against accumulative attacks: those will require something more like Meters and per-message charges ("stamps?" #3103 has some discussion).

This would not protect against dispatch.notify resolution data. We could apply a check there, but how should we report a failure? The resolver is at fault, but they aren't listening anymore. One option would be to replace the oversized resolution with a sorry it was too big i ate it rejection before delivering it to the local subscriber's callback.

Test Plan

unit tests

@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Aug 13, 2022
@warner
Copy link
Member Author

warner commented Aug 13, 2022

Parts of this might be implemented by configuring marshal to apply size checks. It might do these in unserialize, in serialize, or in both. If we do this, and want to make the limits configurable, we'd probably need liveslots to wrap a mutable m marshaller in a pair of stable serialize/unserialize functions, because we pass the latter around to lots of different components (virtualObjectManager.js, virtualReferenceManager.js, collectionManager.js). marshal has no long-term internal state, so it would only be statically configurable, and when we wanted to change the limits, we'd build a replacement marshaller and throw out the old one.

cc @erights

@warner warner changed the title liveslots option to reject oversized inbound messages liveslots/marshal option to reject oversized inbound messages Aug 13, 2022
@warner warner added the marshal package: marshal label Aug 13, 2022
@Tartuffo Tartuffo added this to the Mainnet 1 RC0 milestone Aug 24, 2022
@Tartuffo Tartuffo removed this from the Mainnet 1 RC0 milestone Sep 21, 2022
@erights erights self-assigned this Dec 24, 2022
@ivanlei ivanlei added the vaults_triage DO NOT USE label Jan 17, 2023
@warner warner added the liveslots requires vat-upgrade to deploy changes label Jan 24, 2023
@ivanlei ivanlei removed this from the Vaults EVP milestone Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes marshal package: marshal SwingSet package: SwingSet vaults_triage DO NOT USE
Projects
None yet
Development

No branches or pull requests

4 participants