Closed Bug 1276413 Opened 4 years ago Closed 4 years ago

Clear the buffer we allocate for paletted image frames

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- fixed
firefox-esr45 --- wontfix

People

(Reporter: seth, Assigned: seth)

References

Details

(Keywords: csectype-disclosure, csectype-uninitialized, sec-moderate, Whiteboard: [adv-main49+])

Attachments

(2 files)

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.

Approval Request Comment
[Feature/regressing bug #]: None, really. This problem has existed since ancient times, on all branches.
[User impact if declined]: There's potential for an attacker to be able to read raw memory from JavaScript if they can find an exploit in the GIF or PNG decoders. (And historically such exploits have appeared with some frequency.) That could leak a user's private data; bad stuff.
[Describe test coverage new/current, TreeHerder]: A new test is provided in this bug.
[Risks and why]: This patch could not be any lower risk. It just replaces malloc with calloc when we allocate the buffer for paletted image frames. This is so low risk that I think we can safely uplift to beta if release management is comfortable with that.
[String/UUID change made/needed]: None.
Attachment #8757600 - Flags: approval-mozilla-beta?
Attachment #8757600 - Flags: approval-mozilla-aurora?
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.
Attachment #8757600 - Flags: sec-approval?
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+
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!
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
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-
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Group: core-security → gfx-core-security
Attachment #8757600 - Flags: sec-approval?
Attachment #8757600 - Flags: approval-mozilla-aurora?
Attachment #8757600 - Flags: approval-mozilla-aurora+
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
https://hg.mozilla.org/mozilla-central/rev/817695430906
https://hg.mozilla.org/mozilla-central/rev/9cef1a1fa8a2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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')
Flags: needinfo?(seth)
Group: gfx-core-security → core-security-release
[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.
Flags: needinfo?(seth.bugzilla)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.