Closed Bug 478901 Opened 15 years ago Closed 15 years ago

Upgrade libpng to 1.2.35 (libpng-1.2.34 and earlier might free undefined pointers)

Categories

(Core :: Graphics: ImageLib, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1
Tracking Status
status1.9.2 --- beta1-fixed

People

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

References

Details

(5 keywords, Whiteboard: [sg:critical?])

Attachments

(4 files, 1 obsolete file)

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.
Thanks Glenn
Whiteboard: [sg:critical?] confidential until libpng 1.2.35 is released
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.2?
Flags: blocking1.9.1?
Flags: blocking1.9.0.8?
Flags: blocking1.8.1.next?
Libpng-1.2.35 is presently scheduled for release on February 19, 2009.
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Priority: -- → P2
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.
Flags: blocking1.9.0.7?
Going to re-spin 3.0.7 for this :-(
Flags: blocking1.9.0.8?
Flags: blocking1.9.0.8+
Flags: blocking1.9.0.7?
Flags: blocking1.9.0.7+
Attachment #362738 - Flags: review?(pavlov)
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
Attachment #362738 - Flags: superreview+
Attachment #362738 - Flags: approval1.9.0.7+
Attachment #362738 - Flags: review?(pavlov) → review+
Comment on attachment 362738 [details] [diff] [review]
Update trunk to libpng-1.2.35

r=joe
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.
Whiteboard: [sg:critical?] confidential until libpng 1.2.35 is released → [sg:critical?] embargoed until Feb 19, including check-ins
(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.
(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.
Libpng-1.2.35 is out.  Please clear the security flag.
Group: core-security
Whiteboard: [sg:critical?] embargoed until Feb 19, including check-ins → [sg:critical?]
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.
Martin: What version of libpng is 1.8.1 on? It's probably worth upgrading it all the way to 1.2.35.
Summary: 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 pointers)
(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.
(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.  #:-(
I'm all for doing a full update on 1.8.1. Glenn, would you be willing to work a patch up for that?
(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 on attachment 363218 [details] [diff] [review]
Complete 1.8.1 patch, part 1

Joe: Wanna take a gander at this?
Attachment #363218 - Flags: review?(joe)
Attachment #363218 - Flags: review?(joe) → review+
Attachment #363219 - Flags: review+
Attachment #363218 - Flags: approval1.8.1.next+
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
Attachment #363219 - Flags: approval1.8.1.next+
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.
Flags: blocking1.8.1.next? → blocking1.8.1.next+
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
I could have removed the "APNG additions" block from mozpngconf.h, but it doesn't hurt anything to leave it in.
Attachment #363376 - Flags: review?(joe)
Attachment #363376 - Flags: review?(joe) → review+
Attachment #363219 - Flags: superreview?(dveditz)
Attachment #363376 - Flags: superreview?(dveditz)
Attachment #363218 - Flags: superreview?(dveditz)
Attachment #363218 - Flags: superreview?(dveditz) → superreview+
Attachment #363376 - Flags: superreview?(dveditz) → superreview+
Comment on attachment 363219 [details] [diff] [review]
Complete 1.8.1 patch, part 2

sr=dveditz on all three 1.8.1 patches.
Attachment #363219 - Flags: superreview?(dveditz) → superreview+
Checked into the 1.8 branch.
Keywords: fixed1.8.1.21
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
This seems to cause failure of building Camino 1.6-M1.8.
See <http://tinderbox.mozilla.org/showbuilds.cgi?tree=Camino>.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
(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
(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 ?
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 .
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
(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?
(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
(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
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
(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
(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.
What happens when you put

    ac_add_options --without-system-png

in your .mozconfig?
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.
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.
(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.
(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.
(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.)
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Attachment #363093 - Attachment is obsolete: true
Attachment #363093 - Flags: review?
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
Keywords: fixed1.9.1
Verified code is checked in for 1.9.0.
Verified by checkin on trunk and 1.9.1 too.
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
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)
Keywords: fixed1.9.2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: