WIP vespera mitigation #1
Conversation
| @@ -1,6 +1,9 @@ | |||
| /* global globalThis */ | |||
| /* eslint-disable no-restricted-globals */ | |||
|
|
|||
| // Must run before the rest of commons.js | |||
| import './error/tame-v8-stack-descriptor.js'; | |||
There was a problem hiding this comment.
Why must this taming occur during initialization and before commons? Seems like an odd exception to the pattern, as well as taming during initialization.
There was a problem hiding this comment.
On the early intervention of the mitigating itself — before commons.js rather than in repairIntrinsics — I want to be sure that the reflection support already exported by commons.js, and imported and used in the rest of the trusted SES implementation, does not leak any of these. So I either need to intervene before commons.js samples these, or I need to rename the reflective exports from commons.js to FERAL_* to avoid confusing the rest of SES. The first seems a lot simpler.
My mitigation does internally rename the dangerous ones it replaces to FERAL_*, but it currently does not export any of them. The further mitigations in tame-error-constructor.js and tame-v8-error-constructor.js do, of course, happen during repairIntrinsics.
There was a problem hiding this comment.
My preference is the latter since there is precedent for exporting FERAL_* and I think of commons.js as the first code to run and solely responsible for capturing intrinsics. I’m in favor of dividing the mitigation into a tamed-property-reflection.js (tentative) that exports the tamed “intrinsics” by their conventional names for internal use and also tame-property-reflection.js (also tentative) that provides the behavior for repairIntrinsics.
That is, unless I’ve missed a reason that the mitigation must be applied during initialization, which is unprecedented.
There was a problem hiding this comment.
A reason I can imagine is uses of reflection prior to repairIntrinsics. But it is a good question and I don't know. Worth examining all those uses within the SES shim. If none of the uses prior to repairIntrinsics need the repaired one, then I have no objection.
There was a problem hiding this comment.
I imagine that if SES itself needs the patched versions of the property reflection methods internally, it would import them from tamed-property-reflection.js instead of commons.js. The distinction is that they would not yet be applied to the global environment. The question is whether or how these get exposed to shims, and I suspect the answer is that it’s adequate for them to be available to vetted shims after repairIntrinsics().
There was a problem hiding this comment.
That would require the actual construction of these repaired version to happen early, even if they're not exposed until repairIntrinsics. This early repair work seem like it would break essentially the same uniformity that this PR currently breaks. Yeah, perhaps it is a slightly milder break, but at a significant complexity cost. The current PR almost completely hides the early repair in one small module that doesn't export anything.
But...
That follows if we internally need the repaired version early, which we don't yet know.
| /* | ||
| * Because of | ||
| * https://github.com/tc39/proposal-error-stacks/issues/26#issuecomment-1675512619 | ||
| * in tame-v8-error-constructor, we have commented | ||
| * out the feature being tested here. Should we later restore the feature, | ||
| * we should also unskip this test. | ||
| */ |
There was a problem hiding this comment.
What about the mitigation causes us to lose the ability to emulate these?
There was a problem hiding this comment.
The feature support I commented out can likely be restored. But
- it was hard to think about,
- AFAIK only needed by depd, and
- easier to reason about safety with it omitted.
My plan is that once we’re confidence of safety without it, we examine the possibility of re-enabling this feature, preserving safety.
| return desc; | ||
| }, | ||
| getOwnPropertyDescriptors(obj) { | ||
| if (!('stack' in obj)) { |
There was a problem hiding this comment.
That can be attacked with a proxy object which returns false for hasProperty('stack')
| const { get: FERAL_STACK_GETTER, set: FERAL_STACK_SETTER } = | ||
| FERAL_OBJECT_GOPD(err, 'stack'); |
There was a problem hiding this comment.
This may throw if err does not have an own stack property.
closes: #XXXX
refs: #XXXX
See also
tc39/proposal-error-stacks#26 (comment)
https://github.com/tc39/proposal-error-stacks#compatibility
https://v8.dev/docs/stack-trace-api#stack-trace-collection-for-custom-exceptions
https://bugs.chromium.org/p/v8/issues/detail?id=9386
https://source.chromium.org/chromium/v8/v8.git/+/472d3c8777d2ff0715cd217d6d3d4dcf27000db3:src/accessors.cc;l=821-881
https://chromium-review.googlesource.com/c/v8/v8/+/4459251/10..15/src/execution/messages.cc#b1131
https://bugs.chromium.org/p/v8/issues/detail?id=10551 together with https://github.com/nodejs/node/blob/33710e7e7d39d19449a75911537d630349110a0c/lib/internal/errors.js#L140
https://bugs.chromium.org/p/v8/issues/detail?id=6974
Description
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations