<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Comment 1

•

13 years ago

The report doesn't say but it looks as though (because of the MOZ_ prefixes) reporter was using the bundled libpng. pngcrush indicates that the image is an interlaced PNG with only IHDR, IDAT, and IEND chunks.

Assignee: nobody → glennrp+bmo

Status: NEW → ASSIGNED

Comment 2

•

13 years ago

pngcheck doesn't show anything interesting about the file: pngcheck -v -v test.png File: testff.png (231 bytes) chunk IHDR at offset 0x0000c, length 13 45 x 45 image, 24-bit RGB, interlaced chunk IDAT at offset 0x00025, length 174 zlib: deflated, 32K window, maximum compression rows per pass: 6, 6, 6, 12, 11, 23, 22 row filters (0 none, 1 sub, 2 up, 3 avg, 4 paeth): 1 2 4 2 4 2 | 1 2 4 2 4 2 | 1 2 4 2 4 2 | 1 2 2 2 4 2 2 2 4 2 2 2 | 1 2 2 2 4 2 2 4 2 2 2 | 1 2 2 2 2 2 2 2 4 2 2 2 2 2 2 4 2 2 2 2 2 2 2 | 1 2 2 2 2 2 2 4 2 2 2 2 2 2 2 4 2 2 2 2 2 2 (86 out of 86) chunk IEND at offset 0x000df, length 0 No errors detected in test.png (3 chunks, 96.2% compression).

Comment 3

•

13 years ago

Line 3361 of pngrutil.c is in a block of code that was revised in libpng-1.5.6 to handle aligned memory. Someone please try putting "#define PNG_ALIGN_TYPE 0" in mozpngconf.h and see if the problem still exists.

Reporter

Comment 4

•

13 years ago

