Last Comment Bug 374810 - (CVE-2007-2445) Crash [@ MOZ_PNG_init_read_transf] with png image
(CVE-2007-2445)
: Crash [@ MOZ_PNG_init_read_transf] with png image
Status: RESOLVED FIXED
[sg:dos]
: crash, testcase, verified1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9alpha8
Assigned To: Glenn Randers-Pehrson
:
: Milan Sreckovic [:milan]
Mentors:
data:image/png,%89PNG%0D%0A%1A%0A%00%...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-21 12:15 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
13 users (show)
pavlov: blocking1.9+
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (1.12 KB, patch)
2007-03-22 01:27 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
bugfix, MOZCHANGES, minimum system library 1.2.17 (checked in to trunk) (2.15 KB, patch)
2007-05-05 01:33 PDT, Glenn Randers-Pehrson
tor: review+
dveditz: superreview+
Details | Diff | Splinter Review
patch for 1.8 branch (checked in to branch) (2.15 KB, patch)
2007-05-05 06:02 PDT, Glenn Randers-Pehrson
tor: review+
dveditz: superreview+
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-03-21 12:15:20 PDT
When trying to open this image, Mozilla crashes.
It also happens in Mozilla1.7.13 and other branches, so I'm filing this as security sensitive, just to be sure.

Talkback ID:
MOZ_PNG_init_read_transf  [mozilla/modules/libimg/png/pngrtran.c, line 816]
MOZ_PNG_read_start_row  [mozilla/modules/libimg/png/pngrutil.c, line 3156]
MOZ_PNG_read_update_info  [mozilla/modules/libimg/png/pngread.c, line 628]
MOZ_PNG_push_have_info  [mozilla/modules/libimg/png/pngpread.c, line 1710]
MOZ_PNG_proc_some_data  [mozilla/modules/libimg/png/pngpread.c, line 55]
nsPipeInputStream::ReadSegments  [mozilla/xpcom/io/nspipe3.cpp, line 766]
nsPNGDecoder::WriteFrom  [mozilla/modules/libpr0n/decoders/png/nspngdecoder.cpp, line 259]
imgRequest::OnDataAvailable  [mozilla/modules/libpr0n/src/imgrequest.cpp, line 903]
Comment 1 Daniel Veditz [:dveditz] 2007-03-21 15:17:16 PDT
png_ptr->num_trans is 1 but png_ptr->trans is null, so dereferencing png_ptr->trans[i] crashes.

How did we get in that state? If num_trans and trans can get out of sync there might be ways to get trans set to a non-null value. Are we trusting the image data without validation?
Comment 2 Mats Palmgren (:mats) 2007-03-22 01:26:14 PDT
Also occurs on Linux.
Comment 3 Mats Palmgren (:mats) 2007-03-22 01:27:40 PDT
Created attachment 259303 [details] [diff] [review]
wip

I think the bug is in png_handle_tRNS:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/modules/libimg/png/pngrutil.c&rev=3.13&root=/cvsroot&mark=1309,1318,1319,1321#1288

We set "png_ptr->num_trans" to 1 at line 1309 (ok), but then
there is a CRC error detected on line 1318 and we return
early - it is png_set_tRNS() at the end that allocates
the 'trans' buffer.

FYI, this patch makes us display some kind of fallback image
that says we don't support APNG. I'm not sure if that's the
expected result.
Comment 4 Mats Palmgren (:mats) 2007-03-22 01:28:40 PDT
I also looked briefly at png_set_tRNS.
It does not seem to handle OOM very well...
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/modules/libimg/png/pngset.c&rev=3.11&root=/cvsroot&mark=927,930,932,934,942,943,946#909

We should probably set num_trans=0 here and not set the
FREE_TRNS flag and not set info_ptr->valid bit if OOM occurs.
Comment 5 Mats Palmgren (:mats) 2007-03-22 01:40:03 PDT
Clamping num_trans to PNG_MAX_PALETTE_LENGTH in png_set_tRNS wouldn't
hurt too...
Comment 6 Stuart Parmenter 2007-03-23 00:18:20 PDT
It might be worth asking the libpng people since this is an external library?
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-03-23 17:27:21 PDT
Ah sorry, maybe this is basically what bug 373249 is about?
Comment 8 Mats Palmgren (:mats) 2007-03-23 22:39:05 PDT
bug 373249 looks different, I don't think it's related to this bug.
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2007-05-04 14:53:27 PDT
-> Mats, I know nothing about this
Comment 10 Glenn Randers-Pehrson 2007-05-04 21:58:38 PDT
Cannot figure out how to download the image.  Firefox crashes, Seamonkey
crashes and takes the windows display driver with it.  Lynx and IE can't
get past the "proceed" button.
Comment 11 Glenn Randers-Pehrson 2007-05-05 01:33:19 PDT
Created attachment 263849 [details] [diff] [review]
bugfix, MOZCHANGES, minimum system library 1.2.17 (checked in to trunk)

