Closed Bug 1125734 Opened 5 years ago Closed 5 years ago

Twitter edit profile page consistently crashes with the LastPass extension installed

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- wontfix
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: robin, Assigned: sunfish)

References

Details

(4 keywords, Whiteboard: [adv-main36+])

Crash Data

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150125030202

Steps to reproduce:

Visited Twitter’s edit profile page


Actual results:

Crash


Expected results:

No crash
Couple of crash reports:

https://crash-stats.mozilla.com/report/index/4460798f-cafb-4c97-a660-cafa72150126
https://crash-stats.mozilla.com/report/index/7091a1b5-048f-4a94-bb51-06ba62150126
Crash Signature: js::jit::LIRGeneratorShared::lowerTypedPhiInput(js::jit::MPhi*, unsigned int, js::jit::LBlock*, unsigned long)
Duplicate of this bug: 1125741
One more, this time visiting a Slashdot page: http://yro.slashdot.org/story/15/01/24/2152253/openssl-102-released

This seems pretty explosive.
Severity: normal → critical
Looks like something in this changeset blew it up https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=c18776175a69

digging through the patches
WFM with the same system configuration and build. Do you have any addons installed that might be causing this?
OK, --safe-mode doesn’t crash, but disabling all my extensions in normal mode doesn’t fix it. There’s nothing obvious in about:config that’d throw up a JIT problem (apart from manually disabling E10S as it wasn’t playing nicely with my password manager) but I’ll reset my install if you think it’ll help with debugging the issue.
It probably would, yes, so that'd be great. You can always go back to your original profile if you want to restore it.
Not sure how I was replicating it earlier with all addons disabled, but never mind, I found the offending issue:

With a reset profile and solely the LastPass extension installed (latest version 3.1.77) the browser will reliably crash when visiting the Twitter profile edit page, even if LastPass isn’t logged in. With the extension disabled then I don’t see any crash.
Summary: Twitter edit profile page consistently crashes → Twitter edit profile page consistently crashes with the LastPass extension installed
LastPass (same version) also appears in the crash log of the bug you duped to this one: https://crash-stats.mozilla.com/report/index/ee4534d7-14f8-4f06-90be-b6b532150126
I got a very similar crash [1] on [2], confirmed that it happens on a fresh profile after I install LastPass.

