Closed Bug 1167782 Opened 9 years ago Closed 9 years ago

crash in nsHTMLCSSStyleSheet::RulesMatching(PseudoElementRuleProcessorData*)

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed
firefox-esr31 --- wontfix
firefox-esr38 39+ fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: bgrins, Assigned: dholbert)

References

Details

(5 keywords, Whiteboard: [adv-main39-][adv-esr38.1-])

Crash Data

Attachments

(3 files, 1 obsolete file)

Attached file moz-color-swatch.html
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.
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");
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
}
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
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.
OS: Mac OS X → All
Hardware: Unspecified → All
(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.
(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.
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.
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)
(Sorry, attached wrong patch file)
Attachment #8609673 - Attachment is obsolete: true
Attachment #8609673 - Flags: feedback?(dbaron)
Attachment #8609675 - Flags: feedback?(dbaron)
Here's a simpler testcase that doesn't bother with console.log; it just directly accesses the computed style instead. (So, it just crashes.)
From the proposed fix I'm guessing this is just a null-pointer crash and not a security issue, correct?
Flags: needinfo?(dholbert)
Yup. (The crash report shows it as a null deref, too.)
Flags: needinfo?(dholbert)
Ending up there with a null pseudo-element is in fact sane.
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?
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+
(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
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
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?
Blocks: 875275
Keywords: assertion
Attachment #8609675 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
As it is quite safe, any reason why you didn't request uplift to beta (39) ?
Only fear of rejection. :)

Yeah, let's go ahead & take this on beta too.
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
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.
(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)
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 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+
Attachment #8609675 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Whiteboard: [adv-main39-][adv-esr38.1-]
You need to log in before you can comment on or make changes to this bug.