Closed Bug 492200 Opened 11 years ago Closed 10 years ago

Upgrade libpng to 1.2.37

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .7-fixed
status1.9.1 --- .2-fixed

People

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

References

()

Details

(Keywords: regression, verified1.9.1, Whiteboard: [3.6.x])

Attachments

(1 file, 3 obsolete files)

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()
Attached patch Update libpng to version 1.2.36 (obsolete) — Splinter Review
Attachment #376564 - Flags: review?(tor)
Joe's probably the best guy to review this.
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.
Attachment #376564 - Flags: review?(tor) → review+
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
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.
Summary: Upgrade libpng to 1.2.36 → Upgrade libpng to 1.2.37
Blocks: 495609
Blocks: 463465
No longer blocks: 495609
Attachment #376564 - Attachment is obsolete: true
Attachment #381886 - Flags: superreview?(tor)
Attachment #381886 - Flags: review?(joe)
Flags: blocking1.9.1?
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?
(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.
Blocks: 441971
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?
Flags: wanted1.9.1.x?
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.
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.
Flags: wanted1.9.1.x?
Flags: wanted1.9.1.x+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
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?
Flags: wanted1.9.1-
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.
Simple patch that adds two lines from 1.2.37 to address the potential
uninitialized memory reference in pngrutil.c
Attachment #382523 - Flags: superreview?(vladimir)
Attachment #382523 - Flags: review?(joe)
Attachment #382523 - Flags: superreview?(vladimir) → superreview+
Attachment #382523 - Flags: review?(joe) → review+
Keywords: checkin-needed
Attachment #381886 - Flags: superreview?(vladimir)
Attachment #381886 - Flags: superreview?(tor)
Attachment #381886 - Flags: review?(joe)
Attachment #381886 - Flags: review+
Attachment #381886 - Flags: superreview?(vladimir) → superreview+
Removed some dead code (see comment 13 in bug #441971)
Attachment #382894 - Flags: review?(joe)
Attachment #382894 - Flags: review?(joe) → review+
Attachment #381886 - Attachment is obsolete: true
Attachment #382894 - Attachment description: Update libpng to version 1.2.37 (v1) → Update trunk libpng to version 1.2.37 (v1)
Attachment #382894 - Flags: superreview?(vladimir)
Attachment #382523 - Attachment description: Port UMR bugfix from libpng-1.2.37 (v0) → Update branch with UMR bugfix from libpng-1.2.37 (v0)
You'll need approval to land on branch.
Keywords: checkin-needed
Attachment #382523 - Flags: approval1.9.1?
Attachment #382894 - Flags: approval1.9.1?
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.
@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.
Attachment #382894 - Flags: approval1.9.1?
Flags: blocking1.9.1.1?
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.
Flags: blocking1.9.1.1?
Whiteboard: [needs sr=vlad]
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?
Attachment #382523 - Flags: approval1.9.1? → approval1.9.1.1+
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
Attachment #382523 - Flags: approval1.9.1.2+
Attachment #382523 - Flags: approval1.9.1.1-
Attachment #382523 - Flags: approval1.9.1.1+
libpng-1.2.38 will be out next week.  The small patch will not change.
Depends on: 504805
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
Attachment #382894 - Attachment is obsolete: true
Attachment #382894 - Flags: superreview?(vladimir)
Whiteboard: [needs sr=vlad]
joedrew! Can you please land this on 1.9.1 asap?
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
What is the best/simplest way for QA to verify this bug for Firefox 3.5.2?
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.
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.
Keywords: verified1.9.1
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.
Status: RESOLVED → REOPENED
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Resolution: FIXED → ---
The "v0" patch is still good for applying to 1.9.2. Successful Tryserver builds are at
https://build.mozilla.org/tryserver-builds/glennrp@gmail.com-bug-492200-1.9.2-u-Jan13-UMR/
Keywords: regression
Joe/Jeff: do we need to take this in 1.9.2 before release, or can we take it in a 3.6.1?
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.
Flags: wanted1.9.2?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Whiteboard: [3.6.x][rc-ridealong]
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.
Flags: wanted1.9.2?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Flags: wanted1.9.2?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Do we have a branch-safe patch here, even? Removing [rc-ridealong] since I don't see one ...
Whiteboard: [3.6.x][rc-ridealong] → [3.6.x]
@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.
This never landed on the 1.9.2 branch
Attachment #382523 - Flags: approval1.9.2.3?
(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?
Nope. 

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

https://bugzilla.mozilla.org/attachment.cgi?id=407238
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
Attachment #382523 - Flags: approval1.9.2.4? → approval1.9.2.5+
(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.
Well, hopefully #441971 will fix three of them.

I'm not sure if we have a separate patch for the fourth test.
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.
Keywords: checkin-needed
Whiteboard: [3.6.x] → [3.6.x] [checkin-needed to 1.9.2]
Attachment #382523 - Flags: approval1.9.2.5+ → approval1.9.2.6+
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
Keywords: checkin-needed
Whiteboard: [3.6.x] [checkin-needed to 1.9.2] → [3.6.x]
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Only 1.9.2 needed it so it's fixed.
You need to log in before you can comment on or make changes to this bug.