Closed
Bug 1358200
(CVE-2017-7769)
Opened 8 years ago
Closed 8 years ago
Hide Tooltip with Stylish crashes Firefox
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox-esr45 | --- | unaffected |
thunderbird_esr45 | --- | unaffected |
thunderbird_esr52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox53 | - | wontfix |
firefox54 | + | fixed |
firefox55 | + | fixed |
People
(Reporter: github, Assigned: dholbert)
References
Details
(4 keywords, Whiteboard: [post-critsmash-triage][adv-main54+])
Crash Data
Attachments
(1 file)
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
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
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsXULTooltipListener::FindTooltip ]
Has STR: --- → yes
Component: Untriaged → XUL
Ever confirmed: true
Keywords: crash
Product: Firefox → Core
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0932c3f7208bd6629a7e1dfb6f87501900e94a0a&tochange=308bc1917b91c428eeaffe19940018f1373c3e82
Blocks: 1321394
Has Regression Range: --- → yes
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox55:
--- → affected
status-firefox-esr52:
--- → unaffected
Keywords: regression
Version: unspecified → 53 Branch
Assignee | ||
Comment 4•8 years ago
|
||
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.
Comment 6•8 years ago
|
||
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.
Reporter | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
(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...
Assignee | ||
Comment 10•8 years ago
|
||
(by "the changeset there" I mean Bug 1316556's patch.)
Tentatively marking as a regression from bug 1316556.
Assignee | ||
Comment 11•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8860557 -
Flags: review?(mats)
Assignee | ||
Comment 14•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
status-firefox-esr45:
--- → unaffected
Updated•8 years ago
|
Attachment #8860557 -
Flags: review?(mats) → review+
Comment 15•8 years ago
|
||
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•8 years ago
|
||
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.
Assignee | ||
Comment 17•8 years ago
|
||
That is comforting. I'll go ahead and land this, then, since sec-approval isn't needed for sec-low bugs.
Assignee | ||
Comment 18•8 years ago
|
||
Ah, inbound is closed at the moment. I'll land this later on then.
Flags: needinfo?(dholbert)
Assignee | ||
Comment 19•8 years ago
|
||
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/06c7c56497ad4e34729d6e54da8408ad868ec8de
Flags: needinfo?(dholbert)
Assignee | ||
Comment 20•8 years ago
|
||
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?
Assignee | ||
Comment 21•8 years ago
|
||
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.
Comment 23•8 years ago
|
||
AFAICT this landed in m-c:
https://hg.mozilla.org/mozilla-central/rev/06c7c56497ad4e34729d6e54da8408ad868ec8de
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 24•8 years ago
|
||
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+
Assignee | ||
Comment 25•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
Group: core-security → core-security-release
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+]
Updated•7 years ago
|
Alias: CVE-2017-7769
Updated•7 years ago
|
Keywords: csectype-uaf,
csectype-uninitialized
Reporter | ||
Comment 26•7 years ago
|
||
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?
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(abillings)
Comment 27•7 years ago
|
||
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)
Comment 28•7 years ago
|
||
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•7 years ago
|
||
The "deprecated storage API" was making it hard to debug (fatal asserts), not related to the layout bug triggered by the add-on.
Updated•7 years ago
|
Keywords: csectype-uaf
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•