1358200 - (CVE-2017-7769) Hide Tooltip with Stylish crashes Firefox

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Patrick Albrecht]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170420030346

Steps to reproduce:

Create new profile
Install Stylish
Create Style with the follow Code:
/* AGENT_SHEET */

@-moz-document url("about:addons") {


tooltip{
display:none !important}}
Open about:addons
Hover a menupoint like addons or plugins


Actual results:

FF crashes


Expected results:

FF runs

Comment 1

[Patrick Albrecht]

Some crashes:
https://crash-stats.mozilla.com/report/index/30672db3-0058-4a74-a7c9-dc6ed0170420
https://crash-stats.mozilla.com/report/index/5e170461-bd9f-4178-ad69-fff1c0170420
https://crash-stats.mozilla.com/report/index/b713f667-4d2c-4295-a959-651e70170420
https://crash-stats.mozilla.com/report/index/cb318f11-4ee3-4b28-985f-427690170420
https://crash-stats.mozilla.com/report/index/bp-48a6fb12-b137-4ece-a594-b970f1170420

Comment 2

[Patrick Albrecht]

Happens with FF53 and FF5

Comment 3

[YF (Yang)]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0932c3f7208bd6629a7e1dfb6f87501900e94a0a&tochange=308bc1917b91c428eeaffe19940018f1373c3e82

Comment 4

[Daniel Holbert [:dholbert]]

Does this happen in the default build/localization of Firefox?

If so, I'm initially skeptical of that regression range -- that's a change to nsRubyBaseContainerFrame.cpp , and that code is unlikely to be running at all on about:addons (unless you happen to have an addon with ruby markup in its title, but I don't know if that's even possible).

