492200 - Upgrade libpng to 1.2.37

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Glenn Randers-Pehrson]

Libpng-1.2.36 has been released.  It is mostly a code-cleanup release.  This change might affect mozilla when it writes a PNG with the iCCP chunk, if
color management is sometime added to libpr0n/encoders/png:

 Fixed potential memory leak of "new_name" in png_write_iCCP()

Comment 1

[Glenn Randers-Pehrson]

Attachment: Update libpng to version 1.2.36

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Joe's probably the best guy to review this.

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 376564 [details] [diff] [review]
Update libpng to version 1.2.36

>-     png_ptr->big_row_buf = (png_bytep)png_malloc(png_ptr, row_bytes+64);
>-     png_ptr->row_buf = png_ptr->big_row_buf+32;
>-     png_ptr->old_big_row_buf_size = row_bytes+64;
>+      png_ptr->big_row_buf = (png_bytep)png_malloc(png_ptr, row_bytes + 64);
>+     if (png_ptr->interlaced)
>+       png_memset(png_ptr->big_row_buf, 0, png_ptr->rowbytes + 64);
>+     png_ptr->row_buf = png_ptr->big_row_buf + 32;
>+     png_ptr->old_big_row_buf_size = row_bytes + 64;

Is there a reason you use row_bytes + 64 in the malloc, and png_ptr->rowbytes in the memset?

Also, why not zero out the buffer unconditionally?

Everything's fine otherwise.

Comment 4

[Glenn Randers-Pehrson]

It looks like a mistake, and that we should be using row_bytes in both
places.  I don't think it's something anyone could exploit, though.
Note that in libpng-1.4.0 we use calloc() instead, so such inconsistency
will not be possible in the future.  We'll fix this in 1.2.37.

We don't zero out the buffer in the usual, noninterlaced case because
we know it's not needed and we are just trying  to save a few nanoseconds
of computing time.

Glenn

Comment 5

[Glenn Randers-Pehrson]

Libpng-1.2.37 will be out in about a week (probably June 4), so we might as well skip 1.2.36. I'm changing the bug summary. 1.2.37 contains mostly cosmetic changes, but the problems mentioned in comment #3 will be addressed.

Comment 6

[Glenn Randers-Pehrson]

Attachment: Update libpng to version 1.2.37 (v0)

Comment 7

[Justin Dolske [:Dolske]]

From IRC:

1.2.37 fixes a potential security bug -- http://www.libpng.org/pub/png/libpng.html

I'm a little concerned that there's a libpng security fix that we only seem to be hearing about casually, after the fact. Nothing in this bug indicates it's a potential security issue. Was there a communications breakdown somewhere?

Comment 8

[Glenn Randers-Pehrson]

