Closed Bug 833594 Opened 7 years ago Closed 7 years ago

Disable PNG_READ_TEXT in mozpngconf.h

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- fixed
firefox21 --- fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: glennrp+bmo, Assigned: glennrp+bmo)

Details

(Keywords: sec-moderate)

Attachments

(3 files)

The PNG_READ_TEXT macro was inadvertently defined while updating libpng to version 1.5.9, bug #648690.  This is a performance and security issue; without this definition the decoder will simply skip any tEXt, zTXt, and iTXt chunks,
but with it, those chunks are processed and it is possible to cause firefox to hang by including a large number of text chunks in a PNG file.  This only affects the embedded libpng because a call in nsPNGDecoder.cpp to png_set_keep_unknown_chunks() tells the "system" libpng to skip them.

Marked "security" because the bug provides an easy way to DOS the browser.
Taking bug.
Assignee: nobody → glennrp+bmo
Status: NEW → ASSIGNED
Attachment #705134 - Flags: review?
Attachment #705134 - Flags: review? → review?(ryanvm)
Attachment #705134 - Flags: review?(ryanvm) → review?(joe)
Attachment #705134 - Flags: review?(joe) → review+
Handle this with care after gunzipping.  Besides firefox it may hang your file viewer.  The image is just a 1x1 black pixel.
Attachment #705140 - Attachment mime type: image/png → application/gzip
Presumably, this is going to need uplifting to all the other supported branches too. Glenn, can you request approval?
Comment on attachment 705134 [details] [diff] [review]
v00 disable TEXT in mozpngconf.h

[Approval Request Comment]
Regression caused by (bug #): 648690
User impact if declined: Possible easy DOS via a PNG with a large number of text chunks
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): low
Attachment #705134 - Flags: approval-mozilla-release?
Attachment #705134 - Flags: approval-mozilla-esr17?
Attachment #705134 - Flags: approval-mozilla-beta?
Attachment #705134 - Flags: approval-mozilla-b2g18?
Attachment #705134 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/cf9ef1d95d94

Glenn, is the attached PNG something suitable for checkin as an automated test?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(In reply to Ryan VanderMeulen from comment #6)
> https://hg.mozilla.org/mozilla-central/rev/cf9ef1d95d94
> 
> Glenn, is the attached PNG something suitable for checkin as an automated
> test?

It's big enough to demonstrate sluggish behavior but not big enough to
reach the overflow condition.  I can provide a 1.4Gbyte PNG that has
100 million text chunks and take a few decades to read.  That one is
even a little sluggish with the patch because we still have to skip
over the chunks.  There's nothing that can be done about that.

I'll go ahead and upload the larger PNG, xz-compressed (gzip-compressed
would be about 2.5Mb).
Attachment #705140 - Attachment description: PNG with ten million text chunks → ten.png.gz with ten million text chunks
Attachment #705391 - Attachment description: PNG with one hundred million text chunks → hundred.png.xz with one hundred million text chunks
Attachment #705140 - Attachment mime type: application/gzip → application/octet-stream
Setting as sec-moderate given comment 5. Also only approving for Aurora, given this security rating.
Attachment #705134 - Flags: approval-mozilla-release?
Attachment #705134 - Flags: approval-mozilla-release-
Attachment #705134 - Flags: approval-mozilla-esr17?
Attachment #705134 - Flags: approval-mozilla-esr17-
Attachment #705134 - Flags: approval-mozilla-beta?
Attachment #705134 - Flags: approval-mozilla-beta-
Attachment #705134 - Flags: approval-mozilla-b2g18?
Attachment #705134 - Flags: approval-mozilla-b2g18-
Attachment #705134 - Flags: approval-mozilla-aurora?
Attachment #705134 - Flags: approval-mozilla-aurora+
(In reply to Glenn Randers-Pehrson from comment #0)
> Marked "security" because the bug provides an easy way to DOS the browser.

If DoS really is the only concern, there's no need to hide this. Can you confirm that that's the case?
That is correct.  It makes us vulnerable to a possible overwrite of un-malloced memory, but it would take decades for a 32-bit computer to reach that state.

It is a real possibility, however, on computers where the int is 16 bits.  Do we support any such platform now?

It's no secret that one can DOS a browser by sending it more information than it can handle.
(In reply to Glenn Randers-Pehrson from comment #12)
> It is a real possibility, however, on computers where the int is 16 bits. 
> Do we support any such platform now?

No, and not for a long while, thank goodness.

> It's no secret that one can DOS a browser by sending it more information
> than it can handle.

For serious.
Group: core-security
You need to log in before you can comment on or make changes to this bug.