Closed Bug 1230639 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 + fixed
firefox45 + fixed
firefox46 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(4 keywords, Whiteboard: [adv-main44+])

Attachments

(3 files)

Attached file 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").
Attached file stack
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");
Group: dom-core-security
[Tracking Requested - why for this release]: Possibly critical security issue.
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8696197 - Flags: review?(dbaron)
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
Attachment #8696197 - Flags: review?(dbaron) → review+
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.
Attachment #8696197 - Flags: sec-approval?
Ideas on a security rating for this, Dan or David?
Flags: needinfo?(dveditz)
Flags: needinfo?(dbaron)
Third paragraph of comment 2 suggest sec-critical, I guess.
Flags: needinfo?(dbaron)
Ah, yes. Thanks.

Approved for checkin on 12/29 (two weeks into next cycle).
Keywords: sec-critical
Whiteboard: [checkin on 12/29]
Attachment #8696197 - Flags: sec-approval? → sec-approval+
Keywords: regression
Flags: needinfo?(dveditz)
David, as bz is in PTO, could you fill the uplift requests? Thanks
Flags: needinfo?(dbaron)
Keywords: checkin-needed
Whiteboard: [checkin on 12/29]
Group: dom-core-security → core-security-release
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
Flags: needinfo?(dbaron)
Attachment #8696197 - Flags: approval-mozilla-beta?
Attachment #8696197 - Flags: approval-mozilla-aurora?
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+
Attachment #8696197 - Flags: approval-mozilla-beta?
Attachment #8696197 - Flags: approval-mozilla-beta+
Attachment #8696197 - Flags: approval-mozilla-aurora?
Attachment #8696197 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main44+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.