251381 - new libpng buffer overflow vulnerabilities

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Glenn Randers-Pehrson]

I have been notified that there are some newly discovered buffer overflow
vulnerabilities in libpng.

http://scary.beasts.org/security/CESA-2004-001.txt
contains the bug report and patch.

The PLTE overflow can be used against mozilla.  The hIST, iCCP, sBIT, and
sPLT vulnerabilities would not affect mozilla because they are disabled
via the #defines in mozlibpngconf.h.

libpng-1.2.6 will be released shortly with patches but libpng-1.2.5 and
earlier, if used from the system, would be vulnerable.  Therefore we should
change the MOZPNG configuration variable to require libpng-1.2.6 as a minimum.

Glenn

Comment 1

[Mike Shaver (:shaver emeritus)]

Should we apply the patch from your report to our in-tree copy of libpng?

Comment 2

[Glenn Randers-Pehrson]

Mike: Yes, I think so.   Definitely, the in-tree libpng has to be patched.
I have just tried on my FreeBSD platform and the patch was useable as is.
It contains an arbitrary reduction of the maximum image size (from 2^31-1
rows and columns to 2^24-1 rows and columns) but realistically no one will
be trying to display such large images.

Comment 3

[Glenn Randers-Pehrson]

On FreeBSD5.0/gcc-3.2.1, using mozilla-1.8a to view the pngtest_bad.png
from the bug report, I get a segv core dump.

After applying the patch, I only get the text about
"cannot display the image because it contains errors" which is the
expected proper behavior.

I also tried to look at the image with Firefox 0.9.2 on Win XP Home
but it seemed to hang without showing anything.  After I killed
it and tried to run Firefox again, my profile was busy.

So I conclude that the reported vulnerability is real and the proposed
patch fixes it.

Comment 4

[Glenn Randers-Pehrson]

Taking bug.

The original report that I received said that the bug is to be held
private until 4 August.  I will be releasing libpng-1.2.6 accordingly.

Comment 5

[Daniel Veditz [:dveditz]]

assigning to Glenn per comment 4

Comment 6

[chris hofmann]

what should the release plans be for this? should get patched on trunk, 1.7
branch and aviary 1.0 branch.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

and 1.4 branch.

Comment 8

[Glenn Randers-Pehrson]

Attachment: patch for libpng tRNS chunk overflow

I've posted a set of patches for the reported security problems at
http://libpng.sf.net/scary/patches .  Of these, only the one for the
tRNS chunk overflow is relevant to mozilla.  Here is a copy of that
patch.

Comment 9

[Glenn Randers-Pehrson]

Attachment: a more robust fix

This is a more robust fix for the problem, since it works with system libpng
as well as the supplied one.  We wouldn't need the first patch any more, but
neither would it hurt to apply both.

Comment 10

[Glenn Randers-Pehrson]

Taking bug again.  Why does this keep getting reset to NEW?  Maybe
because it is a security bug?

Comment 11

[Glenn Randers-Pehrson]

Re: comment #8 -- I relocated the libpng patches to
http://libpng.sf.net/fix/
and http://libpng.sf.net/fix/patches/

Comment 12

[Glenn Randers-Pehrson]

Comment on attachment 154405 [details] [diff] [review]
a more robust fix

tor: r? (one or both patches)

Comment 13

[tor]

Comment on attachment 154405 [details] [diff] [review]
a more robust fix

Neat fix.  Minor nit: spaces after the commas would be nice,

Comment 14

[Glenn Randers-Pehrson]

Attachment: a more robust fix, with spaces after the commas

Spaces, you got 'em.

Comment 15

[tor]

Glenn: Are you ok with checking in this fix now, or should we keep it
hidden until public disclosure?

Comment 16

[chris hofmann]

google search for png buffer overflow shows quite a bit of public posting on
this bug...
http://www.google.com/search?hl=en&lr=&ie=UTF-8&q=png+buffer+overflow&btnG=Search

seems like it is public but just hasn't drawn a lot of attention yet.

Comment 17

[Glenn Randers-Pehrson]

Chris: They have actually drawn more attention than they really deserve.  Those
are about two different related, fairly harmless bugs (despite the reports
saying they can be exploited).  They don't affect mozilla at all because we
don't operate with 16-bit samples.

Tor: I think wait until August 4th.  CERT is asking for a 6-week extension
beyond that, for the sake of some big vendor, but we are resisting that.

Dunno if the big vendor is Netscape, but the bug blows NS-4.8 away.  I think
it uses an ancient libpng-0.95 or 0.96.

Re comment #8 and comment #11, the patches are now at
http://www.graphicsmagick.org/libpng/beta

Comment 18

[Glenn Randers-Pehrson]

Saw an announcement that NS 7.2 will be out August 3rd.  Will it contain this fix?

Comment 19

[chris hofmann]

we informed netscape a few days ago. haven't heard back from them.  

we need to get the fix to aviary 1.0 and 1.7 branches as soon as possible, so we
can start spinning up mozilla builds for testing and release next week.

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

This was "disclosed" yesterday on png-implement.  The announcement made
reference to security fixes in the 1.2.6-beta release.  Explicit details were
left out.  We should be able to do the same, I would think....

