827591 - Abort when CSSOM modifies an @page rule, due to reassigning an nsAutoPtr

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: leak testcase

Comment 1

[Jesse Ruderman]

Attachment: fatal-assertion testcase (may crash)

###!!! ABORT: This makes no sense!: 'mRawPtr != newPtr || !newPtr', file nsAutoPtr.h, line 37

(This assertion in nsAutoPtr::assign checks that we don't re-assign the same pointer value using the '=' operator.  It was added in bug 723446.)

Comment 2

[Jesse Ruderman]

Attachment: crash testcase

Comment 3

[Jesse Ruderman]

Attachment: stack for the abort

Comment 4

[Jesse Ruderman]

Filed bug 827596 for fixing nsAutoPtr.  But that won't fix the leaks...

Comment 5

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

Presumably nsCSSKeyframeRule::ChangeDeclaration should look more like nsCSSPageRule::ChangeDeclaration (i.e., have a test around the mDeclaration = aDeclaration).  The code is still pretty sketchy, though.

Comment 6

[Andrew McCreight [:mccr8]]

Marking sec-critical because I think this kind of thing is a UAF.

Comment 7

[Brendan Dahl [:bdahl]]

Attachment: crash fix - v1

This uses the same fix as bug 723446. I assume we should nominate this for aurora and beta too?

I have yet to find the cause of the leak though.

Comment 8

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

Comment on attachment 699941 [details] [diff] [review]
crash fix - v1

r=dbaron, and yes, this should go on aurora and beta

Comment 9

[Brendan Dahl [:bdahl]]

Comment on attachment 699941 [details] [diff] [review]
crash fix - v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 115199
User impact if declined: crashes when setting @page rule properties
Testing completed (on m-c, etc.): try run
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none

As mentioned above this is the same fix as bug 723446 which landed awhile ago.

Comment 10

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

This will need to go through sec-approval on the patch as a sec-critical affecting more than trunk.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 11

[Brendan Dahl [:bdahl]]

Comment on attachment 699941 [details] [diff] [review]
crash fix - v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's trivial to crash the browser, I'm not sure what level of exploit we consider this.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, the test illuminates the problem.

Which older supported branches are affected by this flaw?
ff19+

If not all supported branches, which bug introduced the flaw?
bug 115199

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The code hasn't changed since FF19 so it should be easy to apply the patch.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.

Comment 12

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

Since this affects 19+ only, we should get this in now before we go through more betas. If we fix it now, we don't run the risk of exposing the public to it.

sec-approval+

Comment 13

[Jesse Ruderman]

I filed a separate bug for the leak: bug 829817.

Comment 14

[Brendan Dahl [:bdahl]]

What do I need to do to get this into MC and the other branches?  I imagine setting the usual checkin-needed keyword won't work since this is a security bug?

Comment 15

[Andrew McCreight [:mccr8]]

I've seen RyanVM land security bugs, too.

Comment 16

[Andrew McCreight [:mccr8]]

So just go ahead and set the flag, I mean.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> I've seen RyanVM land security bugs, too.

Lies.

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4c9385482def
https://hg.mozilla.org/releases/mozilla-beta/rev/75e3211747e1

Comment 20

[Mats Palmgren (inactive)]

Can we make this bug public now?  It would allow us to unhide bug 829817 so that
more people can try to fix that bug.

Comment 21

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

We normally would wait until at least Firefox 20 shipped since this was just fixed in 19.