(In reply to Glenn Randers-Pehrson from comment #1) > The report doesn't say but it looks as though (because of the MOZ_ prefixes) > reporter was using the bundled libpng. pngcrush indicates that the image is > an interlaced PNG with only IHDR, IDAT, and IEND chunks. Yes, i am using firefox's bundled libpng. Also, is evident from the src location in crash stack - src/media/libpng/pngrutil.c:3361 (In reply to Glenn Randers-Pehrson from comment #3) > Line 3361 of pngrutil.c is in a block of code that was revised in > libpng-1.5.6 to handle aligned memory. Someone please try putting "#define > PNG_ALIGN_TYPE 0" in > mozpngconf.h and see if the problem still exists. Adding "#define PNG_ALIGN_TYPE 0" fixes the crash.

Comment 5

•

13 years ago

Thank you. Please try changing the two "while" statements to the following: line 3343: while (c >= sizeof(png_uint_32)); and line 3384: while (c >= sizeof(png_uint_16)); (and remove the "#define PNG_ALIGN_TYPE 0"). If that works I'll prepare a proper patch, and apply it to upstream libpng as well.

Comment 6

•

13 years ago

(In reply to Glenn Randers-Pehrson from comment #5) > Thank you. Please try changing the two "while" > statements to the following: I'm looking at the 1.5.13 sources, and I don't see anything matching this?

Comment 7

•

13 years ago

In libpng-1.5.13 the lines are 3165 and 3206. Both are while (c > 0); In the mozilla embedded libpng they are pushed down by a couple hundred lines of APNG supporting code.

Comment 8

•

13 years ago

Oh, I get the point now: if the number of bytes to copy isn't a multiple of the copy-word size, you copy one full word too many. Ooops.

Comment 9

•

13 years ago

Oh, no, scratch that --- c is size_t, so the test is effectively (c != 0). So it'll loop till core dump. Ugh.

Comment 10

•

13 years ago

Right, the "while()" continues if the unsigned "c" gets decremented past zero.

Comment 11

•

13 years ago

Libpng-1.5.14 is due out on January 24th. libpng-1.5.14rc01 will be out on the 10th, without the patch. I'll apply the patch to libpng-1.5.14 as it's released, so I suggest that this bug be held private until the 24th. I'm assuming that the patch I described in comment #5 will fix the problem.

Comment 12

•

13 years ago

(In reply to Glenn Randers-Pehrson from comment #10) > Right, the "while()" continues if the unsigned "c" gets decremented past zero. But wait --- doesn't the initial alignment test prevent that case? I went through the code again, and I don't see where it's not an accurate reimplementation of the memcpy version.

Comment 13

•

13 years ago

I am told that the "while" statements are OK, due to the intial checking. Instead, the problem seems to be a failure to decrement row_width. We need to insert row_width-=sizeof(png_uint_32); and row_width-=sizeof(png_uint_16); inside the two while() loops, respectively.

Comment 14

•

13 years ago

I've been unable to reproduce the buggy behavior with libpng-1.5.13 in another application (ImageMagick, modified to use the "blocky" de-interlacing), running under Ubuntu on a 64-bit workstation. Debug printout shows there's no runaway "while" loop when decoding the test image.

Reporter

Comment 15

•

13 years ago

Based on last comments, I did the patch, but it does not fix the problem. diff -r 6955309291ee media/libpng/pngrutil.c --- a/media/libpng/pngrutil.c Wed Jan 02 19:35:43 2013 -0800 +++ b/media/libpng/pngrutil.c Thu Jan 03 18:11:11 2013 -0800 @@ -3339,6 +3339,7 @@ { *dp32++ = *sp32++; c -= sizeof (png_uint_32); + row_width -= sizeof(png_uint_32); } while (c > 0); @@ -3380,6 +3381,7 @@ { *dp16++ = *sp16++; c -= sizeof (png_uint_16); + row_width -= sizeof(png_uint_16); } while (c > 0);

Comment 16

•

13 years ago

Attached image Test file, without PAETH filtering — Details

Additional test file, without any PAETH filtering. Does this also exhibit the bug?

Reporter

Comment 17

•

13 years ago

(In reply to Glenn Randers-Pehrson from comment #16) > Created attachment 697782 [details] > Test file, without PAETH filtering > > Additional test file, without any PAETH filtering. Does this also exhibit > the bug? Yes, this testcase also crashes.

Comment 18

•

13 years ago

I'm not seeing this bug with the installed Firefox 17.0.1 on my 64-bit Ubuntu platform, nor with the current "nightly" distribution of Firefox 20.0a1 (2013-01-04). I'm running out of ideas. Should we check in a patch to "#define PNG_ALIGN_TYPE 0" in mozpngconf.h to work around the bug (with perhaps a performance penalty) temporarily? A libpng developer suggests maybe it's an optimization problem (try building with pngrutil.c compiled without optimization).

Reporter

Comment 19

•

13 years ago

(In reply to Glenn Randers-Pehrson from comment #18) > I'm not seeing this bug with the installed Firefox 17.0.1 on my 64-bit > Ubuntu platform, nor with the current "nightly" distribution of Firefox > 20.0a1 (2013-01-04). I'm running out of ideas. Should we check in a patch > to "#define PNG_ALIGN_TYPE 0" in mozpngconf.h to work around the bug (with > perhaps a performance penalty) temporarily? This is Mozilla's security team decision. > > A libpng developer suggests maybe it's an optimization problem (try building > with pngrutil.c compiled without optimization). I does look like it. The bug only reproduces in clang -O2 optimization, but does not reproduce with -O1. Steps to build ASAN here https://developer.mozilla.org/en-US/docs/Building_Firefox_with_Address_Sanitizer, clang revision i am using is 171202

Comment 20

•

13 years ago

Does Firefox, built with "clang -O2" but without asan, exhibit a problem?

Assignee: glennrp+bmo → nobody

Comment 21

•

13 years ago

Also, can you verify that the bug still exists if you drop back to clang-3.2?

Reporter

Comment 22

•

13 years ago

(In reply to Glenn Randers-Pehrson from comment #20) > Does Firefox, built with "clang -O2" but without asan, exhibit a problem? This is filed as a security vulnerability, so it does not matter whether it reproduces as a crash in a regular clang build. Many of the OOB reads/writes won't crash in a regular build, but they still matter because of the security implications.

Al Billings [:abillings - ex-MoCo]

Comment 23

•

13 years ago

At this point there is no very good reason to believe there is any libpng bug here at all --- what seems substantially more likely is that we're looking at a code-generation bug in clang 3.3. The argument in comment #22 is therefore off-point.

Updated

•

13 years ago

Status: ASSIGNED → NEW

Andrew McCreight [:mccr8]

Comment 24

•

13 years ago

Rafael, could you possibly look into this a little to evaluate whether it is a Clang problem, and file a bug or whatever as appropriate? Thanks.

Rafael Ávila de Espíndola (:espindola) (not reading bugmail)

Comment 25

•

13 years ago

(In reply to Andrew McCreight [:mccr8] from comment #24) > Rafael, could you possibly look into this a little to evaluate whether it is > a Clang problem, and file a bug or whatever as appropriate? Thanks. I can try. If I understand this correctly, it only reproduces with an asan build using clang trunk. Is that correct? Is this linux or OS X? You suspect a clang bug because a png bug would manifest in an infinite loop writing to memory, not just an off by one access, correct? Taras, what do you consider the relative priority of this bug to be?

Josh Aas

Comment 26

•

13 years ago

Marking the sec-high. This would normally be sec-critical, but it's mitigated by the fact that we think it only happens with a compiler we don't normally use (if I understand correctly).

Keywords: sec-high

(dormant account)

Comment 27

•

13 years ago

(In reply to Josh Aas (Mozilla Corporation) from comment #26) > Marking the sec-high. This would normally be sec-critical, but it's > mitigated by the fact that we think it only happens with a compiler we don't > normally use (if I understand correctly). Rafael, it's high priority to determine if this is a clang bug and if we should take steps to mitigate this if this may cause problems in (non-asan) mac builds

Rafael Ávila de Espíndola (:espindola) (not reading bugmail)

Comment 28

•

13 years ago

I cannot reproduce this. I have built clang 171202 and used that to build firefox on linux x86-64 using browser/config/mozconfigs/linux64/nightly-asan. I then ran ASAN_OPTIONS=redzone=64,alloc_dealloc_mismatch=0 ./obj-x86_64-unknown-linux-gnu/dist/bin/firefox I can then open both png files in this bug without crashing. I have built mozilla-central revision 1eab3e0d7f53.

Rafael Ávila de Espíndola (:espindola) (not reading bugmail)

Comment 29

•

13 years ago

Abhishek tells me this doesn't reproduce with clang 174592. It might have been fixed or at least the bug is hidden for now.

Status: NEW → RESOLVED

Closed: 13 years ago

Resolution: --- → FIXED

Daniel Veditz [:dveditz]

Comment 30

•

13 years ago

We don't ship ESR-17 or b2g18 releases built with clang so we don't need to update any of our toolchains for those branches.

status-b2g18: --- → unaffected

status-firefox-esr17: --- → unaffected

Daniel Veditz [:dveditz]

Comment 31

•

13 years ago

Compiler bugs are generally out of scope for the bounty.

Flags: sec-bounty? → sec-bounty-

Keywords: sec-high → sec-vector

Summary: Heap-buffer-overflow in MOZ_PNG_combine_row → Out of bound read in MOZ_PNG_combine_row

Whiteboard: [asan] → [asan] clang optimization bug?