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

zoe: Validate endoZipBase64 upon receipt #3878

Open
kriskowal opened this issue Sep 22, 2021 · 7 comments
Open

zoe: Validate endoZipBase64 upon receipt #3878

kriskowal opened this issue Sep 22, 2021 · 7 comments
Assignees
Labels
audit-zestival Vulnerability assessment of ERTP + Zoe bundle-source package: bundle-source enhancement New feature or request Zoe package: Zoe

Comments

@kriskowal
Copy link
Member

kriskowal commented Sep 22, 2021

What is the Problem Being Solved?

Third-parties submit contracts to Zoe as bundles and Zoe creates an installation. The installation in turn vends out the bundle to any handle on the installation. This makes it possible for an attacker to install a contract with an arbitrary Zip file that another client would receive from the installation. Although Zoe is safe from malicious Zip files by virtue of using a very narrowly prescribed Zip file reader, the client does not benefit from the same protection. A crafted Zip file for example could be a small Zip file that expands to petabytes of data by exploiting compression, duplication, and recursion within the archive.

Description of the Design

To validate a bundle, Zoe would need only to attempt to extract it into memory with @endo/zip, which would balk at any file that uses compression or an end-of-archive comment. To go farther, @endo/zip could be more weary about overlapping content regions or duplicate canonicalized file names. We should also go forward to ensure that every individual file in the archive is retained by name in the compartment-map.json, that its extension is consistent with its module type, and that it is a valid module of that type. We should keep in mind that the module types are likely to become extensible in the future and to somehow accommodate that. We should also assert that no files are executable.

Security Considerations

Zoe should not be made vulnerable to hand-crafted malicious Zip files.

Test Plan

It should be sufficient to submit a variety of hand-crafted malicious Zip files, each exploiting one of the Zip format features that can lead to problems.

  • compression
  • overlapping content
  • duplicate canonical file names
  • end of archive comment that is itself a zip file
  • misreporting of compressed vs decompressed size
  • &c
@kriskowal kriskowal added the enhancement New feature or request label Sep 22, 2021
@kriskowal kriskowal self-assigned this Sep 22, 2021
@katelynsills
Copy link
Contributor

Adding notes for myself: To protect users when they attempt to inspect the source code of contract installations, E(zoe).install() would need do some additional checks on the zip file before creating the installation, throwing if the checks fail. Specifically, Zoe would need to attempt to extract the bundle into memory with @endo/zip.

Installations do not always come directly from Zoe, however - in particular a dapp might send your wallet an alleged installation that has never gone through Zoe. In this, case, the user should check their installation with Zoe. A method to do this does not yet exist but is mentioned in issue #3344

I also need to learn more about what kinds of attacks through zip files are possible, so this may not cover all the attacks. Additionally, @kriskowal says that a different extension may help.

@kriskowal
Copy link
Member Author

Added:

We should also go forward to ensure that every individual file in the archive is retained by name in the compartment-map.json, that its extension is consistent with its module type, and that it is a valid module of that type. We should keep in mind that the module types are likely to become extensible in the future and to somehow accommodate that. We should also assert that no files are executable.

@zarutian
Copy link
Contributor

/me surripitiously stuffs droste.zip and co back into its archive folder.

@katelynsills katelynsills added the Zoe package: Zoe label Nov 5, 2021
@erights erights added the audit-zestival Vulnerability assessment of ERTP + Zoe label Jan 10, 2022
@Tartuffo Tartuffo added bundle-source package: bundle-source MN-2 labels Jan 19, 2022
@Tartuffo
Copy link
Contributor

@kriskowal We included this in the Mainnet 1 release, even though it is labeled MN-2. I'm removing the MN-2 label. If you think it really should be MN-2, please LMK.

@Tartuffo Tartuffo removed the MN-2 label Jan 31, 2022
@kriskowal
Copy link
Member Author

@Tartuffo @erights This probably can be deferred to adversarial mainnet at latest.

@kriskowal
Copy link
Member Author

And I’ve posted a fib of 5 for this on the assumption that writing good tests is 4/5 of the effort.

@kriskowal
Copy link
Member Author

In #4751, we introduced checkBundle and now SwingSet uses this to validate out-of-band bundlecap submission. This is a step along the way to changing E(zoe).install(bundle) to only accept hash bundles, that is, bundles that refer to a bundle that was submitted to the SwingSet controller out-of-band. When, #4974 is complete, Zoe will not be responsible for vetting bundles.

We may want to also vet content-bundles at Zoe ingress as long as we support that feature, but it seems to me we can be lazy about that if Zoe stops accepting content-bundles before integrity checks become important (MN-3?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-zestival Vulnerability assessment of ERTP + Zoe bundle-source package: bundle-source enhancement New feature or request Zoe package: Zoe
Projects
None yet
Development

No branches or pull requests

5 participants