1332550 - nsCSSKeyframesRule::DeleteRule leaves dangling pointers on the rule being deleted

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It never nulls out the parent rule or stylesheet of the rule being deleted.  So that rule can end up with dangling pointers.

This dates back to when this code was initially added in bug 435442.

I ran into this because in bug 851892 I'm making the cycle collector look at hte stylesheet pointers of rules, and I was getting random crashes.  :(

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Use our existing function for removing a rule at a given index from a group rule

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: tests

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8828643 [details] [diff] [review]
Use our existing function for removing a rule at a given index from a group rule

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Hard to say.  In my case, once I found the buggy line of code, it took about an hour to write the attached testcase; the hardest part was just ensuring that all refs to the parent rule were dropped.  The testcase _does_ use a deterministic way of triggering GC/CC, but that part is not hard.  How hard it is to construct an exploit from the testcase... well, we have a dangling pointer with a vtable, so I expect it's probably fairly easy for someone who does exploits for a living.  

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No.  In fact, I'd strongly consider landing this patch, with its current commit message, in a cover bug that's not security-sensitive.  If someone stops to look at it carefully, they would probably still realize that the new function being called nulls out some stuff the old function did not, but the hope would be that no one stops to look at it carefully if it's landed that way.

Which older supported branches are affected by this flaw?  All of them; this code is 5+ years old.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I expect this patch to backport as-is to pretty much all branches.  The context has changed a bit with the stylo work, but it should be simple to address that.

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

Note that this bug _does_ block landing bug 851892 (because the patches from that bug end up hitting crashes in automation due to this bug), which I'd really rather not sit on for too long if I can avoid it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: tests

Comment 5

[Daniel Veditz [:dveditz]]

bz needs to get bug 851892 landed but he's worried this one-line patch attached to a hidden bug would invite inspection that might not withstand 8 weeks of sitting on trunk (didn't take him that long to work up a triggering testcase). For mozilla-central we're going to bury this fix in the 20-part patchset for bug 851892 and keep this bug to track backports and trigger the security advisory.

Sometime in late Feb (maybe the week of the 20th?) we'll backport this to beta (it'll have merged to aurora already) and ESR.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8828643 [details] [diff] [review]
Use our existing function for removing a rule at a given index from a group rule

sec-approval to merge this fix into bug 851892 and land on m-c now, and to land this stand-alone on branches later.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that applies cleanly to 52, 51, and ESR 45

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8828896 [details] [diff] [review]
Patch that applies cleanly to 52, 51, and ESR 45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 53
Risk to taking this patch (and alternatives if risky): Very low risk.  Just
   nulls out some backpointers.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 435442
[User impact if declined]: Possible to trigger dangling pointer dereferences;
   likely exploitable.
[Is this code covered by automated tests?]: In general yes.  The test in this
   bug has NOT been checked in yet, to avoid disclosing the bug.
[Has the fix been verified in Nightly?]: I've verified the fix makes the
   attached test pass locally.  No nightly verification yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's just switching from:

  mRules.RemoveObjectAt(index);

to a function which does:

  Rule* rule = mRules.SafeObjectAt(aIndex);
  if (rule) {
    rule->SetStyleSheet(nullptr);
    rule->SetParentRule(nullptr);
  }
  return mRules.RemoveObjectAt(aIndex) ? NS_OK : NS_ERROR_ILLEGAL_VALUE;

so all it does is null out the backpointers from the Rule object (which is the security fix), and otherwise do exactly what we do now.

[String changes made/needed]: None.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed in 53, as part of bug 851892.

Comment 10

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

Can we check this in on February 7?

Comment 11

[Boris Zbarsky [:bzbarsky]]

You mean on branches?  I don't see a reason why not; the hard part is remembering to....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Note that we don't need to land this on Aurora at this point, since it's fixed there (Aurora is 53 now).

Comment 13

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

I have no idea what I was thinking now since it is fixed in another bug. That said, we should check it into beta this cycle. If it went in with other work, no one would ever know.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I'm not sure there's obvious style system work getting backported to beta to hide in...  If that's not what the needinfo is for, then I'm not sure what.

Comment 15

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

That or just nominating it to go into beta on 2/7 to reduce the "Hey, I'm a hidden security bug going into beta!" exposure. Either way, this fix should go into Beta somehow.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Nominating it to go into beta on 2/7 sounds good.... What are the concrete steps for that?

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16)
> Nominating it to go into beta on 2/7 sounds good.... What are the concrete
> steps for that?

Ideally, the sheriffs will notice the [checkin on 2/7] whiteboard and land it on beta then. It looks like you have all of the approvals set.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f67c25a9d2a9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/1a39a54b5fea

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1332550 tests. r=heycam

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2c16ec0cd70
tests.

Comment 22

[Andrei Ciure[:aciure]]

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