[1] https://crash-stats.mozilla.com/report/index/043ad6b3-5d4c-4c12-bc41-425f62150126
[2] http://www.nytimes.com/2015/01/24/nyregion/after-a-deal-british-chocolates-wont-cross-the-pond.html?smid=fb-nytimes&smtyp=cur&bicmp=AD&bicmlukp=WT.mc_id&bicmst=1409232722000&bicmet=1419773522000&_r=1
Status: UNCONFIRMED → NEW
Crash Signature: js::jit::LIRGeneratorShared::lowerTypedPhiInput(js::jit::MPhi*, unsigned int, js::jit::LBlock*, unsigned long) → js::jit::LIRGeneratorShared::lowerTypedPhiInput(js::jit::MPhi*, unsigned int, js::jit::LBlock*, unsigned long) js::jit::LIRGenerator::visitBlock(js::jit::MBasicBlock*)
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Oh, and I also have e10s disabled because of the problems with LastPass. I generalized the hardware and platform since I'm using a 64-bit build on Windows 7.
Also worth pointing out that I didn’t need to be logged in to LastPass to trigger a crash; just having the extension installed was enough.
Would be great if somebody could bisect :)
[Tracking Requested - why for this release]:
I can confirm that range.
Flags: needinfo?(sunfish)
(In reply to Alice0775 White from comment #15)
> Pushlog:

You rock, thanks.
Local builds confirm that this is bug 1118894 (as opposed to bug 1111248). FWIW, I can reproduce this just going to twitter.com without being logged in (with LastPass active).
Jan told me that Dan is already looking into it, so this might not be useful, but I do get an assertion failure in a debug+opt build in jit::AssertExtendedGraphCoherency: MOZ_ASSERT(block->id() == idx++) at the top of the loop failed. Unfortunately the opt build does optimize out a lot of this, so I can't say much more than that.
Group: javascript-core-security
Flags: needinfo?(sunfish)
Attached patch osr-yeah-no.patch (obsolete) — Splinter Review
I have been looking into it. I've installed LastPass and have tried logging into Twitter and editing profile setting with various combinations of being logged in/out of LastPass, logged in/out of Twitter, etc. and have not yet reproduced it. The nyt and slashdot links also don't reproduce for me, with or without being logged into LastPass.

Regardless, this is probably yet-another horrible corner case that arises when there's an OSR into the middle of a loop. I should have done this from the beginning, but here's a patch which completely disables this optimization for any loop which has an OSR entry into the middle.

Since I haven't reproduced the failure yet, I can't test that this patch fixes it, but it might.

Marking s-s for the same reason that bug 1118894 is, which is to say out of an abundance of caution, rather than any specific known problem. If anyone who can reproduce the bug can confirm that this patch fixes it, that'd be great.
Assignee: nobody → sunfish
@Dan Gohman [:sunfish]
Just confirmation, Are you using latest LastPass version 3.1.77 ?
( https://lastpass.com/ )
Aha, I missed that. I was using the version the Firefox extension manager gave me. With 3.1.77, I can reproduce the crash. Thanks!
I applied the patch locally to the revision just after bug 1118894 landed, and it does seem to fix the crashes for me (and the assertion failure).
Duplicate of this bug: 1125982
Ok, here's a slightly nicer version of the patch. This fixes the crash, for me.
Attachment #8554683 - Attachment is obsolete: true
Attachment #8554762 - Flags: review?(jdemooij)
Attachment #8554762 - Flags: review?(jdemooij) → review+
Attached patch aurora.patchSplinter Review
This is a version which applies to mozilla-aurora and mozilla-beta.
Comment on attachment 8554762 [details] [diff] [review]
dont-touch-loops-with-osr.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easy. It would require some amount of compiler knowledge to figure out the conditions needed to actually exploit this, if it is actually exploitable.

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

Comments in the patch describe how to get an assertion failure, however not to actually get an exploitable crash.

Which older supported branches are affected by this flaw?

mozilla32 and later; mozilla-release, mozilla-beta, mozilla-aurora, and mozilla-central, but not esr31.

If not all supported branches, which bug introduced the flaw?

Bug 844779.

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

Yes. I've already attached backports for all affected branches to this bug.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely. The change is pretty simple and is just disabling an optimization in an even more conservative way than what the patch in bug 1118894 attempted.
Attachment #8554762 - Flags: sec-approval?
Do we have evidence that this crash is exploitable? This bug needs a security rating before it can get sec-approval.
It is not known to be exploitable. All we have right now is a theoretical possibility. I'm marking sec-high, since that's what bug 1118894 is marked as.
Keywords: sec-high
Comment on attachment 8554762 [details] [diff] [review]
dont-touch-loops-with-osr.patch

sec-approval+ for checkin.
Attachment #8554762 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/ec859a233c28
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Baked for a couple days without any other known issues. I think we're good for approval requests now? :)
Flags: needinfo?(sunfish)
Comment on attachment 8554860 [details] [diff] [review]
aurora.patch

Approval Request Comment
[Feature/regressing bug #]:

bug 844779

[User impact if declined]:

potential security bug

[Describe test coverage new/current, TreeHerder]:

TreeHerder

[Risks and why]: 

This is an even simpler fix than the one attempted in bug 1118894, so it should be low risk.

[String/UUID change made/needed]:

none
Flags: needinfo?(sunfish)
Attachment #8554860 - Flags: approval-mozilla-beta?
Attachment #8554860 - Flags: approval-mozilla-aurora?
Attachment #8554860 - Flags: approval-mozilla-beta?
Attachment #8554860 - Flags: approval-mozilla-beta+
Attachment #8554860 - Flags: approval-mozilla-aurora?
Attachment #8554860 - Flags: approval-mozilla-aurora+
Group: core-security
Group: javascript-core-security
Whiteboard: [adv-main36+]
Duplicate of this bug: 1126405
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.