Closed
Bug 1325938
Opened 7 years ago
Closed 7 years ago
Baseline GetName ICs expose the window object (not the WindowProxy)
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
People
(Reporter: evilpie, Assigned: evilpie)
Details
(Keywords: sec-critical, Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])
Attachments
(5 files, 1 obsolete file)
633 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
618 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.33 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
731 bytes,
text/html
|
Details | |
2.34 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-esr45+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Attachment #8822039 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8822040 -
Flags: review?(jdemooij)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Comment 3•7 years ago
|
||
Turns out the problem exists with ion SetProp checks as well.
Assignee | ||
Comment 4•7 years ago
|
||
Updated testcase that shows this issue with Ion SetProp ICs.
Attachment #8822038 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8822043 -
Flags: review?(jdemooij)
Comment 5•7 years 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•7 years ago
|
Attachment #8822040 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8822043 -
Flags: review?(jdemooij) → review+
Comment 6•7 years 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:
--- → ?
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8822406 -
Flags: sec-approval?
Updated•7 years ago
|
Attachment #8822406 -
Flags: sec-approval?
Comment 9•7 years 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?
Updated•7 years ago
|
Comment 10•7 years ago
|
||
sec-approval+ for trunk. Please nominate a patch for affected branches (Aurora, Beta, ESR45).
Updated•7 years ago
|
Attachment #8822406 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
Group: core-security → javascript-core-security
Assignee | ||
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1580921fec8b822d4e9658a319579870fd6850cd
Assignee | ||
Comment 12•7 years 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?
Comment 13•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1580921fec8b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 14•7 years ago
|
||
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 17•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr45/rev/81c9fdbd96e84d02d5ed519e2d7b6af1d8bc6f34
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•7 years ago
|
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Comment 18•7 years ago
|
||
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
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•