Closed Bug 373249 Opened 17 years ago Closed 17 years ago

libpng v1.2.12 contains security bug


(Core :: Graphics: ImageLib, defect)

Not set





(Reporter: msg, Assigned: pavlov)




(Whiteboard: [sg:nse])


(3 files, 3 obsolete files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.0; SLCC1; .NET CLR 2.0.50727; Media Center PC 5.0; .NET CLR 3.0.04506; .NET CLR 1.1.4322; InfoPath.2)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a3pre) Gecko/20070307 Minefield/3.0a3pre

the libpng located at mozilla/modules/libimg/png is version 1.2.12 of libpng which has the following advisory:

This vulnerability can result in remote code execution

Source file pngset.c was checked to verify vulnerability exists in head of cvs.

Reproducible: Always

Steps to Reproduce:
1. Edit mozilla/modules/libimg/png/pngset.c
2. Goto line 892
3. Note following code: 

png_memcpy(to->entries, from->entries,
    from->nentries * png_sizeof(png_sPLT_t));

Upgrade to later version of libpng or apply code change listed in libpng advisory.
IMO We need to take this for 2003/15011.
Component: Security → ImageLib
Ever confirmed: true
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
Product: Firefox → Core
QA Contact: firefox → imagelib
Attached patch Localized libpng fix, rev. 1 (obsolete) — Splinter Review
This is a patch from the description in

We should also check a diff of 1.2.12 -> 1.2.13 to see if this is correct and if there are any other fixes we should be taking. The current libpng version is 1.2.16!
Attachment #257883 - Flags: review?
Attachment #257883 - Flags: review? → review?(pavlov)
(In reply to comment #0)
> This vulnerability can result in remote code execution

Other comments on the vulnerability (e.g., at and and ) seem to disagree.

And I think I disagree as well, since the bug appears to be:
 * overallocation of a buffer
 * overcopying (by the same amount) into that buffer
so the potential for crash is only because of the potential to page fault while reading garbage into the extra space at the end of the buffer that will never be used.
Agree, should only result in a crash.  Lowering severity.
Severity: critical → normal
a crash is still critical, fixing severity.

This isn't going to block, moving out noms
Severity: normal → critical
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.3-
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.11?
Flags: blocking1.8.0.11-
Attachment #257883 - Flags: review?(pavlov) → review+
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
I tried to do a basic upgrade from 1.2.12 to 1.2.16 on the trunk, using a diff between the two releases. The patch does not apply correctly. There seems to have been haphazard upgrades in the past, and that problem is now compounded by aPNG.

I'd really like someone who knows libpng to take a look at doing a complete version upgrade, instead of piecemeal fixes that will make real upgrades even harder in the future.
I'll do it tomorrow.
Assignee: nobody → pavlov
Attached patch upgrading libpng to 1.2.16

Tested it, but I don't think there was a need, there are no major changes.

I filed a bug with the mailing list but it's not something that affects Mozilla (variable declaration at the beginning of a block) so I figure the next libpng release will be easier to apply if we don't duplicate the fix.
Attachment #257883 - Attachment is obsolete: true
Attachment #260637 - Flags: review?(pavlov)
Attachment #260637 - Flags: review?(pavlov) → review+
Attachment #260637 - Flags: superreview?(tor)
Attachment #260637 - Flags: superreview?(tor) → superreview+
checked in
Closed: 17 years ago
Resolution: --- → FIXED
Should you bump the MOZPNG variable in  That currently says our minimum --with-system-png version is 1.2.7.  We probably don't want Linux distros shipping Mozilla with insecure libpngs.
(Then again, if we do that, maybe we should also make the version check failure be an error, per bug 375921 comment 3.)
looks like it won't compile on windows with that bug.

the 'if(png_ptr == NULL) return;' on pngpread.c:1738 needs to be moved to follow the endif
Comment on attachment 260637 [details] [diff] [review]
upgrade to libpng 1.2.16

Pav: do we need a different patch for the branches? Both 1.8 branches say "libpng version 1.2.7 - September 12, 2004" -- that's bad.
At least one of the bugs fixed in 1.2.12 itself was a buffer overrun of two bytes. Possibly exploitable, not much to play with.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Whiteboard: [sg:nse dos] (moderate/critical on branch?)
Whiteboard: [sg:nse dos] (moderate/critical on branch?) → [sg:nse dos] [sg:moderate/sg:critical on branch]
An upgrade from libpng 1.2.7 to 1.2.16 for MOZILLA_1_8_BRANCH. It's just the vanilla libpng.

Tested on linux, couldn't compile the branch on windows for unrelated reasons. Fix from comment #12 is in this patch also.

Please let me know if there is another branch that needs this upgrade and the patch doesn't apply cleanly.
Comment on attachment 262277 [details] [diff] [review]
upgrade to libpng 1.2.16 for 1_8_BRANCH

rs=me for external library upgrade
Attachment #262277 - Flags: superreview+
Attachment #262277 - Flags: review+
Attachment #262277 - Flags: approval1.8.1.4?
Attachment #262277 - Flags: approval1.8.0.12?
Do we really want to take the full shebang? This seems very risky for being a branch patch. According to stuart we haven't upgraded the full thing in a long time (disregarding a few recent upgrades on trunk which doesn't get much testing).

