Last Comment Bug 478901 - Upgrade libpng to 1.2.35 (libpng-1.2.34 and earlier might free undefined pointers)
: Upgrade libpng to 1.2.35 (libpng-1.2.34 and earlier might free undefined poin...
Status: VERIFIED FIXED
[sg:critical?]
: fixed1.8.1.21, fixed1.9.0.7, verified1.9.0.9, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: P2 critical (vote)
: mozilla1.9.2a1
Assigned To: Glenn Randers-Pehrson
:
:
Mentors:
: 479223 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-17 09:47 PST by Glenn Randers-Pehrson
Modified: 2009-09-02 10:51 PDT (History)
18 users (show)
vladimir: blocking1.9.2+
vladimir: blocking1.9.1+
dveditz: blocking1.9.0.7+
dveditz: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed


Attachments
Update trunk to libpng-1.2.35 (40.65 KB, patch)
2009-02-17 10:27 PST, Glenn Randers-Pehrson
joe: review+
dveditz: superreview+
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review
minimal 1.8 version (5.29 KB, patch)
2009-02-19 05:14 PST, Martin Stránský
no flags Details | Diff | Splinter Review
Complete 1.8.1 patch, part 1 (92.97 KB, patch)
2009-02-19 16:17 PST, Glenn Randers-Pehrson
joe: review+
dveditz: superreview+
samuel.sidler+old: approval1.8.1.next+
Details | Diff | Splinter Review
Complete 1.8.1 patch, part 2 (969.24 KB, patch)
2009-02-19 16:18 PST, Glenn Randers-Pehrson
joe: review+
dveditz: superreview+
samuel.sidler+old: approval1.8.1.next+
Details | Diff | Splinter Review
Restore mozpngconf.h to 1.8.1 (9.87 KB, patch)
2009-02-20 12:53 PST, Glenn Randers-Pehrson
joe: review+
dveditz: superreview+
Details | Diff | Splinter Review

Description Glenn Randers-Pehrson 2009-02-17 09:47:49 PST
Libpng-1.2.34 and earlier contain several instances of

   malloc an array of N elements
   for (i=0; i<N; i++)
      malloc element[i];

If the application runs out of memory before the Nth
element has been allocated, libpng will longjmp to a
cleanup process.  This process will attempt to free()
all of the elements, some of which might be undefined.

A malevolent web site could deliberately force this
behavior.

The fix is to store NULL in all of the elements before
performing the loop that malloc's them.  I will upload
a patch shortly to upgrade the trunk to a preview of
libpng-1.2.35 which addresses the problem.

There are at least 5 instances of this problem.  The first
is in png_read_png() which we do not use.  The second is
in decoding the pCAL chunk, which we also do not use.  The
other three are near the end of pngrtran.c, where 16-bit
gamma tables are built.  I believe we do encounter this
vulnerability, even though we use png_strip_16() to reduce
the input to 8 bits.  Because of the order of function
calls within pngrtran.c, this does not prevent the 16-bit
gamma tables from being built, when the input PNG has
16-bit depth.

The security flag can be cleared once libpng-1.2.35 is released.
Comment 1 Daniel Veditz [:dveditz] 2009-02-17 09:52:25 PST
Thanks Glenn
Comment 2 Glenn Randers-Pehrson 2009-02-17 10:27:04 PST
Created attachment 362738 [details] [diff] [review]
Update trunk to libpng-1.2.35
Comment 3 Glenn Randers-Pehrson 2009-02-17 10:33:58 PST
Libpng-1.2.35 is presently scheduled for release on February 19, 2009.
Comment 4 Daniel Veditz [:dveditz] 2009-02-17 15:08:47 PST
Although we're past code-freeze for 3.0.7 our release date will be well after that. We may want to re-spin with this fix, or at least keep this on the short-list to take in a re-spin.
Comment 5 Daniel Veditz [:dveditz] 2009-02-17 16:54:30 PST
Going to re-spin 3.0.7 for this :-(
Comment 6 Daniel Veditz [:dveditz] 2009-02-17 17:06:07 PST
Comment on attachment 362738 [details] [diff] [review]
Update trunk to libpng-1.2.35

sr=dveditz fwiw, but I'm not an imglib peer.

Approving for the 1.9.0 branch
Comment 7 Joe Drew (not getting mail) 2009-02-17 18:37:06 PST
Comment on attachment 362738 [details] [diff] [review]
Update trunk to libpng-1.2.35

r=joe
Comment 8 Daniel Veditz [:dveditz] 2009-02-17 21:54:39 PST
Reed wondered if our own extra APNG stuff would need a similar fix but from what I can see none of the APNG code contains the pattern in question. Would be better if someone who knows the code double-checks though (maybe Glenn already did that? I'm not sure if APNG is in the upstream or not).

I wrote Glenn for clarification, and comment 3 means "don't check-in until Feb 19". He's willing to go with 00:00 UTC but that would still mean builds don't start until Thursday morning because I'm sure Ben doesn't want to start a long chunk of work after 7pm his time.
Comment 9 Samuel Sidler (old account; do not CC) 2009-02-17 22:58:17 PST
(In reply to comment #8)
> I wrote Glenn for clarification, and comment 3 means "don't check-in until Feb
> 19". He's willing to go with 00:00 UTC but that would still mean builds don't
> start until Thursday morning because I'm sure Ben doesn't want to start a long
> chunk of work after 7pm his time.

If that's the case, then let's get it checked in as late as possible on Wednesday night. If Ben is planning on starting builds sooner, we can check it in sooner. We can work this out tomorrow, in any case.
Comment 10 Glenn Randers-Pehrson 2009-02-18 04:02:04 PST
(In reply to comment #8)
> Reed wondered if our own extra APNG stuff would need a similar fix but from
> what I can see none of the APNG code contains the pattern in question. Would be
> better if someone who knows the code double-checks though (maybe Glenn already
> did that? I'm not sure if APNG is in the upstream or not).

The APNG stuff looks OK with respect to this bug.  No, APNG isn't in
the libpng upstream, but I do keep an eye on it anyway.
Comment 11 Glenn Randers-Pehrson 2009-02-18 17:36:13 PST
Libpng-1.2.35 is out.  Please clear the security flag.
Comment 12 Daniel Veditz [:dveditz] 2009-02-18 19:05:21 PST
Checked into CVS head (1.9.0) and GECKO190_20090217_RELBRANCH (1.9.0.7). The 1.9.0 branch was on 1.2.24 and after talking to Pav we figured it was safer to just take the mozilla-central version (1.2.34) and patch that.
Comment 13 Martin Stránský 2009-02-19 05:14:01 PST
Created attachment 363093 [details] [diff] [review]
minimal 1.8 version
Comment 14 Samuel Sidler (old account; do not CC) 2009-02-19 09:47:22 PST
Martin: What version of libpng is 1.8.1 on? It's probably worth upgrading it all the way to 1.2.35.
Comment 15 Glenn Randers-Pehrson 2009-02-19 10:11:42 PST
(In reply to comment #14)
> Martin: What version of libpng is 1.8.1 on? 

1.8.1.21pre (what you get when checking out 1.8) is
at libpng-1.2.7 dated September 12, 2004.  There have been
a few security bugs reported since then and it appears
that maybe one of them has been fixed in our embedded
copy.

The minimal patch above only addresses the most recent
vulnerability.   I think some of the others are just as bad.
Comment 16 Glenn Randers-Pehrson 2009-02-19 10:26:26 PST
(In reply to comment #15)

> I think some of the others are just as bad.

Or maybe not.  The last one had to do with using lcms which the
1.8 branch does not use.  The main reason for upgrading would
be to not have to look back over 5 years of changelogs and
bug reports to figure out if we are safe or not.  #:-(
Comment 17 Samuel Sidler (old account; do not CC) 2009-02-19 11:40:24 PST
I'm all for doing a full update on 1.8.1. Glenn, would you be willing to work a patch up for that?
Comment 18 Brian Polidoro 2009-02-19 12:24:50 PST
*** Bug 479223 has been marked as a duplicate of this bug. ***
Comment 19 Glenn Randers-Pehrson 2009-02-19 14:27:01 PST
(In reply to comment #17)
> I'm all for doing a full update on 1.8.1. Glenn, would you be willing to work a
> patch up for that?

OK.  It'll be huge because part of change since 1.2.7 is the removal of the unlicensed Intel assembler code (pngvcrd.c and pnggcrd.c).  Also there are
many cosmetic changes (indentation, whitespace).
Comment 20 Glenn Randers-Pehrson 2009-02-19 16:17:29 PST
Created attachment 363218 [details] [diff] [review]
Complete 1.8.1 patch, part 1
Comment 21 Glenn Randers-Pehrson 2009-02-19 16:18:31 PST
Created attachment 363219 [details] [diff] [review]
Complete 1.8.1 patch, part 2
Comment 22 Samuel Sidler (old account; do not CC) 2009-02-19 17:44:17 PST
Comment on attachment 363218 [details] [diff] [review]
Complete 1.8.1 patch, part 1

Joe: Wanna take a gander at this?
Comment 23 Samuel Sidler (old account; do not CC) 2009-02-19 19:45:12 PST
Comment on attachment 363218 [details] [diff] [review]
Complete 1.8.1 patch, part 1

Thanks Joe. Martin, feel free to land this on 1.8.1 when you get chance.

Approved for 1.8.1.21. a=ss
Comment 24 Martin Stránský 2009-02-19 23:21:40 PST
Thanks guys.
Comment 25 Glenn Randers-Pehrson 2009-02-20 03:53:43 PST
I inadvertently zeroed out libimg/png/mozpngconf.h in the big 2-part patch.  Libpng should run OK without it but will be about 50-100kbytes larger than necessary.  Some minor revision of Makefile.in in libimg/png and in libpr0n/decoders/png may be required to make mozpngconf.h work properly.

I can provide a patch to restore this functionality later on this afternoon.
Comment 26 Glenn Randers-Pehrson 2009-02-20 12:53:30 PST
Created attachment 363376 [details] [diff] [review]
Restore mozpngconf.h to 1.8.1

Apply this patch after applying the big 2-part patch, to restore mozpngconf.h and revise libpr0n/decoders/png/Makefile.in and libimg/png/Makefile.in
Comment 27 Glenn Randers-Pehrson 2009-02-20 12:56:10 PST
I could have removed the "APNG additions" block from mozpngconf.h, but it doesn't hurt anything to leave it in.
Comment 28 Daniel Veditz [:dveditz] 2009-02-27 15:32:46 PST
Comment on attachment 363219 [details] [diff] [review]
Complete 1.8.1 patch, part 2

sr=dveditz on all three 1.8.1 patches.
Comment 29 Daniel Veditz [:dveditz] 2009-02-27 22:35:55 PST
Checked into the 1.8 branch.
Comment 30 Eiichi 2009-02-28 04:07:58 PST
This seems to cause failure of building Camino 1.6-M1.8.
See <http://tinderbox.mozilla.org/showbuilds.cgi?tree=Camino>.
Comment 31 Glenn Randers-Pehrson 2009-02-28 04:59:24 PST
The only problem I know of with libpng-1.2.35 is in the png_debug() definition.
Is Camino 1.6-M1.8 being built with debug on?
Comment 32 Eiichi 2009-02-28 05:36:54 PST
(In reply to comment #31)
> The only problem I know of with libpng-1.2.35 is in the png_debug() definition.
> Is Camino 1.6-M1.8 being built with debug on?

Maybe with debuf off.
I'm not a developer and I only build my own build with "--disable-debug" option,
but I get build error same as http://tinderbox.mozilla.org/showlog.cgi?log=Camino/1235822040.1235822488.6445.gz
_______
make[6]: *** [nsPNGDecoder.o] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [tier_9] Error 2
make[2]: *** [alldep] Error 2
make[1]: *** [alldep] Error 2
make: *** [alldep] Error 2
Comment 33 Glenn Randers-Pehrson 2009-02-28 05:48:24 PST
(In reply to comment #32)
> (In reply to comment #31)

> but I get build error same as
> http://tinderbox.mozilla.org/showlog.cgi?log=Camino/1235822040.1235822488.6445.gz
> _______

That error log appears to be saying that it is using libpr0n/decoders/png with the "system libpng".  Are you in fact using the system libpng and not the embedded one in libimg/png ?
Comment 34 Glenn Randers-Pehrson 2009-02-28 05:55:32 PST
When using the "system libpng" you have to include the system png.h and not the embedded png.h in libimg/png.  It appears that there might be a mixup in this regard in the camino build script and the '#include "png.h"' directive inlibpr0n/decoders/png/nsPNGDecoder.cpp .
Comment 35 Glenn Randers-Pehrson 2009-02-28 06:01:45 PST
Please try this with your camino build:

In nsPNGDecoder.cpp, change

    #include "png.h"

to
  
    #ifdef MOZ_PNG_READ
    # include "png.h"
    #else
    # include <png.h>
    #endif
Comment 36 Glenn Randers-Pehrson 2009-02-28 06:08:23 PST
(In reply to comment #35)
> Please try this with your camino build:
>     # include <png.h>

If this works I will prepare a patch.

dveditz: Would you prefer a new small patch to the current 1.8.1
branch (with all three branch patches already applied) or a complete
replacement of the "restore mozpngconf.h" patch?
Comment 37 Eiichi 2009-02-28 06:32:42 PST
(In reply to comment #35)
> Please try this with your camino build:
> 
> In nsPNGDecoder.cpp, change
> 
>     #include "png.h"
> 
> to
> 
>     #ifdef MOZ_PNG_READ
>     # include "png.h"
>     #else
>     # include <png.h>
>     #endif

I test my Camino build with nsPNGDecoder.cpp changed as mentioned above, but I got a same error;
________
error: 'MOZ_PNG_get_progressive_ptr' was not declared in this scope
make[5]: *** [nsPNGDecoder.o] Error 1
make[4]: *** [libs] Error 2
make[3]: *** [libs] Error 2
make[2]: *** [tier_9] Error 2
make[1]: *** [alldep] Error 2
make: *** [alldep] Error 2
Comment 38 Glenn Randers-Pehrson 2009-02-28 06:54:03 PST
(In reply to comment #37)
> (In reply to comment #35)
> build with nsPNGDecoder.cpp changed as mentioned above, but I
> got a same error;

Please try the same except test MOZ_NATIVE_PNG.  I believe it is
0 when using the embedded library and 1 when using the system library.
It's a copy of $SYSTEM_PNG from configure.in.

     #if (MOZ_NATIVE_PNG == 0)
     # include "png.h"
     #else
     # include <png.h>
     #endif
Comment 39 Glenn Randers-Pehrson 2009-02-28 07:05:50 PST
On Sat, Feb 28, 2009 at 9:54 AM,  I wrote:

>     #if (MOZ_NATIVE_PNG == 0)
>     # include "png.h"
>     #else
>     # include <png.h>
>     #endif

Maybe it would be safer to do it the other way around, to avoid
any confusion between "0", "'0'", " ", and "":

     #if (MOZ_NATIVE_PNG == 1)
     # include <png.h>
     #else
     # include "png.h"
     #endif
Comment 40 Eiichi 2009-02-28 07:07:49 PST
(In reply to comment #38)

> Please try the same except test MOZ_NATIVE_PNG.  I believe it is
> 0 when using the embedded library and 1 when using the system library.
> It's a copy of $SYSTEM_PNG from configure.in.
> 
>      #if (MOZ_NATIVE_PNG == 0)
>      # include "png.h"
>      #else
>      # include <png.h>
>      #endif

That's the same.
________________
error: 'MOZ_PNG_get_progressive_ptr' was not declared in this scope
make[5]: *** [nsPNGDecoder.o] Error 1
make[4]: *** [libs] Error 2
make[3]: *** [libs] Error 2
make[2]: *** [tier_9] Error 2
make[1]: *** [alldep] Error 2
make: *** [alldep] Error 2

Just for reference, I'm building with .mozconfig
________________
. $topsrcdir/camino/config/mozconfig

mk_add_options MOZ_MAKE_FLAGS=-j4
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@

ac_add_options --enable-reorder
ac_add_options --enable-strip
ac_add_options --enable-static
ac_add_options --enable-static-libs
ac_add_options --enable-pthreads
ac_add_options --enable-optimize="-O3 -march=nocona -fforce-addr -msse3 -mfpmath=sse"

ac_add_options --disable-debug
ac_add_options --disable-shared
Comment 41 Eiichi 2009-02-28 07:19:01 PST
(In reply to comment #39)

> Maybe it would be safer to do it the other way around, to avoid
> any confusion between "0", "'0'", " ", and "":
> 
>      #if (MOZ_NATIVE_PNG == 1)
>      # include <png.h>
>      #else
>      # include "png.h"
>      #endif

Sorry, failure to build and I get the same error.
Comment 42 Glenn Randers-Pehrson 2009-02-28 07:44:24 PST
What happens when you put

    ac_add_options --without-system-png

in your .mozconfig?
Comment 43 Glenn Randers-Pehrson 2009-02-28 09:38:52 PST
I just checked out Mozilla-1.8 and find that libimg/png/mozpngconf.h has been updated according to the "Restore mozpngconf.h" patch but that libpr0n/decoders/png/Makefile.in was not.  That would probably explain the camino behavior that was reported.
Comment 44 Samuel Sidler (old account; do not CC) 2009-02-28 11:21:14 PST
Not just Camino. Dan checked this in on 1.8 and everything is currently red.

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla1.8

Dan, please either back this out or land a fix.
Comment 45 Smokey Ardisson (offline for a while; not following bugs - do not email) 2009-02-28 12:08:54 PST
(In reply to comment #43)
> I just checked out Mozilla-1.8 and find that libimg/png/mozpngconf.h has been
> updated according to the "Restore mozpngconf.h" patch but that
> libpr0n/decoders/png/Makefile.in was not.  That would probably explain the
> camino behavior that was reported.

(In reply to comment #44)
> Dan, please either back this out or land a fix.

I just checked in the missing hunk that Glenn identified in comment 43; I hope that was all that was missing.
Comment 46 Eiichi 2009-02-28 13:02:51 PST
(In reply to comment #45)
> I just checked in the missing hunk that Glenn identified in comment 43; I hope
> that was all that was missing.

I succeeded in building my Camino.
Comment 47 Samuel Sidler (old account; do not CC) 2009-02-28 13:09:40 PST
(In reply to comment #45)
> I just checked in the missing hunk that Glenn identified in comment 43; I hope
> that was all that was missing.

Thanks Smokey. It looks like the Camino 1.8 tree is building fine now as well. Hopefully Firefox and Thunderbird recover soon as well.

(fwiw, I gave Smokey approval to do this on IRC.)
Comment 48 Smokey Ardisson (offline for a while; not following bugs - do not email) 2009-02-28 19:10:40 PST
SeaMonkey Linux hoshi, the last box that was red from the 1.8 checkin here, went green about a half hour ago; the Firefox and Thunderbird boxes on Mozilla1.8 went green earlier this afternoon, as did Camino's 1.8 box.

Looks like Glenn reopened this after Eiichi's comments, so setting back to FIXED now that all the 1.8 boxen are green.
Comment 49 Daniel Veditz [:dveditz] 2009-03-01 01:19:20 PST
It was not FIXED before, it had landed only on the old branches (represented by keywords, not bug status). It is fixed now, however

http://hg.mozilla.org/mozilla-central/rev/ad8d75516c5e
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b3b4d684496d
Comment 50 Al Billings [:abillings] 2009-04-02 11:45:21 PDT
Verified code is checked in for 1.9.0.
Comment 51 Henrik Skupin (:whimboo) 2009-04-02 14:39:20 PDT
Verified by checkin on trunk and 1.9.1 too.
Comment 52 Mike Beltzner [:beltzner, not reading bugmail] 2009-08-25 10:38:17 PDT
Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

Note You need to log in before you can comment on or make changes to this bug.