Closed Bug 827591 Opened 11 years ago Closed 11 years ago

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


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox18 --- unaffected
firefox19 + fixed
firefox20 + fixed
firefox21 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: jruderman, Assigned: bdahl)



(6 keywords, Whiteboard: [adv-main19-])


(4 files, 1 obsolete file)

Attached file leak testcase (obsolete) —
      No description provided.
###!!! 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.)
Attached file crash testcase
Attached file stack for the abort
Filed bug 827596 for fixing nsAutoPtr.  But that won't fix the leaks...
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.
Marking sec-critical because I think this kind of thing is a UAF.
Attached patch crash fix - v1Splinter Review
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.
Attachment #699941 - Flags: review?(dbaron)
Comment on attachment 699941 [details] [diff] [review]
crash fix - v1

r=dbaron, and yes, this should go on aurora and beta
Attachment #699941 - Flags: review?(dbaron) → review+
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.
Attachment #699941 - Flags: approval-mozilla-beta?
Attachment #699941 - Flags: approval-mozilla-aurora?
This will need to go through sec-approval on the patch as a sec-critical affecting more than trunk.
Blocks: 115199
Keywords: regression
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?

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.
Attachment #699941 - Flags: sec-approval?
Attachment #699941 - Flags: sec-approval? → sec-approval+
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.

Attachment #699941 - Flags: approval-mozilla-beta?
Attachment #699941 - Flags: approval-mozilla-beta+
Attachment #699941 - Flags: approval-mozilla-aurora?
Attachment #699941 - Flags: approval-mozilla-aurora+
I filed a separate bug for the leak: bug 829817.
Keywords: mlk
Summary: Leak or abort when CSSOM modifies an @page rule → Abort when CSSOM modifies an @page rule, due to reassigning an nsAutoPtr
Attachment #698941 - Attachment is obsolete: true
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?
I've seen RyanVM land security bugs, too.
So just go ahead and set the flag, I mean.
Keywords: checkin-needed
(In reply to Andrew McCreight [:mccr8] from comment #15)
> I've seen RyanVM land security bugs, too.

Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla21
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [adv-main19-]
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.
Flags: needinfo?(dveditz)
We normally would wait until at least Firefox 20 shipped since this was just fixed in 19.
Group: core-security
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.