SECURITY [libpng16 ONLY] bug fix: More corrections for cHRM checks #597
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbowler
referenced
this pull request
in LibreOffice/core
Sep 17, 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
force-pushed
the
libpng16-ACES-AP0
branch
from
9029345 to
25d3705
Compare
jbowler
changed the title
jbowler
changed the title
|
@ctruta: VERY important; if you check this in to "master" or "libpng18" it will completely break my attempts to fix the current accuracy problems. |
|
@jbowler if you're telling me not to check it in, then ok, I won't check it in. |
@jbowler I don't know if you can tell from my previous comment, but I'm confused, and even more so after I read this #587 (comment) Did you mean to imply that I SHOULD check this in (both in libpng16 and libpng18)? Or that I SHOULD NOT check it in? |
|
Libpng 16 only.
I'm working on something better for 18. It's too large a change to 16
EDIT: that was sent from a cellphone, hence the abbreviation.
This PR needs to go in to libpng16 but should not go into libpng18.
|
The subtracts in PNG_XYZ_from_xy are producing integer overflow with some valid but extreme xy values. This re-introduces the previous checks but with less limited bounds; sufficient to accomodate the ACEScg end points (ACES AP1) but not for the ACES AP0 end points. Those were not working anyway because libpng reads the cHRM parameters as unsigned values so they must always be at least 0. A better solution requires recognizing reasonable negative values (ones which violate the current spec) and allowing them too, at least on read. Signed-off-by: John Bowler <[email protected]>
jbowler
force-pushed
the
libpng16-ACES-AP0
branch
from
25d3705 to
baaf722
Compare
ctruta
approved these changes
Sep 27, 2024
|
Integrated in branch |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The subtracts in PNG_XYZ_from_xy are producing integer overflow with
some valid but extreme xy values. This re-introduces the previous
checks but with less limited bounds; sufficient to accomodate the
ACEScg end points (ACES AP1) but not for the ACES AP0 end points. Those
were not working anyway because libpng reads the cHRM parameters as
unsigned values so they must always be at least 0.
A better solution requires recognizing reasonable negative values (ones
which violate the current spec) and allowing them too, at least on read.
Referenced report:
LibreOffice/core@905e7c1
Original:
https://gerrit.libreoffice.org/c/core/+/173409
The cHRM chunk is recognized as invalid but only after an integer
overflow has occured. This needs better testing but there is no
obvious way of detecting integer overflow and no obvious way of
generating the invalid cHRM values (given that they tend to be
detected anyway!)
Signed-off-by: John Bowler [email protected]