Seth Fowler [:seth] [:s2h] Assignee
	Description • 8 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 1 • 8 years ago

Here's the patch. The call to malloc is replaced with a call to calloc; that's
really it.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 2 • 8 years ago

Here are some tests that would've caught this. They also verify a little bit of
additional initial state.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 3 • 8 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 4 • 8 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 5 • 8 years ago

Note that the tests are potentially a little harder to backport, but I think we can live without backporting those.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 6 • 8 years ago

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!

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 7 • 8 years ago

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).

	Daniel Veditz [:dveditz]
	Comment 8 • 8 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 9 • 8 years ago

[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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 10 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 11 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/817695430906
https://hg.mozilla.org/mozilla-central/rev/9cef1a1fa8a2

	Carsten Book [:Tomcat]
	Comment 12 • 8 years ago

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')

	Daniel Veditz [:dveditz]
	Comment 13 • 8 years ago

[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.