Hello Jay
The existing verification of CRC value with already stored value is only to
ensure we have processed right amount of data.
This is done at the end and is explained in the comments at Line 853. It does
not validate whether a chunk (length/ data) is valid upfront.
Here is the code:
728 try {
729 stream.mark();
730 stream.seek(stream.getStreamPosition() + chunkLength);
731 chunkCRC = stream.readInt();
732 stream.reset();
733 } catch (IOException e) {
734 throw new IIOException("Invalid chunk length " +
chunkLength);
735 }
736
737 switch (chunkType) {
...
853 // double check whether all chunk data were consumed
854 if (chunkCRC != stream.readInt()) {
855 throw new IIOException("Failed to read a chunk of type
" +
856 chunkType);
857 }
My suggestion was to -
. 'Generate' CRC from Chunk data and compare it with the retrieved value at
Line 731 'before' proceeding to process any of the chunks.
. In mal-formed chunks (corrupted chunk length /or chunk data), the CRC check
will fail thus giving an effective way to identify a valid chunk.
. Many of the if (...) conditions that 've been added to parse_<Chunk> methods
can be avoided with CRC check done upfront.
Hope this helps.
Thank you
Have a good day
Prahalad N.
-----Original Message-----
From: Jayathirth D V
Sent: Tuesday, January 23, 2018 12:18 PM
To: Prahalad Kumar Narayanan; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-8191023: PngReader throws
NegativeArraySizeException when keyword length exceeds chunk size
Hi Prahalad,
Thanks for providing your inputs.
Regarding the CRC check, as of now in PNGImageReader.readMetadata() we store
CRC value for each chunk based on read chunk length before parsing them and
after parsing a given chunk we verify the CRC value at the end of the chunk
with already stored value. If they differ we throw IIOException. Please let me
know if you are suggesting some other logic related to CRC check.
Also we check negative chunk length value and throw IIOException before we
start parsing a chunk.
Main issue in this bug is while we are parsing a chunk, we use the available
chunk length to determine the remaining length that should be parsed. So only
when we parse these individual chunks we get to know the data present in them
like keyword.length(). In our test sample scenario we hit a condition while we
are parsing where the present chunk length is less than keyword.length() and
when we try to create byte array using this value we hit
NegativeArraySizeException. We will not reach this problem scenario until we
parse these individual chunks.
Regarding the problem in other PNG chunks, I went through all the chunk parsing
functions and wherever I saw that we are using available chunk length and
subtracting some value from it and it might cause NegativeArraySizeException I
have added additional checks(in sPLT & tRNS).
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/8191023/webrev.01/
Thanks,
Jay
-----Original Message-----
From: Prahalad Kumar Narayanan
Sent: Tuesday, January 23, 2018 8:24 AM
To: Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-8191023: PngReader throws
NegativeArraySizeException when keyword length exceeds chunk size
Hello Jay
I looked into the bug and the fix.
The root cause of the problem is mal-formed "chunk length" field.
Thus the available chunk data to decode could either be larger (or) lesser than
the value of "chunk length".
The bug seems to target one specific case- where available data is larger than
value of "Chunk length". Moreover bug targets few chunks alone.
When a "chunk length" is mal-formed, exceptions could be triggered while
reading any data (not just the case indicated by the bug).
There are 4 critical chunks and as many as 12 ancillary chunks that may appear
in a PNG stream.
Adding checks for every individual case would flood the PNG image reader with
if (...) checks.
Can you think of any better solution to detect mal-formed "chunk length" and
chunk data ?
How about using the chunk's CRC field ?
You could generate CRC with data read from the stream and compare it against
the CRC stored for the chunk to validate.
Thank you
Have a good day
Prahalad N.
----- Original Message -----
From: Jayathirth D V
Sent: Monday, January 22, 2018 4:20 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] [11] RFR JDK-8191023: PngReader throws
NegativeArraySizeException when keyword length exceeds chunk size
Hello All,
Please review the following fix in JDK11 :
Bug : https://bugs.openjdk.java.net/browse/JDK-8191023
Webrev : http://cr.openjdk.java.net/~jdv/8191023/webrev.00/
Note : Submitter has raised 3 bugs JDK-8191023 , JDK-8191076 , JDK-8191109 with
similar issue but in 3 different PNG chunks. I have closed two bugs and kept
first opened JBS bug for this issue. From the closed bug test samples are
picked and merged into one test case.
Issue: When the issue was reported PNGImageReader was throwing
NegativeArraySizeException when chunk length is malformed and it exceeds
keyword length. After changes present in
https://bugs.openjdk.java.net/browse/JDK-8190332 the NegativeArraySizeException
is wrapped inside IIOException.
Exception in thread "main" javax.imageio.IIOException: Caught exception during
read:
at
java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageReader.java:1707)
at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
at
PngReaderTextChunkKeywordSizeTest.main(PngReaderTextChunkKeywordSizeTest.java:19)
Caused by: java.lang.NegativeArraySizeException
at
java.desktop/com.sun.imageio.plugins.png.PNGImageReader.parse_tEXt_chunk(PNGImageReader.java:563)
at
java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readMetadata(PNGImageReader.java:816)
at
java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNGImageReader.java:1331)
at
java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageReader.java:1700)
Root cause : Since the chunk length present is lesser than keyword length. When
we try to parse individual chunks and create byte Array to store remaining data
in the chunk. We calculate the byte array size from chunk length and size of
alreadt parsed data like keyword (like chunkLength - keyword.length() - 2).
This results in negative value and it causes NegativeArraySizeException when we
try to create the byte Array.
Solution: Add check in parse function of all the individual chunks to check for
negative value for the size of the remaining data to be stored. We have PNG
stream data from 3 bugs with which we can reproduce this issue for zTXt, tEXt
and iCCP chunk but we don't have stream data for iTXt chunk but still I have
added similar check in parse_iTXt_chunk() function also.
Thanks,
Jay
