833594 - Disable PNG_READ_TEXT in mozpngconf.h

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Glenn Randers-Pehrson]

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.

Comment 1

[Glenn Randers-Pehrson]

Attachment: v00 disable TEXT in mozpngconf.h

Taking bug.

Comment 2

[Glenn Randers-Pehrson]

Attachment: ten.png.gz with ten million text chunks

Handle this with care after gunzipping.  Besides firefox it may hang your file viewer.  The image is just a 1x1 black pixel.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9ef1d95d94

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Presumably, this is going to need uplifting to all the other supported branches too. Glenn, can you request approval?

Comment 5

[Glenn Randers-Pehrson]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf9ef1d95d94

Glenn, is the attached PNG something suitable for checkin as an automated test?

Comment 7

[Glenn Randers-Pehrson]

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

Comment 8

[Glenn Randers-Pehrson]

Attachment: hundred.png.xz with one hundred million text chunks

Comment 9

[Alex Keybl [:akeybl]]

Setting as sec-moderate given comment 5. Also only approving for Aurora, given this security rating.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4be66100afd1

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(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?

Comment 12

[Glenn Randers-Pehrson]

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.

Comment 13

[Joe Drew (not getting mail)]

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