1325938 - Baseline GetName ICs expose the window object (not the WindowProxy)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

Attachment: test.html

While working on bug 1324566, I noticed that the Baseline IC code has no check that prevents the window object from being passed to the native getter defined on the window or the prototype chain. I am attaching a test-case that triggers this reliably for me.

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: Check for window object in IsCacheableGetPropCall

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: Check for window object in IsCacheableSetPropCall

Comment 3

[Tom S. (please needinfo tschuster)]

Attachment: Check for window in Ion SetProp

Turns out the problem exists with ion SetProp checks as well.

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: Testcase

Updated testcase that shows this issue with Ion SetProp ICs.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8822039 [details] [diff] [review]
Check for window object in IsCacheableGetPropCall

Review of attachment 8822039 [details] [diff] [review]:
-----------------------------------------------------------------

Good find.

Comment 6

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:

Comment 7

[Marcia Knous [:marcia]]

Tracking 53+ for this sec-critical issue.

Comment 8

[Tom S. (please needinfo tschuster)]

Attachment: Folded patch

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8822406 [details] [diff] [review]
Folded patch

Filing the sec-approval request as discussed.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's unclear. We can let a "Window" (a global object) escape to JS code, which shouldn't happen in the browser (we should only expose the WindowProxy). This breaks a security-related invariant so let's just assume it's sec-crit.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It's not very difficult to figure out the problem from the patch, but that's hard to avoid. There are no tests or anything.

> Which older supported branches are affected by this flaw?
All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply or with minimal changes.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely, the patch just adds an extra condition before performing getter/setter optimizations. There could be some perf regressions maybe.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. Please nominate a patch for affected branches (Aurora, Beta, ESR45).

Comment 11

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1580921fec8b822d4e9658a319579870fd6850cd

Comment 12

[Tom S. (please needinfo tschuster)]

Comment on attachment 8822406 [details] [diff] [review]
Folded patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-crit
User impact if declined: 
Fix Landed on Version: 53 (Nightly)
Risk to taking this patch (and alternatives if risky): Not very risky, just a small potential performance regression
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Multiple different bugs over years, since introduction of baseline/ion
[User impact if declined]: Possible exploit
[Is this code covered by automated tests?]: Not directly this bug, can't land the test now of course
[Has the fix been verified in Nightly?]: Not yet, test case attached
[Needs manual test from QE? If yes, steps to reproduce]: Run testcase in this bug, open WebConsole, make sure there is no console output like "Window http://..."
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No
[Why is the change risky/not risky?]: Disables an optimization
[String changes made/needed]: none

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1580921fec8b

Comment 14

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8822406 [details] [diff] [review]
Folded patch

Fix for sec-critical issue, ok to uplift to aurora, beta, esr45.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5af63cf90f28

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/36ec7bc916d7

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/81c9fdbd96e84d02d5ed519e2d7b6af1d8bc6f34

Comment 18

[Andrei Vaida [:avaida]]

Reproduced the bug on an affected build (53.0a1, 20161226030205) using the testcase from Comment 0 on Ubuntu 16.04 x64. The following Web Console output was noticed:

    Window file:/// [...]


This is verified fixed on:

    - 45.7.0esr-build1 (20170118123525)
    - 51.0-build2 (20170118123726)
    - 52.0b1-build2 (20170124094647)
    - 53.0a2 (20170124223930)
    - 54.0a1 (20170125110119)

using Ubuntu 16.04 x64, Windows 10 x64 and macOS 10.12.2. No output available in the Web Console for these builds.