-
Notifications
You must be signed in to change notification settings - Fork 75
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
compartment-mapper: makeSecureBundle with bundled runtime and no-eval #1449
base: master
Are you sure you want to change the base?
Conversation
c6199a7
to
7f909ed
Compare
6236096
to
e323faa
Compare
packages/compartment-mapper/test/fixtures-0/node_modules/bundle-unsafe/package.json
Outdated
Show resolved
Hide resolved
return function() { | ||
'use strict'; | ||
return ( | ||
${functorSrc} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure when's the good time to do it, but we should probably check functorSrc
parses as JavaScript.
try {
eval(`throw Error('valid');
${functorSrc}`);
} catch(e) {
assert( e.message === 'valid')
}
It'd be the safest to do at runtime in case there's differences in parsing, but that's not something we want to do with end-users' CPUs so earlier in build process would have to suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider 😈
eval(`throw Error('valid'); ${functorSrc}`);
with functorSrc
:
function Error () { /* attack payload */ }
browserify uses throw 'STOP';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- src is validated when creating a bundle by entrypoint, somewhere in
prepareToBundle
- added a failing test showing
makeSecureBundleFromArchive
not validating invalid module sources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that looks solid, the only remaining concern is someone finding a way to make unziping+parsing in the runtime host result in a different module.
eg. we build an archive in Node and then run it in Hermes or some bespoke JS engine with an ASI implementation quirk or parsing bug that changes the AST resulting from the original text. That could be exploited fairly easily (after it's been found, which is the hard part) using the method I used when exploiting the comment in LavaMoat.
This is the only reason to consider the browserify trick at runtime. It depends on eval though and is therefore unusable for us. Unless we have a non-eval alternative the work I put into typing all this up is for nothing anyway ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we censored the functorSrc at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we had not censored -- this has been fixed by running the "mandatory transforms"
e323faa
to
965ed99
Compare
d1e922b
to
97baf9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review for today.
import fs from 'fs'; | ||
import url from 'url'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ocap violation. The relevant powers of these modules must continue to be threaded through the powers argument. (Are these referenced?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relevant comment
endo/packages/compartment-mapper/src/bundle.js
Lines 416 to 419 in 16d0c2f
// these read powers must refer to the disk as we are bundling the runtime from | |
// this package's sources. The user-provided read powers used elsewhere refer | |
// to the user's application source code. | |
const { read } = makeReadPowers({ fs, url }); |
unsure how to proceed - ask the bundle for two different read powers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. What are the two file systems in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- this repo's file system so we can bundle the secure bundler runtime
- the user's file system for their app
return function() { | ||
'use strict'; | ||
return ( | ||
${functorSrc} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we censored the functorSrc at this point?
…ntext object + global
…rchive sources during bundling
97baf9e
to
b4d2d81
Compare
16d0c2f
to
f08ae39
Compare
f08ae39
to
8b17f6b
Compare
previous iterations:
modify existing bundler for ses-wrapping #1437
ensure archive machinery can bundle #1436
Must:
Should:
Could/someday: