The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
ImageLib
P2
critical
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: Glenn Randers-Pehrson, Assigned: Glenn Randers-Pehrson)

Tracking

(5 keywords)

Trunk
mozilla1.9.2a1
fixed1.8.1.21, fixed1.9.0.7, verified1.9.0.9, verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
blocking1.9.2 +
blocking1.9.1 +
blocking1.9.0.7 +
blocking1.9.0.9 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: [sg:critical?])

Attachments

(4 attachments, 1 obsolete attachment)

40.65 KB, patch
Joe Drew (not getting mail)
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
92.97 KB, patch
Joe Drew (not getting mail)
: review+
dveditz
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.8.1.next+
Details | Diff | Splinter Review
969.24 KB, patch
Joe Drew (not getting mail)
: review+
dveditz
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.8.1.next+
Details | Diff | Splinter Review
9.87 KB, patch
Joe Drew (not getting mail)
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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?
(Assignee)

Comment 2

8 years ago
Created attachment 362738 [details] [diff] [review]
Update trunk to libpng-1.2.35
(Assignee)

Comment 3

8 years ago
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.
(Assignee)

Comment 10

8 years ago
(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.
(Assignee)

Comment 11

8 years ago
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.
Keywords: fixed1.9.0.7, fixed1.9.0.8

Comment 13

8 years ago
Created attachment 363093 [details] [diff] [review]
minimal 1.8 version
Attachment #363093 - Flags: review?
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)
(Assignee)

Comment 15

8 years ago
(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.
(Assignee)

Comment 16

8 years ago
(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?

Updated

8 years ago
Duplicate of this bug: 479223
(Assignee)

Comment 19

8 years ago
(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).
(Assignee)

Comment 20

8 years ago
Created attachment 363218 [details] [diff] [review]
Complete 1.8.1 patch, part 1
(Assignee)

Comment 21

8 years ago
Created attachment 363219 [details] [diff] [review]
Complete 1.8.1 patch, part 2
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+

Comment 24

8 years ago
Thanks guys.
(Assignee)

Comment 25

8 years ago
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+
(Assignee)

Comment 26

8 years ago
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
(Assignee)

Comment 27

8 years ago
I could have removed the "APNG additions" block from mozpngconf.h, but it doesn't hurt anything to leave it in.
(Assignee)

Updated

8 years ago
Attachment #363376 - Flags: review?(joe)
Attachment #363376 - Flags: review?(joe) → review+
(Assignee)

Updated

8 years ago
Attachment #363219 - Flags: superreview?(dveditz)
(Assignee)

Updated

8 years ago
Attachment #363376 - Flags: superreview?(dveditz)
(Assignee)

Updated

8 years ago
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
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 30

8 years ago
This seems to cause failure of building Camino 1.6-M1.8.
See <http://tinderbox.mozilla.org/showbuilds.cgi?tree=Camino>.
(Assignee)

Updated

8 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 31

8 years ago
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

8 years ago
(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
(Assignee)

Comment 33

8 years ago
(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 ?
(Assignee)

Comment 34

8 years ago
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 .
(Assignee)

Comment 35

8 years ago
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
(Assignee)

Comment 36

8 years ago
(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

8 years ago
(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
(Assignee)

Comment 38

8 years ago
(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
(Assignee)

Comment 39

8 years ago
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

8 years ago
(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

8 years ago
(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.
(Assignee)

Comment 42

8 years ago
What happens when you put

    ac_add_options --without-system-png

in your .mozconfig?
(Assignee)

Comment 43

8 years ago
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.

Comment 46

8 years ago
(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
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
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.
Keywords: fixed1.9.0.9 → verified1.9.0.9
Verified by checkin on trunk and 1.9.1 too.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
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
Keywords: fixed1.9.2 → verified1.9.2
status1.9.2: --- → beta1-fixed
You need to log in before you can comment on or make changes to this bug.