fix(pass-style,marshal): fix tests consistent with #2200

[erights]

closes: #2198
refs: #2230 #2200 https://chromium-review.googlesource.com/c/v8/v8/+/4459251

Description

At https://chromium-review.googlesource.com/c/v8/v8/+/4459251 v8 changed their error stack property into an own accessor property with per-instance getters and setters. Because these getters and setters are fresh per error instance, on seeing such a property, we have no way to know whether the getter and setter is the platform-provided one, or one provided by an attacker.

#2198 was erroneously closed by #2200 because #2198 only manifested on browsers and we manually tested #2200 on those browsers to "verify" that it was fixed.

Security Considerations

Given the problematic v8 stack accessor behavior, which we're not in a position to fix, we are faced with only bad choices regarding Passable errors:

• platform created errors, or those created by direct calls to the various *Error constructors, are not made Passable merely by freezing them. Rather, the error has to go through some kind of sanitization first to derive from it a Passable error. This is the option we chose in fix(ses): makeError defaults to making passable errors #2200
• Allow errors with own stack accessor properties, with per-instance getters and setters, to be considered Passable. Since we'd have no way to check, on v8, that the getters and setters are the ones introduced by the platform rather that an attacker, this would compromise the desired security properties of Passables.
• Give up on v8 (and therefore Node, Chrome, Brave, etc...) as a target platform.
• (In theory, we could introduce a special case into harden to convert an error's own stack accessor property into a data property before freezing. But this would be a major abstraction levels violation that we should consider only if the none of the other choices above are viable. I mention it for completeness.)

Since we are not willing to give up either on Passable security, nor on supporting v8-based target platforms, this seems to force us into the first choice.

Note that marshal will encode top-level non-passable errors safely, so this does not inhibit the reporting of normal remotely-thrown errors. Thus, even for the first choice, the unpleasantness should be well contained.

Scaling Considerations

none

Documentation Considerations

When an error is used in a context where only Passable is accepted, we will need to explain what to do and why.

Testing Considerations

Because our current CI setup test at nothing more recent than Node 20, which does not have the v8 problem, this PR is not meaningfully verified by our current CI setup. Of course, it should remain green (which it is at the moment). But I'm manually verifying only that I can run yarn test locally while using Node 21, whose v8 does have the problematic accessor behavior.

Compatibility Considerations

This v8 change of the stack property into an own accessor property with unverifiable per-instance getters and setters, causes the compat break that initially manifested as #2198, fixed by #2200 + this PR. But that still leaves a compat problem, as demonstrated by the test cases that this PR needed to fix, so they would continue to work. Old code that used error-handling patterns that used to work may break, due to this change in v8 behavior, even after both #2200 and this PR. Such old code will need to be fixed to somehow sanitize errors that need to be Passable, either by

• coercing them through toPassableError
• making them initially using makeError imported from @endo/errors.

Upgrade Considerations

Neither #2200 nor this PR actually cause a breaking change. Rather, v8 causes a breaking change that these two PRs mitigate, but without reducing this to a non-breaking change. So I added a NEWS.md item. Reviewers, since this PR is not causing any breakage, where should the breaking change be noted?

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

[kriskowal]

Do we need corresponding changes in captp and liveslots to make use of toPassableError in the event an eventual send transits a non-passable error (most errors under this regimen)?

[erights]

Converted back to draft, because I was wrong about a crucial aspect of the v8 problem, which creates a possibly better option for "fixing" it.

I thought I remembered that the getter and setter were fresh per error instance, and therefore passStyleOf had no ability to tell whether the getter and setter it was seeing were the ones provided by the platform, or ones provided by an attacker. I'm wrong. Within the same realm, all these getters are the same, and all these setters are the same. So, security question:

If passStyleOf sees a frozen error with own stack accessor stack property, where the getter and setter are the platform provided ones it expects, how bad would it be for passStyleOf to judge that error object to be Passable?

Note that the getter/setter combination would prevent these error objects from being pure. Rather, they would be a communications channel between any two parties sharing direct access to such an error object. Among passables, we consider Remotables and Promises to be explicit capabilities providing access to cause effects and communications. We will not consider errors to be a means that should be used for communications. But within a vat, we would not be able to suppress it.

If we do adopt this rule, I would still have toPassableError(err) create and return an error object that has no such communications channel, even if its argument err was a passable error with such a communications channel. The errors resulting from toPassableError and makeError would remain pure.

Reviewers, what do you think? (Adding @raphdev and @LuqiPan )

[erights]

Do we need corresponding changes in captp and liveslots to make use of toPassableError in the event an eventual send transits a non-passable error (most errors under this regimen)?

Not for top-level errors, which are the dominant case for marshal. But for errors embedded in copy-data structures that should be passable, if the error is not made passable at the time it is incorporated into that copy-data, the resulting copy-data will not itself be passable. So, I don't think captp and liveslots need to do anything, but all their clients might. (Which is why I'm considering the more dangerous but more compat #2229 (comment) option.)

[erights]

Closing in favor of #2232