890179 - heap-buffer-overflow (write) at mozilla::image::nsPNGDecoder::row_callback

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Aki Helin]

Attachment: repro

A buffer overflow (write) occurs when the attached picture is opened in aurora channel asan build from a few days ago. 

$ opt/firefox-aurora-asan/firefox ff-bofw-rowcallback.png 2>&1 | symbolize | c++filt
=================================================================
==13655== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f60479d1b80 at pc 0x7f606009aeb4 bp 0x7fffd0325910 sp 0x7fffd0325908
WRITE of size 4 at 0x7f60479d1b80 thread T0
    #0 0x7f606009aeb3 in mozilla::image::nsPNGDecoder::row_callback(png_struct_def*, unsigned char*, unsigned int, int) /home/aki/src/mozilla-aurora/image/decoders/nsPNGDecoder.cpp:759
    #1 0x7f6063c1b52a in MOZ_PNG_push_have_row /home/aki/src/mozilla-aurora/media/libpng/pngpread.c:1460
    #2 0x7f6063c1aab9 in MOZ_PNG_proc_IDAT_data /home/aki/src/mozilla-aurora/media/libpng/pngpread.c:1131
0x7f60479d1b80 is located 0 bytes to the right of 224000-byte region [0x7f604799b080,0x7f60479d1b80)
allocated by thread T0 here:
    #0 0x43deca in posix_memalign ??:0
    #1 0x7f6063558fb9 in TryAllocAlignedBytes(unsigned long) /home/aki/src/mozilla-aurora/gfx/thebes/gfxImageSurface.cpp:89
    #2 0x7f606003b0a2 in imgFrame::Init(int, int, int, int, gfxASurface::gfxImageFormat, unsigned char) /home/aki/src/mozilla-aurora/image/src/imgFrame.cpp:198
    #3 0x7f606002114e in mozilla::image::RasterImage::InternalAddFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char, unsigned char**, unsigned int*, unsigned int**, unsigned int*, imgFrame**) /home/aki/src/mozilla-aurora/image/src/RasterImage.cpp:1283
    #4 0x7f6060021cd0 in mozilla::image::RasterImage::EnsureFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char, unsigned char**, unsigned int*, unsigned int**, unsigned int*, imgFrame**) /home/aki/src/mozilla-aurora/image/src/RasterImage.cpp:1412
    #5 0x7f6060021ee2 in mozilla::image::RasterImage::EnsureFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char**, unsigned int*, imgFrame**) /home/aki/src/mozilla-aurora/image/src/RasterImage.cpp:1467
    #6 0x7f606001bf2f in mozilla::image::RasterImage::InitDecoder(bool, bool) /home/aki/src/mozilla-aurora/image/src/RasterImage.cpp:2161
Shadow byte and word:
  0x1fec08f3a370: fa
  0x1fec08f3a370: fa fa fa fa fa fa fa fa
More shadow bytes:
  0x1fec08f3a350: 00 00 00 00 00 00 00 00
  0x1fec08f3a358: 00 00 00 00 00 00 00 00
  0x1fec08f3a360: 00 00 00 00 00 00 00 00
  0x1fec08f3a368: 00 00 00 00 00 00 00 00
=>0x1fec08f3a370: fa fa fa fa fa fa fa fa
  0x1fec08f3a378: fa fa fa fa fa fa fa fa
  0x1fec08f3a380: fa fa fa fa fa fa fa fa
  0x1fec08f3a388: fa fa fa fa fa fa fa fa
  0x1fec08f3a390: fa fa fa fa fa fa fa fa
