Closed Bug 251381 Opened 20 years ago Closed 20 years ago

new libpng buffer overflow vulnerabilities

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

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

References

()

Details

(4 keywords, Whiteboard: [sg:fix]blocking1.4.3+)

Attachments

(2 files, 7 obsolete files)

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
Should we apply the patch from your report to our in-tree copy of libpng?
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.
Whiteboard: blocking1.4.3+
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.
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.
Status: NEW → ASSIGNED
assigning to Glenn per comment 4
Flags: blocking1.8a2?
Flags: blocking1.7.2?
Flags: blocking-aviary1.0RC1?
Whiteboard: blocking1.4.3+ → [sg:fix]blocking1.4.3+
what should the release plans be for this? should get patched on trunk, 1.7
branch and aviary 1.0 branch.
Flags: blocking1.7.2?
Flags: blocking1.7.2+
Flags: blocking-aviary1.0RC1?
Flags: blocking-aviary1.0RC1+
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.
Assignee: jdunn → glennrp
Status: ASSIGNED → NEW
Attached patch a more robust fix (obsolete) — Splinter Review
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.
Taking bug again.  Why does this keep getting reset to NEW?  Maybe
because it is a security bug?
Status: NEW → ASSIGNED
Re: comment #8 -- I relocated the libpng patches to
http://libpng.sf.net/fix/
and http://libpng.sf.net/fix/patches/
Comment on attachment 154405 [details] [diff] [review]
a more robust fix

tor: r? (one or both patches)
Attachment #154405 - Flags: review?(tor)
Comment on attachment 154405 [details] [diff] [review]
a more robust fix

Neat fix.  Minor nit: spaces after the commas would be nice,
Attachment #154405 - Flags: superreview+
Attachment #154405 - Flags: review?(tor)
Attachment #154405 - Flags: review+
Spaces, you got 'em.
Attachment #154063 - Attachment is obsolete: true
Attachment #154405 - Attachment is obsolete: true
Glenn: Are you ok with checking in this fix now, or should we keep it
hidden until public disclosure?
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.
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
Saw an announcement that NS 7.2 will be out August 3rd.  Will it contain this fix?
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.
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....
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.
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?
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?
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
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
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.
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.
Attachment #154509 - Attachment is obsolete: true
Attachment #154998 - Flags: review?(glennrp)
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 on attachment 154998 [details] [diff] [review]
limit image dimensions, includes tRNS fix

r. glennrp
Attachment #154998 - Flags: review?(glennrp) → review+
Adding our release QA team.
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.
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.
...no crash either, using 2004080211-0.9.3 firefox bits on mac os x 10.3.4.
Re comment #33, my talkback ID was TB461197W.
Attached patch patch to skip unused PNG chunks. (obsolete) — Splinter Review
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 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...
Attachment #155039 - Attachment is obsolete: true
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...
Removed sRGB from the unused-chunks list.
Comment on attachment 155042 [details] [diff] [review]
patch to skip unused PNG chunks (but not sRGB!).

tor: r?
Attachment #155042 - Flags: review?(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.
Attachment #155042 - Flags: review?(tor) → review+
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.
Attachment #154998 - Flags: approval1.7.3?
Attachment #154998 - Flags: approval1.4.3?
Attachment #154998 - Flags: approval-aviary?
Attachment #155042 - Flags: approval1.7.3?
Attachment #155042 - Flags: approval1.4.3?
Attachment #155042 - Flags: approval-aviary?
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().
Attachment #155042 - Attachment is obsolete: true
Comment on attachment 155093 [details] [diff] [review]
patch to skip unused PNG chunks (2)

Tor: r?
Attachment #155093 - Flags: review?(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
Attachment #155093 - Flags: review?(tor)
Attachment #155093 - Flags: review+
Attachment #155093 - Flags: approval1.7.3?
Attachment #155093 - Flags: approval1.4.3?
Attachment #155093 - Flags: approval-aviary?
Attachment #155042 - Flags: approval1.7.3?
Attachment #155042 - Flags: approval1.4.3?
Attachment #155042 - Flags: approval-aviary?
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.
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 on attachment 155093 [details] [diff] [review]
patch to skip unused PNG chunks (2)

a=blizzard for 1.4.3
Attachment #155093 - Flags: approval1.4.3? → approval1.4.3+
Comment on attachment 154998 [details] [diff] [review]
limit image dimensions, includes tRNS fix

a=blizzard for 1.4.3
Attachment #154998 - Flags: approval1.4.3? → approval1.4.3+
Combine the two patches and remove the "const" in the unused_chunks 
declaration that was causing compile problems.
Attachment #154998 - Attachment is obsolete: true
Attachment #155093 - Attachment is obsolete: true
Attachment #154998 - Flags: approval1.7.3?
Attachment #154998 - Flags: approval-aviary?
Attachment #155093 - Flags: approval1.7.3?
Attachment #155093 - Flags: approval-aviary?
Attachment #155114 - Flags: approval1.7.3?
Attachment #155114 - Flags: approval-aviary?
Comment on attachment 155114 [details] [diff] [review]
combined set of fixes

a=mkaply for both
Attachment #155114 - Flags: approval1.7.3?
Attachment #155114 - Flags: approval1.7.3+
Attachment #155114 - Flags: approval-aviary?
Attachment #155114 - Flags: approval-aviary+
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.
Group: security
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
(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)!

this should have been relnoted for Firefox.  see bug 254655.
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."
Flags: blocking1.8a2?
*** Bug 257492 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.