Closed Bug 1325938 Opened 7 years ago Closed 7 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox-esr45 51+ verified
firefox50 --- wontfix
firefox51 + verified
firefox52 + verified
firefox53 + verified


(Reporter: evilpie, Assigned: evilpie)


(Keywords: sec-critical, Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])


(5 files, 1 obsolete file)

Attached file test.html (obsolete) —
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.
Attachment #8822039 - Flags: review?(jdemooij)
Attachment #8822040 - Flags: review?(jdemooij)
Assignee: nobody → evilpies
Turns out the problem exists with ion SetProp checks as well.
Attached file Testcase
Updated testcase that shows this issue with Ion SetProp ICs.
Attachment #8822038 - Attachment is obsolete: true
Attachment #8822043 - Flags: review?(jdemooij)
Comment on attachment 8822039 [details] [diff] [review]
Check for window object in IsCacheableGetPropCall

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

Good find.
Attachment #8822039 - Flags: review?(jdemooij) → review+
Attachment #8822040 - Flags: review?(jdemooij) → review+
Attachment #8822043 - Flags: review?(jdemooij) → review+
[Tracking Requested - why for this release]:
Tracking 53+ for this sec-critical issue.
Attached patch Folded patchSplinter Review
Attachment #8822406 - Flags: sec-approval?
Attachment #8822406 - Flags: sec-approval?
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?

> 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.
Attachment #8822406 - Flags: sec-approval?
sec-approval+ for trunk. Please nominate a patch for affected branches (Aurora, Beta, ESR45).
Attachment #8822406 - Flags: sec-approval? → sec-approval+
Group: core-security → javascript-core-security
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 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
Attachment #8822406 - Flags: approval-mozilla-esr45?
Attachment #8822406 - Flags: approval-mozilla-beta?
Attachment #8822406 - Flags: approval-mozilla-aurora?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8822406 [details] [diff] [review]
Folded patch

Fix for sec-critical issue, ok to uplift to aurora, beta, esr45.
Attachment #8822406 - Flags: approval-mozilla-esr45?
Attachment #8822406 - Flags: approval-mozilla-esr45+
Attachment #8822406 - Flags: approval-mozilla-beta?
Attachment #8822406 - Flags: approval-mozilla-beta+
Attachment #8822406 - Flags: approval-mozilla-aurora?
Attachment #8822406 - Flags: approval-mozilla-aurora+
Group: javascript-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
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.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.