Stats: 252M malloced (255M for red zones) by 333799 calls
Stats: 49M realloced by 17163 calls
Stats: 220M freed by 198590 calls
Stats: 83M really freed by 140106 calls
Stats: 440M (112722 full pages) mmaped in 110 calls
  mmaps   by size class: 8:212979; 9:32764; 10:12285; 11:16376; 12:2048; 13:1536; 14:1280; 15:384; 16:768; 17:1376; 18:48; 19:40; 20:20; 21:2;
  mallocs by size class: 8:248243; 9:43800; 10:11322; 11:21445; 12:2360; 13:1984; 14:1609; 15:461; 16:974; 17:1464; 18:72; 19:42; 20:21; 21:2;
  frees   by size class: 8:132541; 9:31130; 10:8119; 11:19784; 12:1559; 13:1323; 14:1412; 15:333; 16:826; 17:1443; 18:61; 19:38; 20:20; 21:1;
  rfrees  by size class: 8:101684; 9:16682; 10:5679; 11:13055; 12:765; 13:636; 14:455; 15:200; 16:613; 17:304; 18:28; 19:3; 20:1; 21:1;
Stats: malloc large: 1601 small slow: 2140
==13655== ABORTING

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase-mozilla-small

Hm, we hit that as well over the days but hadn't checked the logs yet. I will attach a smaller testcase.

Comment 2

[Christoph Diehl [:posidron]]

As for attachment 771247 [details] the following field got mutated as the crash occurred:

 LocateChunk-0.Chunk_IHDR.Core.Data.BitDepth

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

This looks like a single channel image?

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #3)
> This looks like a single channel image?

Never mind, gdb was messing with me.  It's a regular RGB.

Comment 5

[Joe Drew (not getting mail)]

Looks like our image size sniffing is wrong, and we never catch that. Easy enough to fix.

Comment 6

[Joe Drew (not getting mail)]

This was probably caused by bug 869125, which implies that this is in everything but release. Please update these flags if I'm wrong.

Comment 7

[Joe Drew (not getting mail)]

Attachment: check the actual frame rather than image metadata

Well, I stupidly put this in in bug 869125 by checking image metadata, which is created by the decoder, rather than the frame itself. This should fix it, though obviously we need to test. (It fixes the crash I see, though.)

https://tbpl.mozilla.org/?tree=Try&rev=c2c91d003a2d

Comment 8

[Seth Fowler [:seth] [:s2h]]

It was incredibly hard to review this since bugzilla.js shows thumbnails of attachments, which causes Firefox to immediately crash when you load this page. What fun.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 771464 [details] [diff] [review]
check the actual frame rather than image metadata

Review of attachment 771464 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 10

[Joe Drew (not getting mail)]

Yeah, I had to turn off bugzilla.js.

Comment 11

[Joe Drew (not getting mail)]

Comment on attachment 771464 [details] [diff] [review]
check the actual frame rather than image metadata

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

With a little bit of difficulty, though it's not *that* hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The check-in comment could.

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Bug 869125

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Easy.

How likely is this patch to cause regressions; how much testing does it need?

Not terribly likely to cause regressions.

Comment 12

[Joe Drew (not getting mail)]

Shoot. Forgot

Which older supported branches are affected by this flaw?

23, 24, 25.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

But 22 is clear then?

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 771464 [details] [diff] [review]
check the actual frame rather than image metadata

sec-approval+ after discussion with Dveditz. We should have branch patches made and nominated for after trunk checkin.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 771464 [details] [diff] [review]
check the actual frame rather than image metadata

Adding branch approval per discussion with Dveditz.

Comment 16

[Joe Drew (not getting mail)]

(In reply to Al Billings [:abillings] from comment #13)
> But 22 is clear then?

Yep

Comment 17

[Joe Drew (not getting mail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/50e3c3008b8d

Comment 18

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f257dd1c65ca
https://hg.mozilla.org/releases/mozilla-beta/rev/fe25b6cc38b4

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/50e3c3008b8d

Seems like this should have a test?

Comment 20

[Joe Drew (not getting mail)]

Definitely, but committing the testcase means we open the vulnerability up to basically anybody to exploit, so I'll leave that to security.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

In this situation, we create the test as a separate patch (sometimes even a cloned bug just for that) and then check it in six weeks after we ship the fix in the final release to the public.

We would love a checked in test though.