Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SECURITY: possible integer overflow with some cHRM chunk values #587

Closed
jbowler opened this issue Sep 11, 2024 · 16 comments
Closed

SECURITY: possible integer overflow with some cHRM chunk values #587

jbowler opened this issue Sep 11, 2024 · 16 comments

Comments

@jbowler
Copy link
Contributor

jbowler commented Sep 11, 2024

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
Copy link
Contributor Author

jbowler commented Sep 11, 2024

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
Copy link
Contributor Author

jbowler commented Sep 11, 2024

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
Copy link
Contributor Author

jbowler commented Sep 11, 2024

Note that png_xy_from_XYZ did not change in 20f819c - 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
Copy link
Contributor Author

jbowler commented Sep 11, 2024

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
Copy link
Contributor Author

jbowler commented Sep 11, 2024

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
Copy link
Member

ctruta commented Sep 12, 2024

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
Copy link

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

@jbowler
Copy link
Contributor Author

jbowler commented Sep 12, 2024

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
Copy link
Contributor Author

jbowler commented Sep 13, 2024

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 [email protected].

tdf-gerrit pushed a commit to LibreOffice/core that referenced this issue Sep 15, 2024
got a case similar to

pnggroup/libpng#587

with a backtrace of:

/work/workdir/UnpackedTarball/libpng/png.c:1475:23: runtime error: signed integer overflow: -1703155269 - 692774662 cannot be represented in type 'png_fixed_point' (aka 'int')
 #0 0x59bbf901eab0 in png_XYZ_from_xy /work/workdir/UnpackedTarball/libpng/png.c:1475:23
 #1 0x59bbf901eab0 in png_colorspace_check_xy /work/workdir/UnpackedTarball/libpng/png.c:1610:13
 #2 0x59bbf901d8bc in png_colorspace_set_chromaticities /work/workdir/UnpackedTarball/libpng/png.c:1717:12
 #3 0x59bbf9046855 in png_handle_cHRM /work/workdir/UnpackedTarball/libpng/pngrutil.c:1302:10
 #4 0x59bbf902d064 in png_read_info /work/workdir/UnpackedTarball/libpng/pngread.c:175:10
 #5 0x59bbf7c331d6 in (anonymous namespace)::reader(SvStream&, Graphic&, GraphicFilterImportFlags, BitmapScopedWriteAccess*, BitmapScopedWriteAccess*) /src/libreoffice/vcl/source/filter/png/PngImageReader.cxx:404:5
 #6 0x59bbf7c36960 in read /src/libreoffice/vcl/source/filter/png/PngImageReader.cxx:845:55
 #7 0x59bbf7c36960 in vcl::PngImageReader::read() /src/libreoffice/vcl/source/filter/png/PngImageReader.cxx:850:5
 #8 0x59bbf786fe57 in LLVMFuzzerTestOneInput /src/libreoffice/vcl/workben/pngfuzzer.cxx:52:19

(gdb) print *xy
$1 = {
  redx = 9,
  redy = 131616,
  greenx = 598048,
  greeny = 538976288,
  bluex = 0,
  bluey = 151551,
  whitex = 538976288,
  whitey = 538976288
}

but not reproducible with a typical utility because we're unusually
ignoring crc errors for fuzzing so reenable those and see if a testcase
can be generated anyway.

Change-Id: Ifc050ee082800906b087609154ec29ca39cd8fe6
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/173409
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <[email protected]>
@jbowler
Copy link
Contributor Author

jbowler commented Sep 16, 2024

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
Copy link
Contributor Author

jbowler commented Sep 17, 2024

See #597

@bobfriesenhahn
Copy link
Contributor

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:

clusterfuzz-testcase-coder_PNG_fuzzer-5430379631149056
clusterfuzz-testcase-minimized-coder_PNG_fuzzer-5430379631149056

@jbowler
Copy link
Contributor Author

jbowler commented Sep 25, 2024

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
Copy link
Contributor Author

jbowler commented Oct 13, 2024

#621 (#620 in libpng18) is another instance.

@jbowler
Copy link
Contributor Author

jbowler commented Oct 19, 2024

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

@jbowler jbowler closed this as completed Oct 19, 2024
@bobfriesenhahn
Copy link
Contributor

bobfriesenhahn commented Oct 20, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants