Skip to content

fix: @W-21607885 Block distorted properties from being deleted#485

Open
rwaldron wants to merge 1 commit intomainfrom
rwaldron/W-21607885
Open

fix: @W-21607885 Block distorted properties from being deleted#485
rwaldron wants to merge 1 commit intomainfrom
rwaldron/W-21607885

Conversation

@rwaldron
Copy link
Copy Markdown
Contributor

Security Vulnerability Addressed

Because near-membrane proxies project blue-realm (host) objects into the red realm (sandbox) via distortionCallback, the integrity of distortions depends on the assumption that the sandbox cannot remove or replace the distorted property on the original target. Prior to this fix, untrusted code could exploit the delete operator to breach distortion boundaries:

  1. Distortion bypass via property deletion — Sandbox code could delete ShadowRoot.prototype.host (or any property whose getter/setter/value is distorted). Because callableDeleteProperty on the blue side unconditionally called ReflectDeleteProperty(target, key), the deletion succeeded on the real blue-realm object. Subsequent property accesses would either resolve to undefined or fall through to a prototype-chain definition that was not distorted, leaking the original blue-realm behavior.

Technical Implementation

The fix adds a guard in callableDeleteProperty inside createHooksCallback (in membrane.ts). When the protectDistortions option is enabled, the callable retrieves the target property's descriptor via ReflectGetOwnPropertyDescriptor, extracts its get, set, and value components, and checks each against distortionCallback. If any component returns a value different from the original (indicating it has been distorted), the callable throws via pushErrorAcrossBoundary(new TypeErrorCtor('Cannot delete property with a distortion.')) before ReflectDeleteProperty is ever reached.

  • Option threading: A new protectDistortions?: boolean option was added to HooksOptions, VirtualEnvironmentOptions, BrowserEnvironmentOptions, and NodeEnvironmentOptions. It flows from createVirtualEnvironmentVirtualEnvironment constructor → blue connector hooks options → createHooksCallback closure.
  • Error propagation: The error is wrapped with pushErrorAcrossBoundary so it crosses the membrane correctly and surfaces as a TypeError inside the sandbox.
  • Feature gating: The guard is gated behind protectDistortions (defaults to false), so existing consumers are unaffected.
  • Scope: The guard runs in callableDeleteProperty on the blue side, which is called from the red side's passthruDeletePropertyTrap for live proxy targets. Static proxies do not cross the boundary for delete operations and are not covered by this guard.

Test Coverage

Four tests in test/distortions/protect-distortions.spec.js:

Test What it validates
Throw on distorted getter delete delete ShadowRoot.prototype.host throws TypeError when the getter is distorted and protectDistortions: true
Allow non-distorted delete delete obj.x on a plain object succeeds normally
Preserve distortion after failed delete After the throw, elm.shadowRoot.host still returns the distorted value (null)
No throw when disabled Same delete succeeds silently when protectDistortions is not set

Tests use afterEach to restore ShadowRoot.prototype.host via Object.defineProperty to prevent cross-test interference from the "when disabled" case which actually deletes the property.

Security Implications

  • Before: Sandbox code could delete any property on a blue-realm object, including those with distorted getters/setters/values, bypassing the distortion and leaking the original blue-realm definition.
  • After: With protectDistortions: true, deletion of distorted properties throws a TypeError, preserving distortion integrity.
  • The option defaults to false for backward compatibility and must be explicitly enabled by the consumer alongside liveTargetCallback (which ensures delete operations cross the boundary).

@github-actions
Copy link
Copy Markdown

Metric Coverage Percent Covered / Total
Statements 81.33% ( 854/1050 )
Branches 58.29% ( 218/374 )
Functions 74.34% ( 84/113 )
Lines 81.1% ( 824/1016 )
Total 73.77%

See detailed coverage

@rwaldron rwaldron force-pushed the rwaldron/W-21607885 branch from bb9bddc to 3a3d5cb Compare April 20, 2026 15:21
@github-actions
Copy link
Copy Markdown

Metric Coverage Percent Covered / Total
Statements 81.33% ( 854/1050 )
Branches 58.29% ( 218/374 )
Functions 74.34% ( 84/113 )
Lines 81.1% ( 824/1016 )
Total 73.77%

See detailed coverage

Comment thread test/distortions/protect-distortions.spec.js
Copy link
Copy Markdown
Contributor

@arcadeteddy arcadeteddy left a comment

Choose a reason for hiding this comment

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

What about Object.defineProperty? The same bypass works by replacing the distorted descriptor instead of deleting it, right?

I'm confused as to what damage deleting a distorted property can do? Like it's their sandbox, they should be allowed to delete and add and stuff right?

@rwaldron
Copy link
Copy Markdown
Contributor Author

What about Object.defineProperty? The same bypass works by replacing the distorted descriptor instead of deleting it, right?

I explained this in the parking lot discussion, it's not a bypass because the attacking code wouldn't have any access to the original window.top API reference to begin with.

I'm confused as to what damage deleting a distorted property can do? Like it's their sandbox, they should be allowed to delete and add and stuff right?

Deleting the distorted property tricks near-membrane into doing the thing it does for undistorted things: return the undistorted value from the blue realm.

@rwaldron rwaldron force-pushed the rwaldron/W-21607885 branch from 3a3d5cb to 79b8085 Compare April 22, 2026 14:29
@rwaldron rwaldron requested a review from arcadeteddy April 22, 2026 14:29
@github-actions
Copy link
Copy Markdown

Metric Coverage Percent Covered / Total
Statements 81.33% ( 854/1050 )
Branches 53.04% ( 218/411 )
Functions 74.34% ( 84/113 )
Lines 81.1% ( 824/1016 )
Total 72.45%

See detailed coverage

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.

2 participants