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

RESOLVED FIXED in Firefox 23

Status

()

--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aki.helin, Assigned: joe)

Tracking

(4 keywords)

Trunk
mozilla25
x86_64
All
crash, csectype-bounds, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox22 unaffected, firefox23+ fixed, firefox24+ fixed, firefox25+ fixed, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected)

Details

(Whiteboard: [asan][adv-main23-])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 771193 [details]
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
Keywords: crash, csec-bounds, sec-critical, testcase
Created attachment 771247 [details]
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.
OS: Linux → All
Version: 24 Branch → Trunk
As for attachment 771247 [details] the following field got mutated as the crash occurred:

 LocateChunk-0.Chunk_IHDR.Core.Data.BitDepth
This looks like a single channel image?
(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.
(Assignee)

Comment 5

5 years ago
Looks like our image size sniffing is wrong, and we never catch that. Easy enough to fix.
Assignee: nobody → joe
(Assignee)

Comment 6

5 years ago
This was probably caused by bug 869125, which implies that this is in everything but release. Please update these flags if I'm wrong.
Blocks: 869125
status-firefox22: --- → unaffected
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
(Assignee)

Comment 7

5 years ago
Created attachment 771464 [details] [diff] [review]
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
Attachment #771464 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #771464 - Flags: review? → review?(seth)
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 on attachment 771464 [details] [diff] [review]
check the actual frame rather than image metadata

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

Nice.
Attachment #771464 - Flags: review?(seth) → review+
(Assignee)

Comment 10

5 years ago
Yeah, I had to turn off bugzilla.js.
(Assignee)

Comment 11

5 years ago
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.
Attachment #771464 - Flags: sec-approval?
(Assignee)

Comment 12

5 years ago
Shoot. Forgot

Which older supported branches are affected by this flaw?

23, 24, 25.
But 22 is clear then?
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.
Attachment #771464 - Flags: sec-approval? → sec-approval+
Comment on attachment 771464 [details] [diff] [review]
check the actual frame rather than image metadata

Adding branch approval per discussion with Dveditz.
Attachment #771464 - Flags: approval-mozilla-beta+
Attachment #771464 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 16

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

Yep
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/f257dd1c65ca
https://hg.mozilla.org/releases/mozilla-beta/rev/fe25b6cc38b4
status-firefox23: affected → fixed
status-firefox24: affected → fixed
status-firefox25: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/50e3c3008b8d

Seems like this should have a test?
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Comment 20

5 years ago
Definitely, but committing the testcase means we open the vulnerability up to basically anybody to exploit, so I'll leave that to security.
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.

Updated

5 years ago
tracking-firefox23: ? → +
tracking-firefox24: ? → +
tracking-firefox25: ? → +
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main23-]
Whiteboard: [adv-main23-] → [asan][adv-main23-]
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → unaffected
Group: core-security
You need to log in before you can comment on or make changes to this bug.