If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox -esr45

Status

()

Core
JavaScript Engine
VERIFIED FIXED
10 months ago
2 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

({sec-critical})

unspecified
mozilla53
sec-critical
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox-esr4551+ verified, firefox50 wontfix, firefox51+ verified, firefox52+ verified, firefox53+ verified)

Details

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

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

10 months ago
testcase
Created attachment 8822038 [details]
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.
(Assignee)

Comment 1

10 months ago
Created attachment 8822039 [details] [diff] [review]
Check for window object in IsCacheableGetPropCall
Attachment #8822039 - Flags: review?(jdemooij)
(Assignee)

Comment 2

10 months ago
Created attachment 8822040 [details] [diff] [review]
Check for window object in IsCacheableSetPropCall
Attachment #8822040 - Flags: review?(jdemooij)
(Assignee)

Updated

10 months ago
Assignee: nobody → evilpies
(Assignee)

Comment 3

10 months ago
Created attachment 8822043 [details] [diff] [review]
Check for window in Ion SetProp

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

Comment 4

10 months ago
Created attachment 8822044 [details]
Testcase

Updated testcase that shows this issue with Ion SetProp ICs.
Attachment #8822038 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8822043 - Flags: review?(jdemooij)

Comment 5

10 months ago
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+

Updated

10 months ago
Attachment #8822040 - Flags: review?(jdemooij) → review+

Updated

10 months ago
Attachment #8822043 - Flags: review?(jdemooij) → review+

Comment 6

10 months ago
[Tracking Requested - why for this release]:
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox53: --- → ?
Tracking 53+ for this sec-critical issue.
tracking-firefox53: ? → +
(Assignee)

Comment 8

10 months ago
Created attachment 8822406 [details] [diff] [review]
Folded patch
Attachment #8822406 - Flags: sec-approval?

Updated

10 months ago
Attachment #8822406 - Flags: sec-approval?

Comment 9

10 months ago
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.
Attachment #8822406 - Flags: sec-approval?
status-firefox50: affected → wontfix
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox-esr45: --- → ?
sec-approval+ for trunk. Please nominate a patch for affected branches (Aurora, Beta, ESR45).
tracking-firefox51: ? → +
tracking-firefox52: ? → +
tracking-firefox-esr45: ? → 51+

Updated

10 months ago
Attachment #8822406 - Flags: sec-approval? → sec-approval+

Updated

10 months ago
Group: core-security → javascript-core-security
(Assignee)

Comment 11

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1580921fec8b822d4e9658a319579870fd6850cd
(Assignee)

Comment 12

10 months ago
str
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
Attachment #8822406 - Flags: approval-mozilla-esr45?
Attachment #8822406 - Flags: approval-mozilla-beta?
Attachment #8822406 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1580921fec8b
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox53: affected → fixed
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+

Comment 15

10 months ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/5af63cf90f28
status-firefox52: affected → fixed

Comment 16

10 months ago
https://hg.mozilla.org/releases/mozilla-beta/rev/36ec7bc916d7
status-firefox51: affected → fixed

Comment 17

10 months ago
https://hg.mozilla.org/releases/mozilla-esr45/rev/81c9fdbd96e84d02d5ed519e2d7b6af1d8bc6f34
status-firefox-esr45: affected → fixed
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.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
status-firefox52: fixed → verified
status-firefox53: fixed → verified
status-firefox-esr45: fixed → verified
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.