Closed Bug 1276413 Opened 4 years ago Closed 4 years ago
Clear the buffer we allocate for paletted image frames
1.36 KB, patch
|Details | Diff | Splinter Review|
2.99 KB, patch
|Details | Diff | Splinter Review|
The new GTests I'm adding in bug 1276061 revealed that we aren't clearing the memory for paletted image frames when we allocate them. This is a security bug waiting to happen. Let's nip this in the bud ASAP.
Here's the patch. The call to malloc is replaced with a call to calloc; that's really it.
Attachment #8757600 - Flags: review?(edwin)
Here are some tests that would've caught this. They also verify a little bit of additional initial state.
Attachment #8757602 - Flags: review?(n.nethercote)
Comment on attachment 8757600 [details] [diff] [review] (Part 1) - Clear paletted image frames when they're allocated. [Security approval request comment] How easily could an exploit be constructed based on the patch? The problem is certainly obvious from the patch, but to exploit it you'd also need to find an exploit in the GIF or PNG decoders (to cause them to fail to write to all the pixels in the image), and the patch doesn't provide any guidance as to how to do that. Indeed, no such exploit is known to exist, but we certainly don't know that one doesn't exist. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, pretty much. This patch is so trivial that it's hard to obfuscate. Which older supported branches are affected by this flaw? All of them. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This can be backported to any branch we care to backport it to; the patch is trivial. How likely is this patch to cause regressions; how much testing does it need? This patch could not be any safer. It just replaces a call to malloc with a call to calloc. The odds of regression are insanely low.
Note that the tests are potentially a little harder to backport, but I think we can live without backporting those.
Attachment #8757602 - Flags: review?(n.nethercote) → review+
Attachment #8757600 - Flags: review?(edwin) → review+
Hi Al, Dan, I am going to gtb RC1 in the next 2 hours. Is this something that is critical enough to be sec-approved and landed in time for RC1? Please let me know. Thanks!
Comment on attachment 8757600 [details] [diff] [review] (Part 1) - Clear paletted image frames when they're allocated. I spoke to abillings about this one and since this is not easily exploitable, we don't feel the urgency to uplift this in 47 during RC week (taking only release blockers from this point on).
Attachment #8757600 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8757600 [details] [diff] [review] (Part 1) - Clear paletted image frames when they're allocated. Since we don't know of a second bug required complete an exploit here let's go ahead and land it: a=dveditz. OK for aurora as well. Way too late for beta as Ritu marked.
[Tracking Requested - why for this release]: Although this is nominally sec-moderate for now, an image decoder bug could turn this into a sec-high. Let's take this on the ESR branch after this release (Don't land now, they may not have branched) just to be on the safe side.
https://hg.mozilla.org/integration/mozilla-inbound/rev/817695430906243ef1f46f3c8df49f2b86f489ef Bug 1276413 (Part 1) - Clear paletted image frames when they're allocated. r=edwin https://hg.mozilla.org/integration/mozilla-inbound/rev/9cef1a1fa8a2d158bcd53c946462dbb62f871ee9 Bug 1276413 (Part 2) - Add tests for expected state when initializing a SurfaceSink. r=njn
has problems uplifting to aurora, seth could you take a look, thanks! merging image/imgFrame.cpp warning: conflicts while merging image/imgFrame.cpp! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')
[Tracking Requested - why for this release]: Clearly this won't make Firefox 48 at this point. Given the trouble in getting even an Aurora-48 patch (comment 12) it might be worse trying to port this to ESR-45. Given comment 4 maybe we can pass on it.
You need to log in before you can comment on or make changes to this bug.