Closed erights closed 5 months ago
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 )
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 https://github.com/endojs/endo/pull/2229#issuecomment-2067358206 option.)
Closing in favor of https://github.com/endojs/endo/pull/2232
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.https://github.com/endojs/endo/issues/2198 was erroneously closed by https://github.com/endojs/endo/pull/2200 because https://github.com/endojs/endo/issues/2198 only manifested on browsers and we manually tested https://github.com/endojs/endo/pull/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:
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.harden
to convert an error's ownstack
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 bytoPassableError
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?
*BREAKING*:
in the commit message with migration instructions for any breaking change.~NEWS.md
for user-facing changes.