Closed
Bug 1167782
Opened 9 years ago
Closed 9 years ago
crash in nsHTMLCSSStyleSheet::RulesMatching(PseudoElementRuleProcessorData*)
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: bgrins, Assigned: dholbert)
References
Details
(5 keywords, Whiteboard: [adv-main39-][adv-esr38.1-])
Crash Data
Attachments
(3 files, 1 obsolete file)
123 bytes,
text/html
|
Details | |
1.09 KB,
patch
|
dbaron
:
review+
Sylvestre
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr38+
jocheng
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
130 bytes,
text/html
|
Details |
This bug was filed from the Socorro interface and is report bp-f23be59d-ab56-4a14-ab16-a28ed2150522. ============================================================= STR: 1) Open the attached page with the following contents: <html> <body> <script> console.log(window.getComputedStyle(document.body, "::-moz-color-swatch")) </script> </body> </html> 2) Open the web console I get a crash on Nightly, Release, and a local build.
Reporter | ||
Comment 1•9 years ago
|
||
I can also reproduce when running this in the Browser Console: let DOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils); let domRules = DOMUtils.getCSSStyleRules(gBrowser, ":-moz-color-swatch");
Assignee | ||
Comment 2•9 years ago
|
||
The good news is that it's a crash on a null pointer, so it's probably not exploitable. Before the crash, I see this assertion failure: { Assertion failure: aData->mPseudoElement (If pseudo element is supposed to support style attribute, it must have a pseudo element set), at layout/style/nsHTMLCSSStyleSheet.cpp:110 }
Assignee | ||
Comment 3•9 years ago
|
||
Regression range (using STR from comment 0, plus clicking on the "[object CSS2Properties]" object that shows up in the console in builds from ~2013, before we tried to expand this object automatically): http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=177bf37a49f5&tochange=21d97baadc05 So, this has been an issue ever since we added :-moz-color-swatch (as part of <input type="color">). I tried a few other pseudo-elements -- e.g. :-moz-number-spin-box -- and wasn't able to reproduce this with any of them. So there must be something special that we're missing for :-moz-color-swatch that makes it trigger this.
Keywords: regression
Assignee | ||
Comment 4•9 years ago
|
||
i.e. this is a regression from bug 875275, this cset in particular: http://hg.mozilla.org/mozilla-central/rev/3975fe261080 That's the cset that added the assertion in comment 2, incidentally.
Assignee | ||
Updated•9 years ago
|
status-firefox38:
--- → affected
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox-esr31:
--- → affected
status-firefox-esr38:
--- → affected
OS: Mac OS X → All
Hardware: Unspecified → All
Reporter | ||
Comment 5•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #3) > So, this has been an issue ever since we added :-moz-color-swatch (as part > of <input type="color">). Our testing from Bug 1163183 confirms this. We had looped through all of the pseudo elements from this list: https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPseudoElementList.h and used DOMUtils.getCSSStyleRules for each. moz-color-swatch was the only one that we detected as causing a crash.
Reporter | ||
Comment 6•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #3) > I tried a few other pseudo-elements -- e.g. :-moz-number-spin-box -- and > wasn't able to reproduce this with any of them. So there must be something > special that we're missing for :-moz-color-swatch that makes it trigger this. Meant to quote this part of the message in Comment 5 - we had tested all of the other elements in that list and only saw the problem with this one.
Assignee | ||
Comment 7•9 years ago
|
||
Thanks -- yup, the special thing about :-moz-color-swatch is its CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE flag, which sends us down the code path with the assertion and the crash.
Assignee | ||
Comment 8•9 years ago
|
||
One possible way forward: just promote the failing assertion to an explicit check (i.e., only check the psuedo-element for a style attribute if we actually have a non-null element). dbaron, is it sane that we end up with a null psuedo-element at this point in the code, in nsHTMLCSSStyleSheet::RulesMatching? (given a query for the style of pseudo element ::-moz-color-swatch from document.body -- and I think this pseudo-element only exists on <input type="color">)
Attachment #8609673 -
Flags: feedback?(dbaron)
Assignee | ||
Comment 9•9 years ago
|
||
(Sorry, attached wrong patch file)
Attachment #8609673 -
Attachment is obsolete: true
Attachment #8609673 -
Flags: feedback?(dbaron)
Attachment #8609675 -
Flags: feedback?(dbaron)
Assignee | ||
Comment 10•9 years ago
|
||
Here's a simpler testcase that doesn't bother with console.log; it just directly accesses the computed style instead. (So, it just crashes.)
Comment 11•9 years ago
|
||
From the proposed fix I'm guessing this is just a null-pointer crash and not a security issue, correct?
Flags: needinfo?(dholbert)
Keywords: csectype-nullptr,
sec-other
Assignee | ||
Comment 12•9 years ago
|
||
Yup. (The crash report shows it as a null deref, too.)
Flags: needinfo?(dholbert)
Updated•9 years ago
|
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.1S:
--- → affected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
Comment 13•9 years ago
|
||
Ending up there with a null pseudo-element is in fact sane.
Assignee | ||
Comment 14•9 years ago
|
||
Thanks. I'll take the bug and call the strawman fix an actual fix, then. (sanity-check: I think we can open this bug up (and land a testcase with the fix), right? IIRC null-deref crashes are generally public, since they're much harder [if possible at all] to exploit. In this case we're calling a virtual function on a null pointer.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite?
Assignee | ||
Updated•9 years ago
|
Attachment #8609675 -
Attachment description: strawman fix v1: promote the assertion to an actual check → fix v1: promote the assertion to an actual null-check
Attachment #8609675 -
Flags: feedback?(dbaron) → review?(dbaron)
Comment on attachment 8609675 [details] [diff] [review] fix v1: promote the assertion to an actual null-check ok, r=dbaron. I guess if we ever fail to do this in the actual codepaths that matter for style calculation for layout, we'll just notice bugs.
Attachment #8609675 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #14) > (sanity-check: I think we can open this bug up (and land a testcase with the > fix), right? IIRC null-deref crashes are generally public geekboy & mccr8 confirmed my recollection on this, in #security, so I'm opening this up, and I won't bother doing the sec-approval dance or holding off on landing the testcase. Thanks for the review!
Group: core-security
Assignee | ||
Comment 17•9 years ago
|
||
Try run, with crashtest included: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aba953d7f6ea (I verified locally that the crashtest fails without the fix.)
Version: unspecified → Trunk
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8609675 [details] [diff] [review] fix v1: promote the assertion to an actual null-check This is a nice, safe fix, for an easy-to-trigger (non-exploitable) crash. I think we should backport this to Aurora. Approval Request Comment [Feature/regressing bug #]: bug 875275, <input type="color"> (color picker widget) [User impact if declined]: crash (from a null dereference) on web content like the attached testcases. (Unlikely that this would be accidentally triggered with innocent web content; likely just a quick DOS vector.) [Describe test coverage new/current, TreeHerder]: Crashtest included. We have a folder of reftests for the feature that introduced this, too (layout/reftests/input/color) [Risks and why]: Low risk. Just adding a null-check to prevent a crash. [String/UUID change made/needed]: none
Attachment #8609675 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8609675 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 20•9 years ago
|
||
As it is quite safe, any reason why you didn't request uplift to beta (39) ?
Assignee | ||
Comment 21•9 years ago
|
||
Only fear of rejection. :) Yeah, let's go ahead & take this on beta too.
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8609675 [details] [diff] [review] fix v1: promote the assertion to an actual null-check Approval Request Comment: see comment 19
Attachment #8609675 -
Flags: approval-mozilla-beta?
Comment on attachment 8609675 [details] [diff] [review] fix v1: promote the assertion to an actual null-check This seems like a simple fix. It was approved for Aurora. Now approving for uplift to Beta.
Attachment #8609675 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/d0e5e7c59985
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 25•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/fea77ea6d6a2
Flags: in-testsuite? → in-testsuite+
Comment 26•9 years ago
|
||
Should we consider this for esr38/b2g37 as well? It's a bit edge-casey, but it's also a pretty low-risk stability fix.
Flags: needinfo?(dholbert)
Assignee | ||
Comment 28•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #26) > Should we consider this for esr38/b2g37 as well? It's a bit edge-casey, but > it's also a pretty low-risk stability fix. Sure, might as well.
Flags: needinfo?(dholbert)
Assignee | ||
Comment 29•9 years ago
|
||
Comment on attachment 8609675 [details] [diff] [review] fix v1: promote the assertion to an actual null-check [Approval Request Comment] (Approval questions answered in comment 19, except these ESR-specifc ones): If this is not a sec:{high,crit} bug, please state case for ESR consideration: Easy, safe stability fix. Fix Landed on Version: Landed on mozilla-central with version 41, and has been backported to 40 and 39. (current aurora/beta)
Attachment #8609675 -
Flags: approval-mozilla-esr38?
Attachment #8609675 -
Flags: approval-mozilla-b2g37?
Comment 30•9 years ago
|
||
Comment on attachment 8609675 [details] [diff] [review] fix v1: promote the assertion to an actual null-check Approving as it increases stability
Attachment #8609675 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•9 years ago
|
Attachment #8609675 -
Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Updated•9 years ago
|
tracking-firefox-esr38:
--- → 39+
Whiteboard: [adv-main39-][adv-esr38.1-]
You need to log in
before you can comment on or make changes to this bug.
Description
•