Definite Properties Analysis doesn't detect |this| escaping via CallObjects

RESOLVED FIXED in Firefox -esr45

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

({crash, sec-high})

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr4551+ fixed, firefox50 wontfix, firefox51+ fixed, firefox52+)

Details

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

Attachments

(3 attachments)

Assignee

Description

2 years ago
Posted file Shell test
AKA as the Facebook crash.

Consider the following JS code:

  function foo(o) { return function() { return o; }; }

  function SomeObject(o) {
      this.x = foo(this);
  }

When we run the definite properties analysis on SomeObject, we inline foo. That's fine, but we then don't track that foo lets |this| escape via its CallObject.

The bug is in IonBuilder::initEnvironmentChain, where we don't call createCallObject if we're in analysis mode. We should just fix that condition to do the right thing for the DPA.

Attaching a testcase for the JS shell that crashes in exactly the same way.
Assignee

Comment 1

2 years ago
Posted patch PatchSplinter Review
One-line fix.

Note that the "Skip this for analyses, as the script might not have a baseline script with template objects yet." comment only applies to the arguments analysis. We always have a BaselineScript when we run the DPA.
Attachment #8826711 - Flags: review?(shu)
Assignee

Comment 2

2 years ago
It's possible something in Firefox 51 exposed this somehow (probably the frontend rewrite, see bug 1308800 comment 30), but the code we're fixing is also in ESR45. Let's fix this everywhere, just to be safe.
Assignee

Updated

2 years ago
Assignee

Comment 3

2 years ago
Comment on attachment 8826711 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily. Writing the shell test took me a while, even when I knew what the bug was. It's possible this is always a safe nullptr crash, but I'm not 100% sure.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
All, though the crashes started in 51.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be trivial to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely. Green Try push should be sufficient.
Attachment #8826711 - Flags: sec-approval?
Assignee

Comment 4

2 years ago
Comment on attachment 8826711 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Unknown.
[User impact if declined]: Lots of Facebook crashes for certain users.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Verified locally.
[Needs manual test from QE? If yes, steps to reproduce]: Could test FB.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Very small and safe patch. We have lots of tests for this code.
[String changes made/needed]: None.
Attachment #8826711 - Flags: approval-mozilla-esr45?
Attachment #8826711 - Flags: approval-mozilla-beta?
Attachment #8826711 - Flags: approval-mozilla-aurora?
Assignee

Comment 5

2 years ago
NI as requested.
Flags: needinfo?(rkothari)
Flags: needinfo?(gchang)
Assignee

Updated

2 years ago
Flags: needinfo?(lhenry)
Tracked for 51+. Waiting for Al/Dan to confirm whether we ought to take this in ESR45 or not.

Gerry/Liz, given the volume of the FB crash I'd suggest we take this in RC1. Also it's a one-line fix that jandem considers pretty safe and has an automated test for verification purpose.
Flags: needinfo?(rkothari)
Assignee

Comment 7

2 years ago
Posted file Shell test 2
Here's a different test that shows this can be used to read uninitialized memory.

$ dist/bin/js --no-threads test.js
test.js:4:2 Error: Assertion failed: got 757935405, expected (void 0)

>>> hex(757935405)
'0x2d2d2d2d'

Updated

2 years ago
Attachment #8826711 - Flags: review?(shu) → review+
(In reply to Ritu Kothari (:ritu) from comment #6)
> Tracked for 51+. Waiting for Al/Dan to confirm whether we ought to take this
> in ESR45 or not.
> 
> Gerry/Liz, given the volume of the FB crash I'd suggest we take this in RC1.
> Also it's a one-line fix that jandem considers pretty safe and has an
> automated test for verification purpose.

I'll give sec-approval+ for trunk based on you being interested in this for RC1. I'd suggest taking it for ESR45.
Attachment #8826711 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/0431a0ab907d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8826711 [details] [diff] [review]
Patch

Fix for the FB crash, sec-high issue. Please uplift to aurora, beta, and esr45.
Flags: needinfo?(lhenry)
Attachment #8826711 - Flags: approval-mozilla-esr45?
Attachment #8826711 - Flags: approval-mozilla-esr45+
Attachment #8826711 - Flags: approval-mozilla-beta?
Attachment #8826711 - Flags: approval-mozilla-beta+
Attachment #8826711 - Flags: approval-mozilla-aurora?
Attachment #8826711 - Flags: approval-mozilla-aurora+
Flags: needinfo?(gchang)
QA Contact: kjozwiak
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Group: javascript-core-security → core-security-release
Jan, I couldn't reproduce the crash using the shell test from comment#0 with a version of m-c [1] from the beginning of the month. However, I can reproduce the assertion using the shell test from comment#7. I went through bug#1308800 but it doesn't seem like anyone managed to reproduce the crash reliably.

Regarding the first shell test, should I be running it under js with specific flags? Would running verifications against the second shell test be sufficient enough in this case?

[1] https://archive.mozilla.org/pub/firefox/nightly/2017/01/2017-01-01-03-02-04-mozilla-central/
Flags: needinfo?(jdemooij)
Assignee

Comment 14

2 years ago
(In reply to Kamil Jozwiak [:kjozwiak] from comment #13)
> Regarding the first shell test, should I be running it under js with
> specific flags? Would running verifications against the second shell test be
> sufficient enough in this case?

It's possible you need a debug shell build for the first test to crash. But yes, testing the second shell test should be sufficient - it's the same bug, just slightly different ways to expose it.

Regarding bug 1308800, I was able to reproduce the FB crash reliably without the patch and I could no longer reproduce it with the patch applied. It was a topcrash and the number of crashes dropped to zero after the patch landed, so I think we can be confident this is fixed.
Flags: needinfo?(jdemooij)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.