Could someone please take a look this issue?.
Thank you.

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=6190967209328640

Let's see if it reproduces with ASan. Most likely it's just a header for an image with big resolution, not a bug.

The image only reports a size of 32 x 32, but it claims to have a text block (tEXt) with 2147477350 bytes in it. (The file is truncated before reaching that many bytes.)

The stack trace does not look very useful (I'm guessing because this is a Release build), so I don't know know where that memory is being allocated, but the first thing I notice is that the CRC for the IHDR chunk is incorrect. As a result, our decoder should exit before it ever reaches the tEXt block (since IHDR comes first). And ours (i.e. PNGImageDecoder/png_codec) does, but the one being used for fuzzing [1] calls

  set_crc_action(png_ptr, PNG_CRC_QUIET_USE, PNG_CRC_QUIET_USE);

which ignores the CRC error. But with a slightly modified file (to correct the CRC), there still might be a problem with our decoders, if they try to allocate enough space for this tEXt block. My guess is that they do not (since we never attempt to do anything with tEXt blocks), but it's possible libpng is doing it for us. (I have not yet looked into this.)

The fuzzer uses a completely different API from the one used by our decoders, though. It calls

  png_start_read_image

and

  png_read_row

whereas Chromium/Blink use

  png_set_progressive_read_fn

with callbacks. Not sure whether the two entry points handle tEXt differently.

[1] https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc?type=cs

scroggo@, thanks for the detailed analysis! CRC has been disabled intentionally, otherwise fuzzing would not make a lot of sense as it will (almost) always bail out due to incorrect CRC.

I don't think that it's possible to update this file with a correct CRC, as CRC comes only after data, so we have to make a 2GB file to add a correct CRC. However I wonder how libpng handles that: does it exit with an error, or maybe there is an overflow in that case? :)

$ cat 5620496911826944 | hexdump -C
00000000  89 50 4e 47 0d 0a 1a 0a  00 00 00 0d 49 48 44 52  |.PNG........IHDR|
00000010  00 00 00 20 00 00 00 20  10 02 00 00 00 ac 41 54  |... ... ......AT|
00000020  78 7f ff e7 66 74 45 58  74 00 8d 02 49 44 09 54  |x...ftEXt...ID.T|
00000030  67 49 11                                          |gI.|
00000033


hm, the size of tEXt chunk seems to be 0x008d0249 = 9,241,161 bytes.


ASan uses 271 MB to process this testcase. With enabled CRC check, it uses only 14.

I think we can disable some boring chunk types for MSan build:

(3) Don't decode unused chunks. Persons building any version of libpng can cause applications to ignore particular ancillary chunks by defining PNG_NO_* macros, e.g.,

  #define PNG_NO_READ_iCCP
  #define PNG_NO_READ_TEXT /* disables tEXt, zTXt, and iTXt */

On a separate note, scroggo@, would you be interested in writing a new fuzz target that uses png_set_progressive_read_fn instead of png_read_row? This would be super useful for both Chrome and for OSS-Fuzz (v) project :)

> CRC has been disabled intentionally, otherwise fuzzing would not make a
> lot of sense as it will (almost) always bail out due to incorrect CRC.

That makes sense. I was just thinking out loud as I tried to figure out why this obviously broken file was getting as far as it did.

> I don't think that it's possible to update this file with a correct
> CRC, as CRC comes only after data, so we have to make a 2GB file to
> add a correct CRC.

Right, there is not even enough data to reach the CRC for the tEXt chunk. But it's the CRC for the IHDR chunk that is wrong. It appears 29 bytes into the 51 byte file. This is where our decoders will abort. But if the IHDR's CRC chunk was correct, we could still be vulnerable - but PNGImageDecoder and png_codec both use png_set_progressive_read_fn, which does not attempt to allocate the size of the chunk, the way png_read_info does, so this is not a problem for our decoders.

> However I wonder how libpng handles that: does it exit with an error,
> or maybe there is an overflow in that case? :)

If png_malloc_base retuns NULL, libpng signals a warning, which the client can treat as an error or ignore. (A client can also set limits and/or supply its own malloc function.

> On a separate note, scroggo@, would you be interested in writing a new
> fuzz target that uses png_set_progressive_read_fn instead of png_read_row?
> This would be super useful for both Chrome and for OSS-Fuzz (v) project :)