Comment 21

[Glenn Randers-Pehrson]

Libpng-1.2.6beta4 does not include patches for the most gruesome vulnerabilies,
which includes the one that is addressed by the patches for this bug.

Comment 22

[Glenn Randers-Pehrson]

The PNG development group has been working on a response to the bug report.  We
have concluded that the best way to take care of Item 7 (other flaws) is to
limit the dimensions of PNG files that are accepted.  The PNG spec allows
images up to 2^31-1 by 2^31-1 (i.e., 2.147 billion by 2.147 billion).  We
are implementing a soft limit of 1 million by 1 million that can be overridden
at compile time with a #define or at run time by calling an accessor function.

Tor: should I add that to the patch for this bug, put up a separate patch
here, or start a new bug with a patch?

Comment 23

[tor]

mozilla already has a check to make sure that 4 * width * height doesn't
overflow a 32-bit number.  Is that sufficient to avoid the dimension problem?

Comment 24

[Glenn Randers-Pehrson]

That's not sufficient.  See
http://www.graphicsmagick.org/libpng/beta/samples/bigw.png
which crashes my Firefox but is only 65 million columns by 1 row.
It needs a safety of 64 because it has 64-bit RGBA pixels and there is
arithmetic in libpng that uses the number of bits in a row.

The fix is pretty simple; see *patch11* at
www.graphicsmagick.org/libpng/beta/patches

A slightly more complex fix is in libpng-1.2.6beta4j at
www.graphicsmagick.org/libpng/beta/src
This allows user override of the new 1,000,000 row,column limits.
We don't need that though.

The patch11 has an advantage of quickly rejecting very large valid image
files that mozilla could process without crashing but it takes hours
to do it.

Glenn

Comment 25

[tor]

patch11 looks fine for mozilla, though I'd guess we only really need the
width limit since we're doing progressive reading.  r+sr=tor

Comment 26

[tor]

On second thought, since we may be linked to system libpngs that haven't been
updated, maybe we should to this size limitation in info_callback() after
the call to png_get_IHDR.

Comment 27

[Glenn Randers-Pehrson]

The height limit is also useful, to stop DOS attempts using an image
with a half-billion rows by one column.

About system libraries, imposing the limit in the callback would be OK.

Comment 28

[tor]

Attachment: limit image dimensions, includes tRNS fix

Comment 29

[Glenn Randers-Pehrson]

Here are some CVE names

> 1) Remotely exploitable stack-based buffer overrun in png_handle_tRNS
> (pngrutil.c)
> 2) Dangerous code in png_handle_sBIT (pngrutil.c) (Similar code in
> png_handle_hIST).

CAN-2004-0597 for these (we merge issues that have the same flaw type that 
get fixed in the same versions).

> 3) Possible NULL-pointer crash in png_handle_iCCP (pngrutil.c) (this 
> flaw is duplicated in multiple other locations).

CAN-2004-0598 for those

> 4) Theoretical integer overflow in allocation in png_handle_sPLT 
> (pngrutil.c)
> 5) Integer overflow in png_read_png (pngread.c)
> 6) Integer overflows during progressive reading.
> 7) Other flaws.  [integer overflows]

CAN-2004-0599 for those

The patch looks OK.  It fixes Items 1 and 6 (and maybe 7).  Mozilla is
not vulnerable to Item 5 nor to 2, 3, and 4 when using the embedded libpng.
It might be a good idea to require libpng-1.2.6 once it's out (planned for
August 11), although it's not clear that 2,3,and 4 can really be exploited.
I'll mark the "r" flag this evening after I've built and tested against
some dangerous files.

Comment 30

[Glenn Randers-Pehrson]

Comment on attachment 154998 [details] [diff] [review]
limit image dimensions, includes tRNS fix

r. glennrp

Comment 31

[Asa Dotzler [:asa]]

Adding our release QA team.

Comment 32

[sairuh (rarely reading bugmail)]

is loading http://www.graphicsmagick.org/libpng/beta/samples/bigw.png a
sufficient test for this bug?

fwiw, I loaded that page in seamonkey (linux 2004080114-1.7.2 branch), and no
crash occurred.

Comment 33

[Glenn Randers-Pehrson]

I don't know.  bigw.png crashes my Firefox 0.9.2, leaving my profile "in use"
until I reboot the computer.  I created a talkback item yesterday at 4:02 PM
EST but then lost track of the item number.  If someone can find it, or tell
me where to look on my computer for the ID, it would be helpful to see the
traceback.  I've crashed about 3 or 4 times with that file but only got
into the talkback mode once.

Comment 34

[sairuh (rarely reading bugmail)]

...no crash either, using 2004080211-0.9.3 firefox bits on mac os x 10.3.4.

Comment 35

[Glenn Randers-Pehrson]

Re comment #33, my talkback ID was TB461197W.

Comment 36

[Glenn Randers-Pehrson]

Attachment: patch to skip unused PNG chunks.

