Closed Bug 827591 Opened 8 years ago Closed 8 years ago
Abort when CSSOM modifies an @page rule, due to reassigning an ns
217 bytes, text/html
236 bytes, text/html
13.59 KB, text/plain
1.83 KB, patch
|Details | Diff | Splinter Review|
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.)
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.
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.
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 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.
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. sec-approval+
I filed a separate bug for the leak: bug 829817.
Summary: Leak or abort when CSSOM modifies an @page rule → Abort when CSSOM modifies an @page rule, due to reassigning an nsAutoPtr
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.
(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
Target Milestone: --- → mozilla21
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.
We normally would wait until at least Firefox 20 shipped since this was just fixed in 19.
You need to log in before you can comment on or make changes to this bug.