1125734 - Twitter edit profile page consistently crashes with the LastPass extension installed

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Robin Whittleton]

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

Comment 1

[Robin Whittleton]

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

Comment 3

[Robin Whittleton]

One more, this time visiting a Slashdot page: http://yro.slashdot.org/story/15/01/24/2152253/openssl-102-released

This seems pretty explosive.

Comment 4

[Benjamin Kerensa [:bkerensa]]

Looks like something in this changeset blew it up https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=c18776175a69

digging through the patches

Comment 5

[Till Schneidereit [:till]]

WFM with the same system configuration and build. Do you have any addons installed that might be causing this?

Comment 6

[Robin Whittleton]

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.

Comment 7

[Till Schneidereit [:till]]

It probably would, yes, so that'd be great. You can always go back to your original profile if you want to restore it.

Comment 8

[Robin Whittleton]

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.

Comment 9

[Robin Whittleton]

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

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

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

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

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.

Comment 12

[Robin Whittleton]

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.

Comment 13

[Jan de Mooij [:jandem]]

Would be great if somebody could bisect :)

Comment 14

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:

Comment 15

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0c329304fe62&tochange=f313202b1e25

Suspect: Bug 1118894

Comment 16

[Emanuel Hoogeveen [:ehoogeveen]]

I can confirm that range.

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Alice0775 White from comment #15)
> Pushlog:

You rock, thanks.

Comment 18

[Emanuel Hoogeveen [:ehoogeveen]]

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).

Comment 19

[Emanuel Hoogeveen [:ehoogeveen]]

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.

Comment 20

[Dan Gohman [:sunfish]]

Attachment: osr-yeah-no.patch

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.

Comment 21

[Alice0775 White]

@Dan Gohman [:sunfish]
Just confirmation, Are you using latest LastPass version 3.1.77 ?
( https://lastpass.com/ )

Comment 22

[Dan Gohman [:sunfish]]

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!

Comment 23

[Emanuel Hoogeveen [:ehoogeveen]]

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).

Comment 25

[Dan Gohman [:sunfish]]

Attachment: dont-touch-loops-with-osr.patch

Ok, here's a slightly nicer version of the patch. This fixes the crash, for me.

Comment 26

[Dan Gohman [:sunfish]]

Attachment: aurora.patch

This is a version which applies to mozilla-aurora and mozilla-beta.

Comment 27

[Dan Gohman [:sunfish]]

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.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Do we have evidence that this crash is exploitable? This bug needs a security rating before it can get sec-approval.

Comment 29

[Dan Gohman [:sunfish]]

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.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8554762 [details] [diff] [review]
dont-touch-loops-with-osr.patch

sec-approval+ for checkin.

Comment 31

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec859a233c28

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec859a233c28

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Baked for a couple days without any other known issues. I think we're good for approval requests now? :)

Comment 34

[Dan Gohman [: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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/829537f1f271
https://hg.mozilla.org/releases/mozilla-beta/rev/bbdf662015c0

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/829537f1f271
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/205b89828e36
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/b961877cacba

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/205b89828e36
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/b961877cacba