fix(ses): makeError defaults to making passable errors

erights commented 3 months ago

closes: #2198 refs: #2156

Description

The symptoms of #2156 reveal that some hosts add "extraneous" or harmful own properties to errors

Chrome, Brave (v8) adds an own stack accessor property, whose getter and setter functions can be used for mischief.
Firefox (SpiderMonkey) adds own fileName and lineNumber properties that are useful for diagnostics but should be redacted from public view.
Safari (JSC) adds an own line property, that likewise is useful for diagnostics, but should be redacted from public view.

Note that the contents of the stack should also be redacted from public view, but SES already deals with that separately.

Prior to this PR, some code assumed that the error that makeError returns would be passable once frozen. But these extra own properties, or own accessor properties, added by the host before makeError returns the error, caused it not to be passable, causing #2198 . As of this PR, makeError by default makes reasonable effort to return a passable error despite such host-added own properties. However, some code took advantage of the mutability of the error that makeError was returning, so this PR adds a sanitize: false option to suppress this cleaning and freezing of the returned error.

makeError does not guarantee that the returned error is passable, no matter what cause or errors options are provided. The purpose of this new sanitization behavior is to protect against host mischief. To test or coerce an error into a passable error, use toPassable in @endo/pass-style.

Security Considerations

Having makeError, by default, return errors that are already better behaved and more discreet helps security. However, because the underlying platform makes errors that are not sanitized in this manner, this PR doesn't help security significantly.

Scaling Considerations

none

Documentation Considerations

I added doc-comments to the makeError options doc-comment. However, the important added doccomment is on the new sanitizeError function which is encapsulated with SES's assert.js. I made a TODO that I don't know how one doccomment can link to another.

Testing Considerations

This problem was caught by accident. We should do enough browser-based testing early enough that problems like this would be caught first by purposeful testing.

Compatibility Considerations

This PR is staged on #2156. Like #2156, it presents a compat problem in theory but likely not in practice. #2156 stops exporting isPassableError and assertPassableError, which it turns out were never correct but which no one imports or uses.

This PR changes makeError to return a "sanitized" and frozen error, where it used to return a non-sanitized and non-frozen error. I looked through every call to makeError in endo and agoric-sdk, and found one site that made use of the non-frozen nature of that error. However, makeError is much older, so it is more plausible that there are other such uses outside these two repos. To accommodate that one known use case and mitigate problems at possible others, this PR adds a sanitize: false option to makeError to recover the old behavior. That one known site uses this option.

I leave it to my reviewers whether this PR as well as #2156 should be marked with a ! or not. In theory both should be. IMO, we should mark neither with !. Please let me know.

Upgrade Considerations

Since this bug manifested for #2156 on unmarshalling, i.e., unserializing use the @endo/marshal mechanisms, that bug raises the possibility of an upgrade hazard -- that an error saved to durable storage before an upgrade might cause another error on the attempt to read it from durable storage after the upgrade. This PR prevents that hazard.

[ ] Includes *BREAKING*: in the commit message with migration instructions for any breaking change.
[ ] Updates NEWS.md for user-facing changes.

erights commented 3 months ago

Following the repro instructions at #2198 , without this PR I get the bug it describes on the console output of Brave (v8), Firefox (SpiderMonkey) and Safari (JSC). With this PR, I get the following screenshots, showing that the desired error is now successfully reported

Brave:

Firefox

Safari

erights commented 3 months ago

Now testing CI for https://github.com/Agoric/agoric-sdk/pull/9201 with force:integration label and #endo-branch: markm-sanitize-errors. We'll see how it goes.

erights commented 3 months ago

Now testing CI for Agoric/agoric-sdk#9201 with force:integration label and #endo-branch: markm-sanitize-errors. We'll see how it goes.

That CI run successful. All green.

endojs / endo