The patch seems correct.  Here is an updated patch that also updates MOZCHANGES and sets the minimum system libpng to 1.2.17 which will be released next week with the patch, and uses the bracketing style of libpng.
Comment 12 Glenn Randers-Pehrson 2007-05-05 06:02:25 PDT
Created attachment 263854 [details] [diff] [review]
patch for 1.8 branch (checked in to branch)
Comment 13 Glenn Randers-Pehrson 2007-05-05 06:08:40 PDT
(In reply to comment #0)
> When trying to open this image, Mozilla crashes.
> It also happens in Mozilla1.7.13 and other branches, so I'm filing this as
> security sensitive, just to be sure.

The vulnerability has existed in libpng since version 0.90 so it will affect
all versions of gecko, whether they use the system libpng or the embedded one.
Comment 14 Mats Palmgren (:mats) 2007-05-05 22:18:16 PDT
Comment on attachment 263854 [details] [diff] [review]
patch for 1.8 branch (checked in to branch)

> dnl Set the version number of the libs included with mozilla
> dnl ========================================================
> MOZJPEG=62
>-MOZPNG=10207
>+MOZPNG=10217

This doesn't seem right, fixing this bug does not take us to 1.2.17
*on the branches* which is lacking (most of) the changes from 1.2.7
to 1.2.16, right?

Looks good otherwise, also for the 1.8.0 branch.
Comment 15 Glenn Randers-Pehrson 2007-05-05 22:37:38 PDT
(In reply to comment #14)


> This doesn't seem right, fixing this bug does not take us to 1.2.17
> *on the branches* which is lacking (most of) the changes from 1.2.7
> to 1.2.16, right?

See discussion in bug #373249.  Security-wise, the branch is at the same
level as trunk, except for some additional tests for NULL pointers on
the trunk.  But this bug casts a new light on the situation.  Unpatched
system libraries through 1.2.16 are vulnerable to a simple attack via
a PNG with a malformed tRNS chunk.  We don't have a way of detecting
whether the system library has been patched.  Of course they are all
unpatched now because we haven't released the patch yet (I've reported
it to CERT but have not heard back from them).
Comment 16 Mats Palmgren (:mats) 2007-05-05 22:53:54 PDT
Ok, that bug explains it, thanks.
Comment 17 Glenn Randers-Pehrson 2007-05-06 10:32:29 PDT
Comment on attachment 263849 [details] [diff] [review]
bugfix, MOZCHANGES, minimum system library 1.2.17 (checked in to trunk)

Tor: r?
Comment 18 Glenn Randers-Pehrson 2007-05-06 10:33:23 PDT
Comment on attachment 263854 [details] [diff] [review]
patch for 1.8 branch (checked in to branch)

tor: r for branch?
Comment 19 Glenn Randers-Pehrson 2007-05-06 14:59:46 PDT
(In reply to comment #15)
>(I've reported
> it to CERT but have not heard back from them).

Still no word from CERT.  Silly me forgot they are paid to respond and
therefore won't look at it until the next work day.

We should probably coordinate the release of the mozilla patch with
the libpng-1.2.17 release.
Comment 20 Daniel Veditz [:dveditz] 2007-05-07 10:22:02 PDT
If this doesn't make 1.8.1.4/1.8.0.12 then the next planned release is 1.8.1.5 in July. Can this wait, or should we squeeze it into 1.8.1.4 ahead of any coordination (and not announce anything).

What "new light" (comment 15) does this bug cast? It still looks like a null deref which we can live with. I guess the size of num_trans is under the control of the attacker, but there's at least one check that it's <= 256 which doesn't get an attacker into the heap on most OS's.
Comment 21 Glenn Randers-Pehrson 2007-05-07 10:43:09 PDT
"new light" is as I said in comment #10, seamonkey crashes and takes my
windows display with it.  Throws my screen into "safe" mode.

I doubt that an attacker could use it to break into a system or to elevate
priveleges.  This is probably only a Denial of Service problem.

This is new because it can be caused by a malformed PNG image on a web site.
The other NULL problems are only due to application programming errors,
which gecko/libpr0n doesn't have.

The erroneous num_trans can only be "1".

I still haven't heard from CERT (guess they don't come in early on Mondays) but suspect they will ask for 45 days.  I don't have a problem with releasing a change without anouncing why.
Comment 22 Glenn Randers-Pehrson 2007-05-07 10:55:14 PDT
I just now heard from CERT:

Thank you for the report, we are tracking this as VU#684664. We will follow up with our established contacts at libpng.

Comment 23 Glenn Randers-Pehrson 2007-05-07 14:52:08 PDT
Comment on attachment 263849 [details] [diff] [review]
bugfix, MOZCHANGES, minimum system library 1.2.17 (checked in to trunk)

sr anyone?
Comment 24 Glenn Randers-Pehrson 2007-05-07 19:15:58 PDT
(In reply to comment #3)

> FYI, this patch makes us display some kind of fallback image
> that says we don't support APNG. I'm not sure if that's the
> expected result.

Yes, I see that, too, with the patch and also with a MNG-supporting
build that does not use libpng and is therefore not vulnerable.  That
is correct behaviour for the MNG-supporting build but I'm not sure why
the patched one does not show the APNG animation.

Here's a "pngcrush -fix -v" report:

Reading IHDR chunk, length = 13.
Reading PLTE chunk, length = 48.
Reading tRNS chunk, length = 1.
libpng warning: [00][00][00][00]: CRC error
Reading bKGD chunk, length = 1.
Reading acTl chunk, length = 16.
Reading IDAT chunk, length = 2386.
Reading fcTl chunk, length = 25.
Reading tIME chunk, length = 7.
Reading fdAt chunk, length = 2554.
Reading fcTl chunk, length = 25.
Reading fdAt chunk, length = 2579.
[etc]

Hmmm, the chunk names are wrong (should end with L and T not l and t)
so that explains why we see the fallback.  It's proper behaviour.

Comment 25 Glenn Randers-Pehrson 2007-05-08 04:49:51 PDT
I received a request to use CVE name CVE-2007-2445 for this issue
Comment 26 Glenn Randers-Pehrson 2007-05-08 13:40:20 PDT
(In reply to comment #20)
> If this doesn't make 1.8.1.4/1.8.0.12 then the next planned release is 1.8.1.5
> in July.

When are 1.8.1.4/1.8.0.12 due out?  I've spoken with CERT and they don't
have a problem with a quick release, e.g., May 15th, with an announcement.

I was planning to release libpng-1.2.17 on May 12 but it's no problem waiting
until May 15 or so.

CERT is pretty much leaving it up to us when to disclose/release.
Comment 27 Glenn Randers-Pehrson 2007-05-14 04:47:26 PDT
Libpng-1.2.17, with the patch and a security advisory, will be released
tomorrow, May 15th, at noon EST.
Comment 28 Glenn Randers-Pehrson 2007-05-14 05:03:28 PDT
If anyone here has sr privileges, please do the superreviews.
Comment 29 Daniel Veditz [:dveditz] 2007-05-14 12:27:52 PDT
Comment on attachment 263849 [details] [diff] [review]
bugfix, MOZCHANGES, minimum system library 1.2.17 (checked in to trunk)

sr=dveditz
Comment 30 Daniel Veditz [:dveditz] 2007-05-14 12:28:20 PDT
Comment on attachment 263854 [details] [diff] [review]
patch for 1.8 branch (checked in to branch)

sr=dveditz
Comment 31 Glenn Randers-Pehrson 2007-05-14 14:04:07 PDT
The libpng release is delayed from noon until 4:00 PM EST tomorrow, May 15.
Comment 32 Glenn Randers-Pehrson 2007-05-14 14:52:05 PDT
Oops, EDT not EST.
Comment 33 Glenn Randers-Pehrson 2007-05-15 14:08:21 PDT
Libpng-1.2.17 has been released.
Comment 34 Daniel Veditz [:dveditz] 2007-06-15 10:30:38 PDT
Comment on attachment 263854 [details] [diff] [review]
patch for 1.8 branch (checked in to branch)

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Comment 35 Daniel Veditz [:dveditz] 2007-07-11 10:01:20 PDT
fix landed on branches
Comment 36 Daniel Veditz [:dveditz] 2007-07-12 17:06:41 PDT
fix checked into trunk.
Comment 37 Carsten Book [:Tomcat] 2007-07-12 18:09:46 PDT
verified fixed 1.8.1.5 using  Mozilla/5.0 (X11; U; Linux i686; en-US;
rv:1.8.1.5pre) Gecko/2007071216 BonEcho/2.0.0.5pre and Mozilla/5.0 (Windows; U;
Windows NT 6.0; en-US; rv:1.8.1.5pre) Gecko/2007071216 BonEcho/2.0.0.5pre also
a mac build with this testcase - no crash on testcase - adding verified keyword
Comment 38 Glenn Randers-Pehrson 2007-08-17 04:58:22 PDT
Someone please clear the security-sensitive flag.
Comment 39 Carsten Book [:Tomcat] 2007-08-23 04:51:27 PDT
also no crash with thunderbird 1.5.0.13 with thunderbrowse. Thunderbrowse mark this url as too long.
Comment 40 Carsten Book [:Tomcat] 2007-08-23 07:49:00 PDT
verified fixed 1.8.0.13 with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.13pre) Gecko/20070822 Firefox/1.5.0.13pre - no crash on test url
adding verified keyword
Comment 41 Bob Clary [:bc:] 2009-04-24 10:52:46 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/90a53b9435a4

Note You need to log in before you can comment on or make changes to this bug.