(In reply to comment #7)
> From IRC:
> 
> 1.2.37 fixes a potential security bug --
> http://www.libpng.org/pub/png/libpng.html
> 
> I'm a little concerned that there's a libpng security fix that we only seem to
> be hearing about casually, after the fact. Nothing in this bug indicates it's a
> potential security issue. Was there a communications breakdown somewhere?

It's the same bug that is mentioned here in comment #3 and comment #4.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't think this blocks, and am a little nervous about taking a fix of this magnitude for a "potential security bug." How much testing do we have around this library?

Comment 10

[Glenn Randers-Pehrson]

My personal opinion, not necessarily that of the entire PNG group, is that it is not a security bug.  We are talking about using uninitialized, but properly malloc'ed bytes.  The only use libpng could possibly make of them is as color samples or indices and any byte is a valid color sample or index* so all that would go wrong is that a pixel might have an unintended color.  Furthermore, the pixels in question would be off the end of the scanline so no one would ever see them.  Anyhow they have been uninitialized since the beginning of libpng.

*an out-of-range index for 1, 2, or 4-bit PNGS is not valid per the PNG spec,
but libpng always allocates a full 256-entry palette so it won't cause any
trouble.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Based on comment 10, I don't think I would block the release on this. When the patch is reviewed it should be checked in on mozilla-central immediately and nominated for approval if it cycles green and passes all tests. If we don't get it in time for release, we'll get it for 1.9.1.1.

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We shouldn't take this for 3.5.0/1.9.1, IMO -- too close to the line, it's too exposed an attack surface.  Have we fuzzed this version of libpng?

Comment 13

[Glenn Randers-Pehrson]

I could supply a small patch that takes care of the "security problem" in the existing embedded libpng by changing a line or two of code.  It might be easier to fuzz that.  Then upgrade the rest of libpng at our leisure.

Comment 14

[Glenn Randers-Pehrson]

Attachment: Update branch with UMR bugfix from libpng-1.2.37 (v0)

Simple patch that adds two lines from 1.2.37 to address the potential
uninitialized memory reference in pngrutil.c

Comment 15

[Glenn Randers-Pehrson]

Attachment: Update trunk libpng to version 1.2.37 (v1)

Removed some dead code (see comment 13 in bug #441971)

Comment 16

[Dão Gottwald [:dao]]

You'll need approval to land on branch.

Comment 17

[Glenn Randers-Pehrson]

OK.  I'm asking for approval of both the complete patch ("Update trunk") and the small bugfix ("Update branch").  If the complete patch is approved then forget the bugfix.

Comment 18

[Glenn Randers-Pehrson]

@vladimir: the only difference between the obsolete v0 complete patch that you sr'ed and the v1 complete patch is removal of some dead code that was already inside #if 0/#endif.

Comment 19

[Samuel Sidler (old account; do not CC)]

Would probably help to have Vlad CCed on this bug...

I don't think we should block on this for 1.9.1.1, but taking the small patch makes sense.

Comment 20

[Samuel Sidler (old account; do not CC)]

Comment on attachment 382523 [details] [diff] [review]
Update branch with UMR bugfix from libpng-1.2.37 (v0)

Approved for 1.9.1.1. a=ss

Can someone help get this landed on 1.9.1 for Glenn?

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 382523 [details] [diff] [review]
Update branch with UMR bugfix from libpng-1.2.37 (v0)

We're now firedrilling for 1.9.1.1, so this will have to wait for 1.9.1.2

Comment 22

[Glenn Randers-Pehrson]

libpng-1.2.38 will be out next week.  The small patch will not change.

Comment 23

[Glenn Randers-Pehrson]

Comment on attachment 382894 [details] [diff] [review]
Update trunk libpng to version 1.2.37 (v1)

This patch is incorporated in the patch to update to libpng-1.2.38 in bug #504805

Comment 24

[Samuel Sidler (old account; do not CC)]

joedrew! Can you please land this on 1.9.1 asap?

Comment 25

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/17b86640570d

I'm not 100% sure whether this is a complete fix from this bug's point of view, but I am gonna call it fixed and let Glenn reopen if he needs to.

Comment 26

[u279076]

What is the best/simplest way for QA to verify this bug for Firefox 3.5.2?

Comment 27

[Glenn Randers-Pehrson]

I don't know of any exploit or test case.  About all you can do is look at
the source and verify that the two lines of code were added.

There are problems with artifacts in APNG rendering that are not addressed
by the patch, but those aren't security problems.

Comment 28

[u279076]

Using the mozilla-1.9.1 I pulled an hour ago I can see the changes from comment 25.  Based on comment 27, verified1.9.1.

Comment 29

[Glenn Randers-Pehrson]

Reopening.  Because the latest libpng (1.2.37 or later) has not made it into 1.9.2, there is a reversion of this bug from 1.9.1, and this small patch should be checked in to 1.9.2.

Comment 30

[Glenn Randers-Pehrson]

The "v0" patch is still good for applying to 1.9.2. Successful Tryserver builds are at
https://build.mozilla.org/tryserver-builds/[e-mail]-bug-492200-1.9.2-u-Jan13-UMR/

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Joe/Jeff: do we need to take this in 1.9.2 before release, or can we take it in a 3.6.1?

Comment 32

[Joe Drew (not getting mail)]

This shouldn't cause a respin on its own, but should be a ridealong if there's going to be a respin. I'm okay with it being only in a 3.6.1.

Comment 33

[Glenn Randers-Pehrson]

I agree with waiting for 3.6.1 if it's too late for 3.6.  I don't think this is a real security problem (see my comment #10), but the black hats have had about 7 months now since we made the bug public to find an exploit.

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we have a branch-safe patch here, even? Removing [rc-ridealong] since I don't see one ...

Comment 35

[Glenn Randers-Pehrson]

@mike, See comment #30.  I did the Tryserver run with the v0 patch as input and checked "Use another mercurial repository: http://hg.mozilla.org/releases/mozilla-1.9.2".  I hope that means the patch is "branch-safe".  Note that 1.9.1 has already had the patch checked in and 1.9.3/trunk don't need it because the fix is incorporated in libpng-1.4.0.  Also note that checkin of the v0 patch to 1.9.2 needs to be coordinated with the small patch for 1.9.2 in bug #497056 because both patches update the same lines in modules/libimg/png/MOZCHANGES.

Comment 36

[Daniel Veditz [:dveditz]]

This never landed on the 1.9.2 branch

Comment 37

[Reed Loden [:reed]]

(In reply to comment #36)
> This never landed on the 1.9.2 branch

Has this not been obsoleted by bug 504805? Did libpng 1.4.x not land on 1.9.2 branch?

Comment 38

[Max Stepin]

Nope. 

All four testcases from 504805 are still buggy in Firefox 3.6.3:

https://bugzilla.mozilla.org/attachment.cgi?id=407238

Comment 39

[LegNeato]

Comment on attachment 382523 [details] [diff] [review]
Update branch with UMR bugfix from libpng-1.2.37 (v0)

a=LegNeato for 1.9.2.5. Please ONLY land this on mozilla-1.9.2 default, as we
are still working on 1.9.2.4 on the relbranch

Comment 40

[Glenn Randers-Pehrson]

(In reply to comment #38)
> Nope. 
> 
> All four testcases from 504805 are still buggy in Firefox 3.6.3:

Max, the (UMR) patch being discussed here won't fix those.

Comment 41

[Max Stepin]

Well, hopefully #441971 will fix three of them.

I'm not sure if we have a separate patch for the fourth test.

Comment 42

[Glenn Randers-Pehrson]

I need a "check-in buddy" to get this in to the 1.9.2 branch.

Please check the patch in and set the "status1.9.2" field to ".6-fixed".

Code-freeze for the 1.9.2.6 release is planned for the end of next
week. Please land soon and don't leave it to the last minute.

Comment 43

[(no longer active)]

I'm not sure if the status here should be RESOLVED FIXED or not, but pushed to branch anyways:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a478fe5144d5

Comment 44

[Glenn Randers-Pehrson]

Only 1.9.2 needed it so it's fixed.