SECURITY: possible integer overflow with some cHRM chunk values

[jbowler]

New issue 71387 by ClusterFuzz-External: libpng:libpng_read_fuzzer: Integer-overflow in png_xy_from_XYZ

@ctruta: I asked them to raise a bug here. They sent you the details too. Probably a side effect of the ACEScg support but there is no issue at present (since 1.6.44 has not been released). I do need to know the actual cHRM values.

[jbowler]

Note that this is unlikely to be an issue on any real world machine (ARM, AMD64) because they do not fault on integer overflow, but I thought the code was catching it anyway. Maybe it checks after the overflow. Anyway it can easily be converted to unsigned; the overflow doesn't matter because it's garbage-in, garbage-out.

[jbowler]

It would, however, be helpful to PNG_DEPRECATE the following APIs in libpng 1.6:

png_get_cHRM_XYZ png_get_cHRM_XYZ_fixed png_set_cHRM_XYZ png_set_cHRM_XYZ_fixed

They are all "utilities" and, per Marti Maria's comments and other comments, it doesn't seem that anyone really needs them (so deprecated until we see). PNG_DEPRECATED seems to have support for clang, gcc and vc

[jbowler]

Note that png_xy_from_XYZ did not change in 20f819c2 - the change is in png_XYZ_from_xy - and that corresponds to only one of the two calls to png_xy_from_XYZ but png_XYZ_normalize seems to have a similar error.

I need to know the call stack and the XYZ values but the fix is almost certainly the code in png_XYZ_normalize; i.e. how the addition of Y is performed, in place of the simple additions in png_xy_from_XYZ at lines 1216, 1225, 1234. I'll move the png_XYZ_normalize stuff to an appropriate static and just call it.

[jbowler]

Assuming the problem does come from #579 #588 should fix it but since I haven't been given the parameters, i.e. the chromaticities which cause the problem (there may be none) or a repro (it may involve calling png_set_cHRM_XYZ) I have no way of testing this.

mbechard could you verify that this doesn't break anything in your test cases? @fuzzers, the problem always existed PR#579 just made it accessible via the API. I don't think a cHRM chunk will trigger it but it is conceivable that some variety of rounding in flxed point multiplication might do so. It would require a very carefully tuned cHRM chunk so for that reason I really would like you to disclose the repo.

Note that in absence of information I'm assuming the cause was #579 rather than some very careful crafted numbers. It's not clear what version of libpng was being tested but the problem only exists in the as yet unreleased libpng-1.6.44 (I assume it isn't released; there is no tag).

@ctruta - mostly issue but technically it's invalid in ANSI-C to overflow in addition, always assuming this is the bug.

[jbowler]

While this issue is being discussed I'm in favour of moving to floating point arithmetic internally and eliminating all of the fixed-point stuff including deprecating all the _fixed APIs. Some processors, particular IoT ones, still don't have floating point units but they also don't have enough RAM to run an unconfigured libpng. The default APIs have always used fp (double) parameters and the internal implementations, like this one, have only used fixed point instead to avoid writing the same code twice (i.e. fixed arithmetic was always possible so it was used.)

[ctruta]

It would, however, be helpful to PNG_DEPRECATE the following APIs in libpng 1.6:

png_get_cHRM_XYZ png_get_cHRM_XYZ_fixed png_set_cHRM_XYZ png_set_cHRM_XYZ_fixed

They are all "utilities" and, per Marti Maria's comments and other comments, it doesn't seem that anyone really needs them (so deprecated until we see). PNG_DEPRECATED seems to have support for clang, gcc and vc

👍

[mbechard]

Yes, the ACEScg test I have still works with this patch instead of the other.

[jbowler]

Thanks, that's good to know. I'm pretty sure it's correct but unfortunately it's yet another case where libpng needs test cases.

[jbowler]

Incidentally I am unable to find a way to access the "detailed report" or, indeed, anything; the email was sent to a valid email of mine the links required me to create a Google account (or use one I already have, do I look like a complete fool?) So I still don't have the faintest idea what they are whining about.

The email I received was from an entity named "ClusterFuzz" [sic: my apologies but that is what it is]. Despite all the warning signs it looks genuine but I can't tell for sure. If it is genuine I created the far more acceptable ClusterFuzy@gmail.com.

[jbowler]

Possible test case:

redx = 9, redy = 131616, greenx = 598048, greeny = 538976288, bluex = 0, bluey = 151551, whitex = 538976288, whitey = 538976288

From https://github.com/tdf-gerrit above.

The assumption in the source code about the range is invalidated by the change. Probably better to do this in floating point in libpng 1.8 and just check for division by 0. This can be handled most of the time but 0/0 is a signaling is undefined.

[jbowler]

See #597

[bobfriesenhahn]

GraphicsMagick oss-fuzz issue 368060014 ("Integer-overflow · png_colorspace_check_xy") seems to match this bug. Although it is described as being a security issue, it does not appear to be one so I will post the test cases here.

These are the two test cases which were produced:

[jbowler]

Thanks; the more test cases the better since pngstest can easily test them.

This problem was fixed by #578 and that is in 1.6.45 however there was an error in the fix (or the fix was insufficient) and that caused the same issue (broadly) elsewhere.

597 reintroduces the range checks without reintroducing the original bug, I'm working on a different solution for 1.8 which just uses floating point (almost infinitely easier to get right and overflows typically don't signal anyway.) I'm still waiting for #597 to be merged so 1.6.45 itself is still broken.

It's not a security issue unless libpng is run on hardware which faults integer overflow. I can't remember whether any such hardware still exists IRL (there was some such hardware in the 1980s.)

[jbowler]

621 (#620 in libpng18) is another instance.

[jbowler]

Fuzzed to death. Does anyone, including hackers of all nationalities, care? It's no exploitable.

[bobfriesenhahn]

The problem is that automated fuzzing systems like oss-fuzz do not support human input to analyze and dismiss issues. Instead, all issues are equally important and can only be eliminated by changing code. An issue found by one system is likely to be found by another.

Bob

On Oct 19, 2024, 5:47 PM, at 5:47 PM, John Bowler @.***> wrote:

Fuzzed to death. Does anyone, including hackers of all nationalities, care? It's no exploitable.

-- Reply to this email directly or view it on GitHub: https://github.com/pnggroup/libpng/issues/587#issuecomment-2424295816 You are receiving this because you commented.

Message ID: @.***>