Is there any way we could just take the security updates that were done instead? I'm definitely nervous about regressions here.
(In reply to comment #17)
> Do we really want to take the full shebang? This seems very risky for being a
> branch patch. [...] Is there any way we could just take the security updates
> that were done instead? I'm definitely nervous about regressions here.

I think I'm more nervous about trying to cherry-pick out of the libpng distribution considering how far behind we were for FF2, unless someone from libpng like Glenn wants to bless doing so. A 200K patch sounds big, but if we end up with the stock libpng that's shipping in lots of places that doesn't sound that risky to me.  (Unless the libpng we were using was seriously customized to start with -- then who knows).
Linux distros have been shipping Firefox against newer libpngs.  (Fedora Core 6 against 1.2.10 (probably plus some security fixes), for example.)
This bug does not present a security problem to Firefox.  The
advisory referenced in the bug description says

"Libraries that are built with PNG_NO_READ_sPLT defined are not
vulnerable.  Neither are applications that use png_set_keep_unknown_chunks
with PNG_HANDLE_CHUNK_NEVER to ignore the sPLT chunk."

Both situations are true for Firefox and later.  The first
covers the embedded library and the second covers the use of an unpatched
system library.

I think the security-sensitivity of this bug can be removed.

The misplaced NULL test that Andrew mentioned in comment #12 will be
fixed in libpng 1.2.17.
Glenn: ignore the bug summary for the moment, that applies to the trunk. See comment 13, our shipping branch versions (FF1.5x and FF2.0x) appear to be using 1.2.7. That's missing at least the fix for CVE-2006-3334 and the sCAL chunk-writing bug (however rare it might be in normal practice, if it's exploitable it could be used).

I'd be very happy to hear we don't need to worry about those and can ship Firefox unchanged, but if Firefox users are at risk I want to fix it.
It's the same deal with sCAL.  The vulnerable code is #ifdef'ed out of the
embedded library and the unknown-chunk handler prevents Firefox from
ever entering the vulnerable code in the system library.

On the other hand, no harm is done by upgrading to the current libpng.
I'll have a look at the patch today, but at first glance it looks OK.
(In reply to comment #22)
> the unknown-chunk handler prevents Firefox from
> ever entering the vulnerable code in the system library.

Correction; it is the fact that libpr0n doesn't call the sCAL chunk-writing
function that prevents Firefox from ever entering the vulnerable code (the
unknown-chunk handler protects Firefox against existing and future bugs in
unused chunk handlers while *reading* PNG files).
Glenn: What i'm worried about in the upgrade is potenial regressions, say in parsing slightly errorous png files off the web. In general it would be great to get your input on the benefits and risks involved with this upgrade.

I'm all for upgrading to latest on trunk (which it looks like has already happened), what i'm less sure about is doing it on a stable branch that gets relatively little testing before release.
modules/libimg/png/mozpngconf.h should be updated on branch to what's in trunk.  Also has some PNG-related stuff that should be updated at the same time.
Use this patch together with 262277 to update libpng to 1.2.16 on the 1.8 BRANCH
Remove some testing cruft
Attachment #262379 - Attachment is obsolete: true
Comment on attachment 262381 [details] [diff] [review]
Update mozpngconf.h and on 1.8 BRANCH (v1)

even more cruft appeared.
Attachment #262381 - Attachment is obsolete: true
Glenn, I'd still be interested in your input on the benefits are risks involved with applying these patches to our release branch.
I do not know of any security risk associated with the current libpng
(1.2.7-based) that is embedded in the 1.8 branch.  It does contain some
bugs but they are in code that is unused by Firefox, and Firefox cannot
be tricked into using that code by any malformed PNGs.  I do not believe
upgrading to 1.2.16 would introduce any new security risks.  Upgrading
to libpng-1.2.16 and applying the mozpngconf.h patch will result in about
a 10 percent reduction in the size of the compiled libpng (more code
is ifdef'ed out resulting in even less potential vulnerability).
Whiteboard: [sg:nse dos] [sg:moderate/sg:critical on branch] → [sg:nse dos]
Given Glenn's statement we don't have to take this, and it's a big patch coming in late in the release. We'll look at it early in the next release, instead.
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12-
Flags: blocking1.8.0.12+
Attachment #262277 - Flags: approval1.8.1.5?
Attachment #262277 - Flags: approval1.8.1.4?
Attachment #262277 - Flags: approval1.8.0.13?
Attachment #262277 - Flags: approval1.8.0.12?
Honestly, as great as this seems for trunk, i don't really see a reason to take it on branch. But we can debate that for the next release.
There is some relevant discussion in bug #334110 (upgrade to libpng-1.2.12) in which tor kept saying it's too late to put this on the branch.  Nothing has really changed since then, except that it's a lot later now, so there's no new reason to check up-to-date libpng in to the branch.

Someone please remove the security-sensitive check.  This is very old news.
See bug #374810 which has good reason for updating the branch to the latest libpng version.
Looks like we have a branch spot-fix for bug 374810 without having to upgrade libpng for 1.8.x, we think we can live without this.
Group: security
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5-
Attachment #262277 - Flags: approval1.8.1.5?
Attachment #262277 - Flags: approval1.8.0.13?
Flags: wanted1.8.1.x-
based on the comments, there wasn't even a dos, because there was no attack surface, updating whiteboard.
Whiteboard: [sg:nse dos] → [sg:nse]
You need to log in before you can comment on or make changes to this bug.