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

RESOLVED FIXED in Firefox 19



6 years ago
6 years ago


(Reporter: jruderman, Assigned: bdahl)


(6 keywords)

assertion, crash, csectype-uaf, regression, sec-critical, testcase
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 unaffected, firefox19+ fixed, firefox20+ fixed, firefox21+ fixed, firefox-esr17 unaffected, b2g18 unaffected)


(Whiteboard: [adv-main19-])


(4 attachments, 1 obsolete attachment)



6 years ago
Created attachment 698941 [details]
leak testcase

Comment 1

6 years ago
Created attachment 698942 [details]
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

6 years ago
Created attachment 698943 [details]
crash testcase

Comment 3

6 years ago
Created attachment 698944 [details]
stack for the abort

Comment 4

6 years ago
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.
Keywords: csec-uaf, sec-critical

Comment 7

6 years ago
Created attachment 699941 [details] [diff] [review]
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.
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 9

6 years ago
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
Assignee: nobody → bdahl
status-b2g18: --- → unaffected
status-firefox18: --- → unaffected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?
tracking-firefox19: ? → +
tracking-firefox20: ? → +
tracking-firefox21: ? → +

Comment 11

6 years ago
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+

Comment 13

6 years ago
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


6 years ago
Attachment #698941 - Attachment is obsolete: true

Comment 14

6 years ago
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.


6 years ago
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
Last Resolved: 6 years ago
status-firefox21: affected → fixed
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.