The original comments at the scary beasts URL talk about problems with
some ancillary chunks such as sBIT, sPLT, and iCCP.  While we are fixing
those in libpng-1.2.6, it leaves Mozilla applications open to these
problems when loaded with an unpatched older system library.

This patch causes all processing of these unused chunks to be skipped when
the system library is used.

We are running out of time, since cevans plans to go ahead with his
announcement on 4 August at 1400UTC.  I see the other patch is already
checked in.

Comment 37

[Glenn Randers-Pehrson]

Comment on attachment 155039 [details] [diff] [review]
patch to skip unused PNG chunks.

Oops, sRGB shouldn't be on the unused_chunks list.  Let me redo the patch...

Comment 38

[sairuh (rarely reading bugmail)]

the talkback server claims that there's no TB461197W incident. :( nor could I
find reports based on Glenn's email or the test URI from comment 24. unless it's
waiting in a queue somehow...

Comment 39

[Glenn Randers-Pehrson]

Attachment: patch to skip unused PNG chunks (but not sRGB!).

Removed sRGB from the unused-chunks list.

Comment 40

[Glenn Randers-Pehrson]

Comment on attachment 155042 [details] [diff] [review]
patch to skip unused PNG chunks (but not sRGB!).

tor: r?

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

http://talkback-public.mozilla.org/talkback/fastfind.jsp claims that the current
talkback queue is rather long...

Comment 42

[tor]

Comment on attachment 155042 [details] [diff] [review]
patch to skip unused PNG chunks (but not sRGB!).

'b', 'K', 'G', 'D' etc. might have been a bit easier to read,
but the table checks out.

Comment 43

[Glenn Randers-Pehrson]

The PNG spec cautions us to use integers, not characters, to build the chunk
types.  Otherwise you're hosed if you happen to use a compiler with a localized
character set.  I never thought there was a lot of danger of that, but there you
have it.  The table works out OK because I just cut-and-pasted it out of png.h.

Comment 44

[Glenn Randers-Pehrson]

Attachment: patch to skip unused PNG chunks (2)

There were a couple of bugs in my previous patch.  Missing ";" at the
end of the unused chunks list, and an extra parameter "mInfo" in the
call to png_set_keep_unknown_chunks().

Comment 45

[Glenn Randers-Pehrson]

Comment on attachment 155093 [details] [diff] [review]
patch to skip unused PNG chunks (2)

Tor: r?

Comment 46

[tor]

Comment on attachment 155093 [details] [diff] [review]
patch to skip unused PNG chunks (2)

You'll fix libpng.txt in the next libpng release to remove the
documentation of the extra parameter (still mentioned in rc5)?
r=tor

Comment 47

[Glenn Randers-Pehrson]

libpng documentation fixed in version rc6.  Thanks, I couldn't figure why
I had included the extra param.

We want to think about installing libpng-1.2.6.  The final "rc" version will
be posted tomorrow and released as libpng-1.2.6 on August 11.  We might want
to let it simmer for another week or so before installing it here.  It
fixes a few more security issues that our patches don't fix, but they don't
look terribly serious.

Comment 48

[tor]

Once libpng 1.2.6 has been out a bit, I was thinking of updating the mozilla
tree version and the minimum required version.  Most of these fixes can come
out at the same time, save possibly the dimension limit.

Branches will stay at 1.2.5 + these fixes.

Comment 49

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 155093 [details] [diff] [review]
patch to skip unused PNG chunks (2)

a=blizzard for 1.4.3

Comment 50

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 154998 [details] [diff] [review]
limit image dimensions, includes tRNS fix

a=blizzard for 1.4.3

Comment 51

[tor]

Attachment: combined set of fixes

Combine the two patches and remove the "const" in the unused_chunks 
declaration that was causing compile problems.

Comment 52

[Mike Kaply [:mkaply]]

Comment on attachment 155114 [details] [diff] [review]
combined set of fixes

a=mkaply for both

Comment 53

[tor]

Checked into trunk, MOZILLA_1_7_2_BRANCH, MOZILLA_1_4_BRANCH,
MOZILLA_1_7_BRANCH, AVIARY_1_0_20040515_BRANCH.

Past scheduled public disclosure, removing security flag.

Comment 54

[Glenn Randers-Pehrson]

Firefox 0.9.3 seems to have no problem with my sample bad images.
GECKO 20040803/Firefox 0.9.3
The installer thinks it's installing 0.9.2 though.

verified fixed.

Comment 55

[sairuh (rarely reading bugmail)]

(In reply to comment #54)
> Firefox 0.9.3 seems to have no problem with my sample bad images.
> GECKO 20040803/Firefox 0.9.3
> The installer thinks it's installing 0.9.2 though.

side note: the installer issue is bug 254097.

Glenn, thanks for verifying this (as well as the testing and patches)!

Comment 56

[Marc Bejarano]

this should have been relnoted for Firefox.  see bug 254655.

Comment 57

[David Epstein]

Attachment: bad png image for test case

This image shouldn't open. When trying to open, it should give msg "the image
pngtest_bad.png cannot be displayed, because it contains errors."

Comment 58

[Matthias Versen [:Matti]]

*** Bug 257492 has been marked as a duplicate of this bug. ***