Filed  issue 697101 .

FWIW, we *also* have a fuzzer for PNGImageDecoder [1]. I'm not aware of one for png_codec.

[1] https://chromium.googlesource.com/chromium/src/+/1cc893851ef9ad007f4fb1eaabf58870fc634e81/third_party/WebKit/Source/platform/PngFuzzer.cpp

(Here is the one for png_codec/PNGCodec: https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/gfx_png_image_fuzzer.cc)

Issue 691695 has been merged into this issue.

@mmoroz -- Could you please look into the issue and respond according to Comment# 9 and update the issue.
Thank You.

I would like to learn some internals of libpng, but we have another two hundreds of fuzz targets in Chromium. To make it possible to scale our fuzzing efforts, maintaining of fuzz targets and fixing of bugs usually is being done by OWNERS of corresponding projects. Of course, from time to time we (security team) also fix bugs, but this won't scale if we try to fix every issue we find :)

Leon, let me assign this to you, since you are an owner of libpng and know it much better than me.

Similar to https://bugs.chromium.org/p/chromium/issues/detail?id=681497 and https://bugs.chromium.org/p/chromium/issues/detail?id=701957, this OOM is not a bug.

Large images with corrupt/large chunks will cause libpng to allocate a lot of memory.

I've previously suggested not filing bugs in these cases - though mmoroz@ has indicated that this is not a reasonable option for the fuzzer.

Nobody is saying that this is a bug in libpng. This is a bug in the fuzz target, so I've tried to explicitly point this out in c#14:

"maintaining of fuzz targets and ... usually is being done by OWNERS of corresponding projects."

I guess that you may be not impressed by fuzz testing approach, since we haven't reported really dangerous bugs in libpng so far. However, fuzz testing has proven to be quite efficient, and we've reported lots of scary bugs in other projects. One of the possible reasons why we haven't found anything in libpng yet, is having problems like this issue blocking the fuzz target from being efficient. For example, this OOM error happens in 78% of runs (https://clusterfuzz.com/v2/performance-report/libfuzzer_libpng_read_fuzzer/libfuzzer_chrome_msan/2017-03-29).

Obviously, I'm not in a position to require something from you guys, so I'm kindly asking to help to block OOMs either in fuzz target code or via other means (e.g. re-defining limits, blocking some chunk types, etc).

We've found ways to mitigate previous issues of a similar type (e.g. https://codereview.chromium.org/2377293002/ or https://codereview.chromium.org/1808123004/), but this one seems to be a more tricky one. Your help would be appreciated and would have an impact on making Chrome more stable and more secure.

Sorry if #15 came off the wrong way.  It was meant to be informative - not at all to detract from our use fuzzers.

As someone who has spent a good amount of time working on codecs, I can attest to the many times fuzzers have caught real and dangerous bugs.  I think it is critical that we have fuzzers for our decoding libraries and I am glad that we are fuzzing libpng.

This particular style of bug is not dangerous, which is why I marked as Won't Fix.  libpng allocates a lot of memory because a corrupt chunk told it to.  There is an approach to "fix" this - we can control the size of the chunks we are willing to allocate.  Out current choices are:
#define PNG_USER_CHUNK_CACHE_MAX 128
#define PNG_USER_CHUNK_MALLOC_MAX 4000000L
https://bugs.chromium.org/p/chromium/issues/detail?id=117369

These are arbitrary choices and not necessarily "correct".  But I also don't think it makes sense to choose our global compile constants based on the fuzzers.  Perhaps we could consider compiling libpng with a lower limit for the fuzzer?  On MSAN builds?

I think the suggestion in #7 is interesting, but we probably *do* want the fuzzer to parse all of the chunks that we are parsing in the real build.

If this bug was meant to be tracking, "let's fix libpng fuzzer from filing non-dangerous OOM bugs" then I'm sorry for closing.  There is an opportunity for improvement there.

The approach I was taking (maybe this is wrong) was to simply close non-dangerous fuzzer bugs - and of course to fix the real ones when they come up.

Yeah, this issue is definitely not a dangerous one, this is just a blocker.

