373249 - libpng v1.2.12 contains security bug

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[lsg-mtso]

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: http://switch.dl.sourceforge.net/sourceforge/libpng/libpng-1.2.12-ADVISORY.txt.

This vulnerability can result in remote code execution

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

http://nvd.nist.gov/nvd.cfm?cvename=CVE-2006-5793

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.

Comment 1

[Benjamin Smedberg]

IMO We need to take this for 2003/15011.

Comment 2

[Benjamin Smedberg]

Attachment: Localized libpng fix, rev. 1

This is a patch from the description in http://switch.dl.sourceforge.net/sourceforge/libpng/libpng-1.2.12-ADVISORY.txt

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!

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #0)
> This vulnerability can result in remote code execution

Other comments on the vulnerability (e.g., at http://nvd.nist.gov/nvd.cfm?cvename=CVE-2006-5793 and https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=211705 and https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=216706 ) 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.

Comment 4

[lsg-mtso]

Agree, should only result in a crash.  Lowering severity.

Comment 5

[Mike Connor [:mconnor]]

a crash is still critical, fixing severity.

This isn't going to block 1.8.1.3/1.8.0.11, moving out noms

Comment 6

[Benjamin Smedberg]

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.

Comment 7

[Andrew Smith]

I'll do it tomorrow.

Comment 8

[Andrew Smith]

Attachment: upgrade to libpng 1.2.16

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.

Comment 9

[Stuart Parmenter]

checked in

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should you bump the MOZPNG variable in configure.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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Then again, if we do that, maybe we should also make the version check failure be an error, per bug 375921 comment 3.)

Comment 12

[Andrew Smith]

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 13

[Daniel Veditz [:dveditz]]

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.

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Andrew Smith]

Attachment: upgrade to libpng 1.2.16 for 1_8_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 16

[Stuart Parmenter]

Comment on attachment 262277 [details] [diff] [review]
upgrade to libpng 1.2.16 for 1_8_BRANCH

rs=me for external library upgrade

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 18

[Daniel Veditz [:dveditz]]

(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).

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Linux distros have been shipping Firefox against newer libpngs.  (Fedora Core 6 against 1.2.10 (probably plus some security fixes), for example.)

Comment 20

[Glenn Randers-Pehrson]

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 1.5.0.8 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.

Comment 21

[Daniel Veditz [:dveditz]]

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 2.0.0.4 unchanged, but if Firefox users are at risk I want to fix it.

Comment 22

[Glenn Randers-Pehrson]

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.

Comment 23

[Glenn Randers-Pehrson]

(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).

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 25

[Glenn Randers-Pehrson]

modules/libimg/png/mozpngconf.h should be updated on branch to what's in trunk.  Also configure.in has some PNG-related stuff that should be updated at the same time.

Comment 26

[Glenn Randers-Pehrson]

Attachment: Update mozpngconf.h and configure.in on 1.8 BRANCH

Use this patch together with 262277 to update libpng to 1.2.16 on the 1.8 BRANCH

Comment 27

[Glenn Randers-Pehrson]

Attachment: Update mozpngconf.h and configure.in on 1.8 BRANCH (v1)

Remove some testing cruft

Comment 28

[Glenn Randers-Pehrson]

Comment on attachment 262381 [details] [diff] [review]
Update mozpngconf.h and configure.in on 1.8 BRANCH (v1)

even more cruft appeared.

Comment 29

[Glenn Randers-Pehrson]

Attachment: Update mozpngconf.h and configure.in on 1.8 BRANCH (v2)

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Glenn, I'd still be interested in your input on the benefits are risks involved with applying these patches to our release branch.

Comment 31

[Glenn Randers-Pehrson]

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).

Comment 32

[Daniel Veditz [:dveditz]]

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.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 34

[Glenn Randers-Pehrson]

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.

Comment 35

[Glenn Randers-Pehrson]

See bug #374810 which has good reason for updating the branch to the latest libpng version.

Comment 36

[Daniel Veditz [:dveditz]]

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.

Comment 37

[timeless]

based on the comments, there wasn't even a dos, because there was no attack surface, updating whiteboard.