Also, I tried to reproduce using current Firefox release, but I was unable to.  (Also, latest Stylish seems partly-broken right now -- it wouldn't give me a create-new-style page half the time -- but it did seem to pop up that page when about:addons is the foreground tab.)

Comment 5

[YF (Yang)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Does this happen in the default build/localization of Firefox?

I see this issue in en-US and zh-CN.

> If so, I'm initially skeptical of that regression range -- that's a change
> to nsRubyBaseContainerFrame.cpp , and that code is unlikely to be running at
> all on about:addons (unless you happen to have an addon with ruby markup in
> its title, but I don't know if that's even possible).
> 
> Also, I tried to reproduce using current Firefox release, but I was unable
> to.  (Also, latest Stylish seems partly-broken right now -- it wouldn't give
> me a create-new-style page half the time -- but it did seem to pop up that
> page when about:addons is the foreground tab.)

Stylish 2.0.7. New the userstyle, reload the about:addons, mouse hover on "More" of the userstyle.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

I don't really believe this regression range... That seems to be something completely unrelated. I also failed to reproduce this issue on Firefox 54.0a2 (2017-04-18) (64-bit) on Windows.

If this is reproducible reliably, could you try a debug build (probably with mozregression) and see if there is any assertion triggered (I'm not sure how to check that with mozregression, though...)

Comment 7

[YF (Yang)]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #6)
> I don't really believe this regression range... That seems to be something
> completely unrelated. I also failed to reproduce this issue on Firefox
> 54.0a2 (2017-04-18) (64-bit) on Windows.

I can reproduce the problem steadily in 54.0a2 (2017-04-18) (32 bit) on Win10 1703.

> If this is reproducible reliably, could you try a debug build (probably with
> mozregression) and see if there is any assertion triggered (I'm not sure how
> to check that with mozregression, though...)

I did not find a workable debug package from ftp and mozregression, they started and then crashed.

Comment 8

[Patrick Albrecht]

I never used mozregression before so I'm not quite sure if I used it correctly

But Nightly 2016-12-01 was the last good build, NIghtly 2016-12-02 the first bad one.

This is the log from Mozregression:
2017-04-21T23:23:45: INFO : Narrowed inbound regression window from [78e008ce, db70aca7] (3 revisions) to [c63c761e, db70aca7] (2 revisions) (~1 steps left)
2017-04-21T23:23:45: DEBUG : Starting merge handling...
2017-04-21T23:23:45: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=db70aca765dc6400864e16701f4c9d42866ad713&full=1
2017-04-21T23:23:45: DEBUG : Found commit message:
Bug 1316556 - Remove zeroing allocation in class nsIPresShell.  r=dbaron.

2017-04-21T23:23:45: INFO : The bisection is done.
2017-04-21T23:23:46: INFO : Stopped

The URL is:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=78e008ce3a8c40c79f41567ede23e7de83d614dc&tochange=db70aca765dc6400864e16701f4c9d42866ad713


I hope I set the needinfo correct?

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to YF (Yang) from comment #7)
> > If this is reproducible reliably, could you try a debug build (probably with
> > mozregression) and see if there is any assertion triggered (I'm not sure how
> > to check that with mozregression, though...)
> 
> I did not find a workable debug package from ftp and mozregression, they
> started and then crashed.

I filed bug 1358656 for this debug build crash. (It seems Stylish is using a deprecated storage API which has recently started fatally asserting so that people will notice the deprecation.)

(In reply to Patrick Albrecht from comment #8)
> This is the log from Mozregression:
[...]
> Bug 1316556 - Remove zeroing allocation in class nsIPresShell.  r=dbaron.
[...]
> The URL is:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=78e008ce3a8c40c79f41567ede23e7de83d614dc&tochange=db70aca765dc6400864e16701f4c9d42866ad713

Thanks! That's near the earlier regression-range from comment 3 (within a day), but the changeset there is more believable as something that could've caused this...

Comment 10

[Daniel Holbert [:dholbert]]

(by "the changeset there" I mean Bug 1316556's patch.)

Tentatively marking as a regression from bug 1316556.

Comment 11

[Daniel Holbert [:dholbert]]

OK, I managed to catch this in a debug build, and it's indeed a regression from Bug 1316556's patch.

We're returning a member-variable that's uninitialized. I think frame poisoning might mitigate any/most exploit risk here, but I'm not sure, so I'm marking as security-sensitive and tagging as sec-high for now.

Comment 12

[Daniel Holbert [:dholbert]]

As shown in the crash reports, we're crashing in nsXULTooltipListener::FindTooltip -- and specifically, we're crashing when we dereference the return value from a call to "rootBox->GetDefaultTooltip".

rootBox is a nsRootBoxFrame, and nsRootBoxFrame::GetDefaultTooltip just returns a member-variable, which is never initialized. (though before bug 1316556, it was always initialized to null)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 14

[Daniel Holbert [:dholbert]]

The key change here is adding mDefaultTooltip to the init list -- otherwise it's never initialized, as shown by this DXR search:   https://dxr.mozilla.org/mozilla-central/search?q=mDefaultTooltip

But I'm also fixing up the init list style and pulling another member-var (from this same class) up into it, while I'm at it.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Not sure the crash volume warrants consideration for ride-along status in the event of a dot release, but nominating it for tracking anyway.

Comment 16

[Mats Palmgren (inactive)]

nsRootBoxFrame is only used for XUL documents and is allocated from the pres shell arena,
so I think this would be extremely hard if not impossible to exploit from a regular web page.

Comment 17

[Daniel Holbert [:dholbert]]

That is comforting. I'll go ahead and land this, then, since sec-approval isn't needed for sec-low bugs.

Comment 18

[Daniel Holbert [:dholbert]]

Ah, inbound is closed at the moment. I'll land this later on then.

Comment 19

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/06c7c56497ad4e34729d6e54da8408ad868ec8de

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8860557 [details] [diff] [review]
fix v1

Let's uplift this to Aurora and Beta.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1316556
[User impact if declined]: Potential for crashes.
[Is this code covered by automated tests?]: No. The crash can't be triggered except via cooperation from an add-on, as far as we know.
[Has the fix been verified in Nightly?]: No, but I verified that it works locally before I landed it.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. STR in comment 0.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]:  This is just initializing a variable to null (as it used to be, before bug 1316556 unintentionally converted it to being left uninitialized).  And callers null-check this variable before using it, so they shouldn't crash from a null-deref (but they can crash from an uninitialized-pointer-deref, in builds without my fix).
[String changes made/needed]: None.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8860557 [details] [diff] [review]
fix v1

Er, I guess there *is* no Aurora to uplift this to, since Aurora is going away.   So this only needs uplift to Beta54.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

This is so low volume in crash-stats that I think it would be better to let it ride with 54. Wontfix for 53.

Comment 23

[Julien Cristau [:jcristau]]

AFAICT this landed in m-c:
https://hg.mozilla.org/mozilla-central/rev/06c7c56497ad4e34729d6e54da8408ad868ec8de

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8860557 [details] [diff] [review]
fix v1

fix a crash, beta54+, should be in 54b3

Comment 25

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/releases/mozilla-beta/rev/419d01b08a9ef9d9f8698c73b573eb2ffeeef0c1

Comment 26

[Patrick Albrecht]

It seems like this issue is not listed on the Mozilla Foundation Security Advisory 2017-15 here (https://www.mozilla.org/en-US/security/advisories/mfsa2017-15/). Was it just forgotten or is there any reason for that?

Comment 27

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

Per discussion with Dveditz, as a "low" rated security issue that requires an extension to be exposed, it did not meet the bar to wind up in the advisories. Default users of Firefox are not affected by this issue.

Comment 28

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

Oh, and it is caused by Stylish using the "deprecated storage API" and, per comment 20, "The crash can't be triggered except via cooperation from an add-on, as far as we know."

Comment 29

[Daniel Veditz [:dveditz]]

The "deprecated storage API" was making it hard to debug (fatal asserts), not related to the layout bug triggered by the add-on.