Bug 1358200 (CVE-2017-7769)

Hide Tooltip with Stylish crashes Firefox

RESOLVED FIXED in Firefox 54

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: github, Assigned: dholbert)

Tracking

(4 keywords)

53 Branch
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, thunderbird_esr45 unaffected, thunderbird_esr52 unaffected, firefox-esr52 unaffected, firefox53- wontfix, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main54+], crash signature)

Attachments

(1 attachment)

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
Happens with FF53 and FF5
Version: 55 Branch → unspecified
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsXULTooltipListener::FindTooltip ]
Has STR: --- → yes
Component: Untriaged → XUL
Ever confirmed: true
Keywords: crash
Product: Firefox → Core
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.)
(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.
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...)
(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.
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?
Flags: needinfo?(jseward)
(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...
(by "the changeset there" I mean Bug 1316556's patch.)

Tentatively marking as a regression from bug 1316556.
Blocks: 1316556
No longer blocks: 1321394
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.
Group: core-security
Flags: needinfo?(jseward)
Keywords: sec-high
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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)
Posted patch fix v1Splinter Review
Attachment #8860557 - Flags: review?(mats)
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.
Attachment #8860557 - Flags: review?(mats) → review+
Not sure the crash volume warrants consideration for ride-along status in the event of a dot release, but nominating it for tracking anyway.
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.
Keywords: sec-highsec-low
OS: Unspecified → All
Hardware: Unspecified → All
That is comforting. I'll go ahead and land this, then, since sec-approval isn't needed for sec-low bugs.
Ah, inbound is closed at the moment. I'll land this later on then.
Flags: needinfo?(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.
Attachment #8860557 - Flags: approval-mozilla-beta?
Attachment #8860557 - Flags: approval-mozilla-aurora?
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.
Attachment #8860557 - Flags: approval-mozilla-aurora?
This is so low volume in crash-stats that I think it would be better to let it ride with 54. Wontfix for 53.
AFAICT this landed in m-c:
https://hg.mozilla.org/mozilla-central/rev/06c7c56497ad4e34729d6e54da8408ad868ec8de
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8860557 [details] [diff] [review]
fix v1

fix a crash, beta54+, should be in 54b3
Attachment #8860557 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+]
Alias: CVE-2017-7769
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?
Flags: needinfo?(abillings)
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.
Flags: needinfo?(abillings)
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."
The "deprecated storage API" was making it hard to debug (fatal asserts), not related to the layout bug triggered by the add-on.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.