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

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
assertion, regression, sec-critical, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43 wontfix, firefox44+ fixed, firefox45+ fixed, firefox46 fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [adv-main44+])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8695997 [details]
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").
(Reporter)

Comment 1

2 years ago
Created attachment 8695998 [details]
stack
Blocks: 1184842
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-firefox42: --- → ?
tracking-firefox43: --- → ?
tracking-firefox44: --- → ?
tracking-firefox45: --- → ?
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.
tracking-firefox43: ? → +
tracking-firefox44: ? → +
tracking-firefox45: ? → +
Created attachment 8696197 [details] [diff] [review]
Propagate the namespace ID to AttributeRuleProcessorData on attribute changes
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).
status-firefox42: --- → wontfix
status-firefox43: --- → wontfix
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox-esr38: --- → ?
tracking-firefox42: ? → ---
tracking-firefox43: + → ---
Keywords: sec-critical
Whiteboard: [checkin on 12/29]
Attachment #8696197 - Flags: sec-approval? → sec-approval+
Keywords: regression
status-firefox-esr38: ? → unaffected
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]
https://hg.mozilla.org/integration/mozilla-inbound/rev/f916b88c87b5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f916b88c87b5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
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+
https://hg.mozilla.org/releases/mozilla-beta/rev/1e37941702ec
status-firefox44: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/9cd8b59d1a9d
status-firefox45: affected → fixed
Whiteboard: [adv-main44+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.