Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ses): handle properties that are already override protected #1969

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Jan 16, 2024

Fixes a bug initially reported by @FrederikBolding.
image

Staged on #1974

closes: #1971
refs: #1655 #1968 https://github.com/tc39/proposal-iterator-helpers #1973

Description

The iterators-helpers proposal https://github.com/tc39/proposal-iterator-helpers is now shipping in Chrome Canary. As per spec, it makes two properties into accessors that act like data properties that protect against the override mistake, i.e., that enable the property to be overridden by assignment. However, our permits for these properties were written when we knew them to be data properties, are they currently are on the other engines we've tested on to date. Because of this discrepancy, SES failed to initialize on Chrome Canary.

The purpose of this PR is to provide a mechanism to tolerate such methods, so that SES initializes when it encounters an accessor method of this form.

During the repairIntrinsics phase of lockdown, before even vetted shims are supposed to run, for each property we said may be of this form, validate as best we can whether it indeed seems to be an accessor that protects from the override mistake, but otherwise emulates a data property. For those, temporarily turn it into the plain data property it seems to emulate. But writable, so it won't trigger the override mistake in that form. Note that this JS is not a conformant initial JS state, and is observable by vetted shims.

Then, during the enable-property-override portion of the hardenIntrinsics phase, after all vetted shims ran, those properties listed in enablements.js will turn them (back) into accessors, but ones that we control. This PR also adds these two properties to all phases of enablements.

It turned out that the enablements language could not use symbols as property names. This PR fixed that because it needed that feature.

As a drive-by, the accessor functions installed by enable-property-overrides.js were function functions. This PR changes them to concise methods, so they'll still have the this sensitivity they need, but omit the prototype property and the [[Construct]] behavior.

Security Considerations

ses did exactly the right thing here. When the initial pre-ses state deviates from what the ses authors knew about as of that version of ses, we divide those changes into two categories:

  • Simple addition of stuff we did not know about. JS additions are usually additive, so we assume it is safe enough to delete these that ses init still results in a secure state. But even for these, we issue a warning (now made a bit less noisy feat(ses): group removal cleanup diagnostics #1942 ) so that we become aware that there are changes to the initial state beyond what ses authors knew about as of that version of ses.
  • Any change to stuff we already knew about, where the change is something we did not know about. For these, that version of ses must fail to initialize with a decent diagnostic (unless overridden by an explicitly “unsafe” configuration setting). Such changes should not be assumed to simply be additive. Ses must fail to initialize because the initial state changed in ways we don’t know if we can still secure. Ses init should NOT blindly do an intervention to restore the initial state to something it expects, because this surprising change may be symptomatic of coordinated semantic changes to other things that we cannot detect. The only way to get back to secure is exactly what we’ll do today: the ses authors must manually ensure they understand the actual meaning of the surprise, and only then, alter ses to arrive at a secure state starting from this now-understood initial state.

So,

  • SES did exactly what it should do in refusing to initialize with a good enough diagnostic for us to pinpoint the problem quickly.
  • Chrome did exactly what it should do, in releasing an agreed change to the JS spec first in Canary, so others (like us) get an early warning if JS changes in a way we need to adjust to.
  • @FrederikBolding did what we SHOULD periodically do ourselves, which is run SES on the latest canaries of all the platforms we care about. @ivan we need such a process. In fact, we still need to try it against the canaries of the other browsers.
  • We did in tc39 exactly what we should have done, which is lobby, successfully in this case, that a change that was going to happen somehow happened in the way least damaging to us.
  • Having gotten the change in at tc39, I should have better anticipated it and adjusted ses init to expect this change without breakage.

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

None of the existing tests broke, and I manually verified that things work as expected both on Chrome Canary, and on an earlier Chrome browser that has not implemented the iterators-helpers proposal.

I also exported the core logic as a function that I unit test.

Upgrade Considerations

This JS change was just encountered in Chrome Canary for now, but it is accepted into the language and will shortly ship on all platforms. Before then, all users of SES should upgrade to a SES incorporating this PR.

@erights erights self-assigned this Jan 16, 2024
@erights erights force-pushed the markm-some-properties-already-override-protected branch 2 times, most recently from 7f2abe9 to 6357a8e Compare January 16, 2024 07:11
@erights erights requested a review from rekmarks January 16, 2024 07:12
@FrederikBolding
Copy link

Can confirm this makes lockdown run successfully in Chrome Canary again! 🎉

Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review

As a matter of process, I would really prefer if independent changes were in separate commits. The change to the property override logic is great, but it seems it could easily stand in its own commit.

None of the existing tests broke, and I manually verified that things work as expected both on Chrome Canary, and on an earlier Chrome browser that has not implemented the iterators-helpers proposal.

You should be able to test this in the node-v8 nightly build.

packages/ses/src/enable-property-overrides.js Outdated Show resolved Hide resolved
packages/ses/src/enablements.js Show resolved Hide resolved
packages/ses/src/tame-faux-data-properties.js Outdated Show resolved Hide resolved
packages/ses/src/tame-faux-data-properties.js Outdated Show resolved Hide resolved
packages/ses/src/tame-faux-data-properties.js Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-some-properties-already-override-protected branch from 6357a8e to a394a2a Compare January 16, 2024 18:11
@erights erights changed the base branch from master to markm-suppress-1973-until-fixed January 16, 2024 18:12
Base automatically changed from markm-suppress-1973-until-fixed to master January 16, 2024 18:35
@erights erights force-pushed the markm-some-properties-already-override-protected branch 3 times, most recently from b0f496f to d0de868 Compare January 16, 2024 19:40
@erights erights force-pushed the markm-some-properties-already-override-protected branch from d0de868 to d058f07 Compare January 16, 2024 20:27
@erights erights requested a review from mhofman January 16, 2024 21:05
@erights erights marked this pull request as draft January 16, 2024 21:07
@erights erights force-pushed the markm-some-properties-already-override-protected branch from 2cf8cf2 to 0449938 Compare January 16, 2024 21:18
@erights erights marked this pull request as ready for review January 16, 2024 21:24
@erights
Copy link
Contributor Author

erights commented Jan 16, 2024

Ready for review again

Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be ok with this PR landing as-is but please consider adding messages to the test assertions.

packages/ses/src/tame-faux-data-properties.js Outdated Show resolved Hide resolved
@kriskowal
Copy link
Member

Also: consider adding an entry to NEWS.md.

@erights
Copy link
Contributor Author

erights commented Jan 17, 2024

I would be ok with this PR landing as-is but please consider adding messages to the test assertions.

All test assertions are well commented.

@erights erights force-pushed the markm-some-properties-already-override-protected branch from 0449938 to a95ca2b Compare January 17, 2024 02:06
@erights erights changed the title fix(ses): handle properties already override protected fix(ses): handle properties that are already override protected Jan 17, 2024
@erights
Copy link
Contributor Author

erights commented Jan 17, 2024

Also: consider adding an entry to NEWS.md.

Done. After merge though, please do look it over consistency with your style. Thanks.

@erights erights enabled auto-merge (squash) January 17, 2024 02:31
@erights erights merged commit 5792949 into master Jan 17, 2024
14 checks passed
@erights erights deleted the markm-some-properties-already-override-protected branch January 17, 2024 02:35
@mhofman
Copy link
Contributor

mhofman commented Jan 17, 2024

Thanks for addressing all the feedback!

gibson042 added a commit that referenced this pull request Jan 17, 2024
* docs(ses): Remove errant character
* chore(ses): Cleanup tame-faux-data-properties.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lockdown failing in Chrome Canary 122.0.6249
4 participants