If I understand the limits correctly, they give 512 MBs in total. MSan overhead should be 2x. We use rss_limit_mb of 2GB, so I'm not sure that lowering these limits for MSan builds would help. I've also tried it locally:

A) redefine the constants in pnglibconf.h 

B) call png_set_chunk_cache_max and png_set_chunk_malloc_max in https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc?l=55

Neither of these solutions resolved the error :(

>> Perhaps we could consider compiling libpng with a lower limit for the fuzzer?  On MSAN builds?

Yes! If we have a solution like limits change, we need it only for fuzzing builds. For this particular issue, we can use a more precise condition and change limits only for MSan, not for all fuzzing builds. This can be implemented via '#ifdef MEMORY_SANITIZER' in code or 'if (is_msan) { ... }' in GN.

Let me rename this bug and re-open it. I believe that we also can lower the priority.

Thanks mmoroz@, sgtm.

> Neither of these solutions resolved the error :(

In version 1.6.25, libpng added a check for a large ICC chunk based on png_set_chunk_malloc_max, and Chromium is still on an older version (1.6.22). So if we were to upgrade, I suspect (B) would work, and possibly (A) (though I suspect A would be messy).

>> Perhaps we could consider compiling libpng with a lower
>> limit for the fuzzer?  On MSAN builds?

> Yes! If we have a solution like limits change, we need it only for
> fuzzing builds. For this particular issue, we can use a more
> precise condition and change limits only for MSan, not for all
> fuzzing builds. This can be implemented via '#ifdef MEMORY_SANITIZER'
> in code or 'if (is_msan) { ... }' in GN.

I've uploaded a CL at https://codereview.chromium.org/2813693002 that uses a custom allocator.

It looks like mmoroz@ is OOO. Can someone else take a look at https://codereview.chromium.org/2813693002 ? I still need an owners approval. kcc@ or inferno@?

> In version 1.6.25, libpng added a check for a large ICC chunk based on
> png_set_chunk_malloc_max, and Chromium is still on an older version (1.6.22).
> So if we were to upgrade

Here I suggested that upgrading could be part of the solution, but didn't address why we are not:

Though we'll likely upgrade at some point, this is not the reason to do so. And upgrading can be time-consuming. Further, it would fix this particular problem, but there may be other parts of libpng that it does not fix.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/645a85acd14bdef0471070b9f928b0c29c3d3dae

commit 645a85acd14bdef0471070b9f928b0c29c3d3dae
Author: scroggo <scroggo@chromium.org>
Date: Mon Apr 17 19:20:51 2017

Stop reporting OOM as errors in libpng fuzzers

Use a custom allocator to make malloc fail beyond an arbitrary max.
Set the maximum to match the default png_user_chunk_malloc_max.

BUG= 673082 

Review-Url: https://codereview.chromium.org/2813693002
Cr-Commit-Position: refs/heads/master@{#464972}

[modify] https://crrev.com/645a85acd14bdef0471070b9f928b0c29c3d3dae/testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc

ClusterFuzz has detected this issue as fixed in range 464964:465012.

Detailed report: https://clusterfuzz.com/testcase?key=5620496911826944

Fuzzer: libfuzzer_libpng_read_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Out-of-memory (exceeds 2048 MB)
Crash Address: 
Crash State:
  libpng_read_fuzzer
  
Sanitizer: memory (MSAN)

Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=464964:465012

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv94KZ0GNGLV529vSEGv0RjlNWSN_GGfF04gfuztCMe28W5np6XCmV-fJkXm7L9Qy9rNktfdIN9JHcHqx4JCczAjBUFvx8ZpfRcktovaunau2H9TBrd-QmT9ZZSDqrmwsI15-TG8Q3Ea9VYHeiShJKYTGxLcJwWRap2ujLRvxVzOJSQH8G0h_YPK3roFC4RdZrpuMRs86XqufNJC78mcunJTTkOAckjp3JtPOsNstGI2TlEx3QJIkxhPVhqXOD_MRs05Sm0MAFakPljFF39ENcN5Z1WPl9yZMGLkmfQSBYn26nWZ7QZsKQ6KmcJezRTemRAm5pnrhDLo0Zxcf-T0JTLM5enhYdN7kBoo3Bs3Bd2eBF66Z9tA?testcase_id=5620496911826944


See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.