Closed Bug 1412145 Opened 2 years ago Closed 2 years ago

Backpointer in CSSOM objects need to be cleared when they are unlinked

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 58+ fixed
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 + fixed
firefox59 + fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

(Keywords: csectype-uaf, sec-audit, sec-high, Whiteboard: [adv-main58+][adv-esr52.6+][post-critsmash-triage])

Attachments

(2 files, 2 obsolete files)

Just for what was observed in bug 1410808 which may lead to use-after-free.
Actually both Servo and Gecko can have this issue...
Attached patch patch (obsolete) — Splinter Review
Attachment #8922583 - Flags: review?(bzbarsky)
Group: core-security → layout-core-security
Assignee: nobody → xidorn+moz
I'm sorry for the horrible lag here.  I will do this review first thing on Monday...
Argh.  I still haven't had a chance to sort through this carefully, and now we're going to end up shipping this in 57.  :(  I'm really really sorry....

I am unfortunately off tomorrow, but I will review this first thing Monday morning.  I know that's what I said last week.  :(
This is wontfix for 57 at this stage.
I can undestand that given the timing.  That said, this is a security regression in 57, and our fuzzers have found it at least twice in the last few weeks.  We should probably start planning on a dot-release that will include this fix...

I'll make time this weekend to do the review, so at least that's out of the way.
Flags: needinfo?(jcristau)
Flags: needinfo?(dveditz)
We should get a review and land the fix on m-c to make sure this doesn't cause regressions (but request sec-approval so Al and I can argue over timing). If it's after 57 release then uplift to 58 beta. We'll need a fix for the non-Servo parts for 52-ESR so might as well write that patch, too.

Not sure how to mark this for a point-release consideration. I added a tracking nom ('?') but I'm not sure if those will be looked at anymore. Note bug 1410808 was found by an external reporter -- it's not just our own fuzzers finding it.
Flags: needinfo?(dveditz)
Comment on attachment 8922583 [details] [diff] [review]
patch

>+  virtual ~ServoKeyframeList() {
>+    MOZ_ASSERT(!mParentRule, "Backpointer should have been cleared");
>+    MOZ_ASSERT(!mStyleSheet, "Backpointer should have been cleared");
>+    DropAllRules();
>+  }

Why do we need to walk over the list of rules here?

And if we _do_, why do we need to clear mRules afterward, or touch mRawRule?  We're in the destructor; those will happen automatically anyway...  This isn't wrong per se, just seems like unnecessary work.

>+++ b/layout/style/nsCSSRules.cpp
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(ImportRule, CSSImportRule)
>+  tmp->mMedia = nullptr;

NS_IMPL_CYCLE_COLLECTION_UNLINK(mMedia)

r=me.  I'm really sorry again for the lag....
Attachment #8922583 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)
> Comment on attachment 8922583 [details] [diff] [review]
> patch
> 
> >+  virtual ~ServoKeyframeList() {
> >+    MOZ_ASSERT(!mParentRule, "Backpointer should have been cleared");
> >+    MOZ_ASSERT(!mStyleSheet, "Backpointer should have been cleared");
> >+    DropAllRules();
> >+  }
> 
> Why do we need to walk over the list of rules here?

Hmmm... I'm not sure what I was thinking. I guess you are right that this seems to be unnecessary as far as the assertions hold.
Since it is not completely clear to me when I would back to work... so ni? myself for landing this patch as well as writing patch for esr52 uplifting.
Flags: needinfo?(xidorn+moz)
Comment on attachment 8922583 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
May not be too hard? It indicates a possible pattern for use-after-free on some CSSOM objects with and without stylo.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I'm not sure whether "backpointer" would be something sensitive. If yes, then yes, the commit message as well as several new assertions are mentioning this word. Otherwise I don't think there are anything else being a problem.

Which older supported branches are affected by this flaw?
ESR52 has the problem for one CSSOM object.

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The ESR52 fix would be simply part of this patch. It should be an easy one, and I don't think it's risky.

How likely is this patch to cause regressions; how much testing does it need?
I would say unlikely, but not impossible. In general, I think as far as we can successfully land it (passing all existing tests), it should be fine enough.
Attachment #8922583 - Flags: sec-approval?
Priority: -- → P2
Sec-approval+ for checkin on November 28, two weeks into the new cycle. Please do not check in before that date.

We'll want this on other branches so branch patches should be made and nominated at that time as well.
Whiteboard: [checkin on 11/28]
Attachment #8922583 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(xidorn+moz)
Ritu, Xidorn thinks this sec bug fix would be a good ride-along if there is a 57.0.1 dot release.

Al gave sec-approval+ for checkin on November 28. If there is a 57.0.1 release before November 28, how and where would we land this fix?
Flags: needinfo?(rkothari)
Also note that this patch isn't completely risk-free... We should have it in nightly before baking it into a dot release.
(In reply to Chris Peterson [:cpeterson] from comment #13)
> Ritu, Xidorn thinks this sec bug fix would be a good ride-along if there is
> a 57.0.1 dot release.
> 
> Al gave sec-approval+ for checkin on November 28. If there is a 57.0.1
> release before November 28, how and where would we land this fix?

I have added this to my list of 57.x ride-along tracking list. Depending on the timing, risk assessment and input from sec team (Al, Dan) we will consider this one for inclusion.
Flags: needinfo?(rkothari)
It seems that the backport patch for esr is more involving than I thought...
Attached patch patch for esr uplift (obsolete) — Splinter Review
Basically the Gecko part... but since the related code has been evolved quite a bit since 52, this patch isn't simply a striped version of the original patch, so it may be worth another review.
Flags: needinfo?(xidorn+moz)
Attachment #8928353 - Flags: review?(bzbarsky)
Comment on attachment 8928353 [details] [diff] [review]
patch for esr uplift

r=me
Attachment #8928353 - Flags: review?(bzbarsky) → review+
[Tracking Requested - why for this release]:

ESR 52 is affected. Xidorn has an r+'d patch to backport his fix to ESR 52.
I think given comment 14 and the fact that this requires an esr52 release at the same time, it's not worth trying to rush this in a 57 dot release.
Flags: needinfo?(jcristau)
Dan and I discussed this and don't think we should take this on the point release. It is an internal find and can ride the release train.
Need a null-check there. Fixed and landed again:
https://hg.mozilla.org/integration/autoland/rev/43eb913e5974cbb9b6206bb4ca307443148b97e3
Flags: needinfo?(xidorn+moz)
https://hg.mozilla.org/mozilla-central/rev/43eb913e5974cbb9b6206bb4ca307443148b97e3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
It's not clear whether I should request uplift for it given comment 21.
Yes, you should (and ESR52). We just don't want it for an Fx57 dot release.
This is the patch landed.
Attachment #8922583 - Attachment is obsolete: true
Attachment #8933171 - Flags: review+
Comment on attachment 8933171 [details] [diff] [review]
updated patch, r=bz

Approval Request Comment
[Feature/Bug causing the regression]: security issue since long ago
[User impact if declined]: may be exploited
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: just landed a day ago
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: shouldn't be risky
[Why is the change risky/not risky?]: it just resets some pointers when an object is unlinked by cycle collector, so it shouldn't affect behavior
[String changes made/needed]: n/a
Attachment #8933171 - Flags: approval-mozilla-beta?
Comment on attachment 8928353 [details] [diff] [review]
patch for esr uplift

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: security bug
Fix Landed on Version: 59
Risk to taking this patch (and alternatives if risky): this patch shouldn't be risky
String or UUID changes made by this patch: n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8928353 - Flags: approval-mozilla-esr52?
Comment on attachment 8933171 [details] [diff] [review]
updated patch, r=bz

Fix a sec-high. Beta58+ & ESR52+.
Attachment #8933171 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8928353 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Group: layout-core-security → core-security-release
Duplicate of this bug: 1415901
Backed out from ESR52 for mass crashes.
https://hg.mozilla.org/releases/mozilla-esr52/rev/9f29f1543819

https://treeherder.mozilla.org/logviewer.html#?job_id=152370725&repo=mozilla-esr52

0  xul.dll!mozilla::css::MediaRule::CheckForRightParticipant() [nsCSSRules.h:14a389d40329 : 56 + 0x38]
   eip = 0x6e9e14d7   esp = 0x00b5f51c   ebp = 0x00b5f520   ebx = 0x00b5f604
   esi = 0x09a66c70   edi = 0x01218570   eax = 0x720c7b74   ecx = 0x71d506ef
   edx = 0xff1fe000   efl = 0x00000202
   Found by: given as instruction pointer in context
1  xul.dll!DowncastCCParticipantImpl<mozilla::css::GroupRule,1>::Run(void *) [nsCycleCollectionParticipant.h:14a389d40329 : 303 + 0x6]
   eip = 0x6ea17251   esp = 0x00b5f528   ebp = 0x00b5f52c
   Found by: call frame info
2  xul.dll!mozilla::css::GroupRule::cycleCollection::Traverse(void *,nsCycleCollectionTraversalCallback &) [nsCSSRules.cpp:14a389d40329 : 430 + 0xf]
   eip = 0x6e9d25c9   esp = 0x00b5f534   ebp = 0x00b5f54c
   Found by: call frame info
3  xul.dll!CCGraphBuilder::BuildGraph(js::SliceBudget &) [nsCycleCollector.cpp:14a389d40329 : 2282 + 0x7]
   eip = 0x6ccb0ec9   esp = 0x00b5f554   ebp = 0x00b5f570
   Found by: call frame info
4  xul.dll!nsCycleCollector::MarkRoots(js::SliceBudget &) [nsCycleCollector.cpp:14a389d40329 : 2879 + 0x16]
   eip = 0x6ccb68c9   esp = 0x00b5f578   ebp = 0x00b5f59c
   Found by: call frame info
5  xul.dll!nsCycleCollector::Collect(ccType,js::SliceBudget &,nsICycleCollectorListener *,bool) [nsCycleCollector.cpp:14a389d40329 : 3655 + 0x7]
   eip = 0x6ccb1d4c   esp = 0x00b5f5a4   ebp = 0x00b5f5e4
   Found by: call frame info
6  xul.dll!nsCycleCollector::ShutdownCollect() [nsCycleCollector.cpp:14a389d40329 : 3592 + 0x10]
   eip = 0x6ccbb5b0   esp = 0x00b5f5ec   ebp = 0x00b5f624
   Found by: call frame info
7  xul.dll!nsCycleCollector::Shutdown(bool) [nsCycleCollector.cpp:14a389d40329 : 3883 + 0x6]
   eip = 0x6ccbb581   esp = 0x00b5f62c   ebp = 0x00b5f630
   Found by: call frame info
8  xul.dll!nsCycleCollector_shutdown(bool) [nsCycleCollector.cpp:14a389d40329 : 4201 + 0x10]
   eip = 0x6ccbe9a0   esp = 0x00b5f638   ebp = 0x00b5f654
   Found by: call frame info
9  xul.dll!mozilla::ShutdownXPCOM(nsIServiceManager *) [XPCOMInit.cpp:14a389d40329 : 1019 + 0x6]
   eip = 0x6cd3b56a   esp = 0x00b5f65c   ebp = 0x00b5f684
   Found by: call frame info
10 xul.dll!XRE_XPCShellMain [XPCShellImpl.cpp:14a389d40329 : 1606 + 0x6]
   eip = 0x6d428744   esp = 0x00b5f68c   ebp = 0x00b5f990
   Found by: call frame info
11 xul.dll!XPCVariant::cycleCollection::Unlink(void *) [XPCVariant.cpp:14a389d40329 : 94 + 0xa]
   eip = 0x6d4297ac   esp = 0x00b5f6a0   ebp = 0x00b5f990
   Found by: stack scanning
Flags: needinfo?(xidorn+moz)
And a maybe-helpful debug assertion:
Assertion failure: p == &_cycleCollectorGlobal (MediaRule should QI to its own CC participant), at layout/style/nsCSSRules.h:56
An updated version of attachment 8928353 [details] [diff] [review] which got approval in comment 31.
Attachment #8928353 - Attachment is obsolete: true
Flags: needinfo?(xidorn+moz)
Attachment #8938213 - Flags: review+
Attachment #8938213 - Flags: approval-mozilla-esr52?
Comment on attachment 8938213 [details] [diff] [review]
updated patch for esr uplift

This doesn't need re-approval since it's a just one-line change from the last patch.
Attachment #8938213 - Flags: approval-mozilla-esr52?
Whiteboard: [adv-main58+][adv-esr52.6+]
Flags: qe-verify-
Whiteboard: [adv-main58+][adv-esr52.6+] → [adv-main58+][adv-esr52.6+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.