Closed Bug 963974 Opened 6 years ago Closed 6 years ago
Faulty crash: use-after-free of m
Current Composite Task in Compositor Parent::Recv Flush Rendering()
8.25 KB, text/plain
19.94 KB, text/plain
664 bytes, patch
|Details | Diff | Splinter Review|
Found by Christoph Diehl's "Faulty" fuzzer, see bug 777067
I forgot to get a stack trace in that session @#$!
Classification: either PCompositor or non-protocol, IPC CancelableTask lifetime management bug (apparently IPC system destroying our CancelableTask under our feet), hard
This is caused by forgetting to null the raw mCurrentCompositeTask after calling Cancel() on it.
Attachment #8373653 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8373653 [details] [diff] [review] Null mCurrentCompositeTask after calling Cancel() on it [Security approval request comment] How easily could an exploit be constructed based on the patch? An attacker might guess that if a secure patch is 1-line just clearing a pointer, then it's probable a use-after-free fix. Based on that, they could infer how to cause a use-after-free in the parent process. Then they would have to look at the specifics of CancelableTask to find an exploit (which is likely to be feasible, as this class is virtual). The patch doesn't give any hint about how to do that. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? All branches (regressed in gecko24). Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? 1-line patch trivial to backport as needed. How likely is this patch to cause regressions; how much testing does it need? No risk at all.
Attachment #8373653 - Flags: sec-approval?
Comment on attachment 8373653 [details] [diff] [review] Null mCurrentCompositeTask after calling Cancel() on it sec-approval+ for trunk. I'd like to get Aurora, Beta, and ESR24 patches and nominations as well.
Attachment #8373653 - Flags: sec-approval? → sec-approval+
Does sec-approval+ mean green light to land on central?
Comment on attachment 8373653 [details] [diff] [review] Null mCurrentCompositeTask after calling Cancel() on it Not bothering to write "backported" versions of this 1-line patch, which is likely to apply just fine on various trees. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 873944 User impact if declined: use-after-free in compositor (parent) process Testing completed (on m-c, etc.): no landing yet, since I'm waiting for an explicit green light to land on central. But locally this works fine. Risk to taking this patch (and alternatives if risky): very low risk. String or IDL/UUID changes made by this patch: none
(I've read the documentation now, so, yes, sec-approval+ did mean a green light to land :-) )
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Attachment #8373653 - Flags: approval-mozilla-esr24?
Attachment #8373653 - Flags: approval-mozilla-esr24+
Attachment #8373653 - Flags: approval-mozilla-beta?
Attachment #8373653 - Flags: approval-mozilla-beta+
Attachment #8373653 - Flags: approval-mozilla-aurora?
Attachment #8373653 - Flags: approval-mozilla-aurora+
Matt, can you please evaluate if this needs QA testing before we release Firefox 28?
QA Contact: mwobensmith
Whiteboard: [qa-] → [qa-][adv-main28+][adv-esr24.4+]
You need to log in before you can comment on or make changes to this bug.