1230639 - "ASSERTION: All class values should be StoresOwnData and parsedvia Element::BeforeSetAttr, so available here"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: All class values should be StoresOwnData and parsedvia Element::BeforeSetAttr, so available here: 'otherClasses || aData->mModType == nsIDOMMutationEvent::REMOVAL', file layout/style/nsCSSRuleProcessor.cpp, line 3018

This assertion is part of code added in https://hg.mozilla.org/mozilla-central/rev/ec4b62affc04 (bug 1184842)

Btw, there's a typo in the assertion ("parsedvia").

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, makes sense.  This is an attribute named "class" in a random namespace.  AttributeRuleProcessorData only stores the attr name, not the namespace.

We should probably thread the namespace id through from RestyleManager::AttributeWillChange/AttributeChanged to AttributeRuleProcessorData and then check it here before ever entering this code block.

I'm a bit more worried about cases when otherClasses ends up non-null but isn't an attr array value, if that can happen, because then we'll jump into the weeds here.

> Btw, there's a typo in the assertion ("parsedvia").

The preprocessor strikes again.  The actual code is:

3015       NS_ASSERTION(otherClasses ||
3016                    aData->mModType == nsIDOMMutationEvent::REMOVAL,
3017                    "All class values should be StoresOwnData and parsed"
3018                    "via Element::BeforeSetAttr, so available here");

Comment 3

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Possibly critical security issue.

Comment 4

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

Tracking for 43+. We are building the release candidate on Monday. So if this is critical, and you land a fix for this,  please request uplift to m-r.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Propagate the namespace ID to AttributeRuleProcessorData on attribute changes

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8696197 [details] [diff] [review]
Propagate the namespace ID to AttributeRuleProcessorData on attribute changes

Maybe also add the trailing space to the first line of the assertion text to fix the "parsedvia"?

r=dbaron

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8696197 [details] [diff] [review]
Propagate the namespace ID to AttributeRuleProcessorData on attribute changes

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Probably not.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No, but I think the patch itself makes it pretty clear the issue is class attributes in some nontrivial namespace.

Which older supported branches are affected by this flaw? 42 and newer.

If not all supported branches, which bug introduced the flaw?  Bug 1184842.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  This should be pretty straightforward and safe to backport.

How likely is this patch to cause regressions; how much testing does it need?  I don't think this is very regression-prone.

Comment 8

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

Ideas on a security rating for this, Dan or David?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Third paragraph of comment 2 suggest sec-critical, I guess.

Comment 10

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

Ah, yes. Thanks.

Approved for checkin on 12/29 (two weeks into next cycle).

Comment 11

[Sylvestre Ledru [:Sylvestre]]

David, as bz is in PTO, could you fill the uplift requests? Thanks

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f916b88c87b5

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f916b88c87b5

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8696197 [details] [diff] [review]
Propagate the namespace ID to AttributeRuleProcessorData on attribute changes

Approval Request Comment
[Feature/regressing bug #]: maybe bug 1184842, although I'm not actually sure
[User impact if declined]: potentially-critical crashes
[Describe test coverage new/current, TreeHerder]: none for the bug, but the underlying code has reasonably decent coverage, although maybe not a whole lot with namespaces
[Risks and why]: a little bit of risk to handling of namespaced attributes, which are probably quite rarely used on the Web, although the patch appears pretty straightforward, so things should probably be fine
[String/UUID change made/needed]: no

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8696197 [details] [diff] [review]
Propagate the namespace ID to AttributeRuleProcessorData on attribute changes

Sec-crit rated and has been on Nightly for a week, beta44+, aurora45+

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1e37941702ec

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9cd8b59d1a9d