fix(ses): harden hacks v8 stack own accessor problem
#2232
Conversation
erights
changed the title
harden hacks v8 stack own accessor problem
| const stackDesc = getOwnPropertyDescriptor(obj, 'stack'); | ||
| if ( | ||
| stackDesc && | ||
| stackDesc.get === FERAL_STACK_GETTER && |
There was a problem hiding this comment.
Test for either the getter or setter?
| stackDesc.get === FERAL_STACK_GETTER && | |
| (stackDesc.get === FERAL_STACK_GETTER || stackDesc.set === FERAL_STACK_SETTER) && |
There was a problem hiding this comment.
Done, but with && rather than || to be more conservative. But either they are both set or neither is set, so cannot actually make a difference.
Resolving.
There was a problem hiding this comment.
I believe I didn't express myself correctly. I was trying to handle the case where an error object would show up with either a getter or a setter own prop. Arguably someone would have to go muck with the prop descriptor before hardening the error object, so probably not worth bothering about.
There was a problem hiding this comment.
This test here is only testing whether we're on a platform suffering from v8's misbehavior.
Ok to reresolve?
There was a problem hiding this comment.
My understanding is that we already know we're on an affected platform, but here we're testing whether the error object we found exhibit the bad behavior. It's possible the error object got modified already (e.g. stack prop removed, or replaced with data). The question is which kind of modifications we want to handle here (in particular whether we want to handle a setter only stack prop if we find one)
erights
force-pushed
the
markm-harden-hacks-stack-accessor
branch
from
661f391
to
b11beba
Compare
|
Since #2231 was already merged, causing CI errors for the CI cases that this PR is trying to address, I added to this PR the change from #2233 / #2234 that switches from trying to use 22.x, which fails because it is not found, to using 21.x, which is found. I will thus also close #2233 and #2234 in favor of this PR. |
This is a repair that harden does for the benefit other layers (pass style). IMO, it's conceptually hard to justify its presence in the ses shim itself. It wouldn't be justifiable at all in a standalone harden shim. |
It could be exposed as a configuration option in a standalone harden shim. I agree that it is unpleasant, but the underlying problem caused by v8 is the source of the unpleasantness. "Fixing" it in harden is, I think, the fix that's least unpleasant for users, as evidenced by all the test case changes that I no longer needed to do. |
## Description All this `path` maintenance within `harden` was there in case we encountered failures to harden that were hard to diagnose. They accumulate the path accumulated from the harden root to the thing that failed to be hardened. But it was never used to produce a better error message. Rather, they could have been useful from a debugger's eye view. But in all the years we've paid the price for this diagnostic, we have not made use of it. The operations used to accumulate the path are expensive string operations and WeakSet operations. Because the diagnostic information was never actually shown in any diagnostic, this change should be completely without any observable effect. Hence, this PR is a pure refactor. ### Security Considerations Mostly none. By making `harden` cheaper, perhaps it'll be used more, which would help security. But we have not generally avoided harden due to its runtime cost, with the exception of the SwingSet kernel. ### Scaling Considerations The point. Note though that on XS we're already using the native harden that does not pay this price. The SwingSet kernel `harden` is already suppressed. So this PR only affects use of `harden` not on XS and not within the SwingSet kernel itself. @gibson042 , before merging this, I'd like to work with you to quantify its performance impact. If it has none, perhaps it isn't work doing and we should keep the diagnostic info, just in case? ### Documentation Considerations none. ### Testing Considerations none ### Compatibility Considerations none However, I noticed this while working on #2232 because I had to modify the same code for different purposes. There will be minor merge conflicts coming between these two PRs which I'll resolve after one of them is merged. ### Upgrade Considerations none - ~[ ] Includes `*BREAKING*:` in the commit message with migration instructions for any breaking change.~ - ~[ ] Updates `NEWS.md` for user-facing changes.~
closes: #2198
refs: #2230 #2200 https://chromium-review.googlesource.com/c/v8/v8/+/4459251 #2229 #2231 https://issues.chromium.org/issues/40279506
Description
Alternative to #2229 that just hacks
hardento directly repair a problematic error own stack accessor property, replacing it with a data property.Security Considerations
Both before and after this PR,
passStyleOfwill reject errors with the v8 problematic error own stack accessor property, preventing the unsafety at stake here. However, this would mean that much existing code that used to be correct will break when run on a v8 with this problem.Scaling Considerations
Avoids any extra overhead on platforms without this problem, including all platforms other than v8.
Documentation Considerations
probably none. This PR essentially avoids the need to document the v8 problem that it masks.
Testing Considerations
Only needed to repair one test to use
hardenrather thanfreeze, in a case wherehardenwas more natural anyway.Compatibility Considerations
This PR enables more errors to pass that check without further changes to user code. #2229 had similar goals, but would still require more changes to user code than this PR. This is demonstrated by all the test code in #2229 that needed to be fixed that does not need to be fixed in this PR.
Upgrade Considerations
none
[ ] Includes*BREAKING*:in the commit message with migration instructions for any breaking change.[ ] UpdatesNEWS.mdfor user-facing changes.