[OpenJDK 2D-Dev] [15] RFR JDK-8242557: Add length limit for strings in PNGImageWriter

[Prasanta Sadhukhan]

OK. Thanks for the clarification. +1

Regards
Prasanta
On 30-Apr-20 4:47 PM, Jayathirth D v wrote:
> Hi Prasanta,
>
> Thats why I pointed to JDK-8195841 
> <https://bugs.openjdk.java.net/browse/JDK-8195841> where 
> PNGImageReader.readNullTerminatedString() is changed.
> Second argument that is getting passed to readNullTerminatedString() 
> is maximum length including null termination.
> While reading the string in readNullTerminatedString() we exit as soon 
> as we hit null termination, it is not necessary to read 80 bytes.
>
> So in case where we have read 80 bytes(where we have corrupted PNG) 
> and last byte is not null termination we throw IIOException.
>
> private String readNullTerminatedString(String charset, int maxLen) 
> throws IOException {
>         ByteArrayOutputStream baos = new ByteArrayOutputStream();
>         int b = 0;
>         int count = 0;
>         while ((maxLen > count++) && ((b = stream.read()) != 0)) {
>             if (b == -1) throw new EOFException();
>             baos.write(b);
>         }
>         if (b != 0) {
>             throw new IIOException("Found non null terminated string");
>         }
>         return new String(baos.toByteArray(), charset);
>     }
>
> So overall in case of reader we throw exception when we have longer 
> than 79 bytes strings and we follow same approach in writer as per 
> specification.
>
> Thanks,
> Jay
>
>> On 30-Apr-2020, at 3:35 PM, Prasanta Sadhukhan 
>> <prasanta.sadhukhan at oracle.com 
>> <mailto:prasanta.sadhukhan at oracle.com>> wrote:
>>
>> Hi Jay,
>>
>> >> we will throw exception in case of reader also
>>
>> But in the fix, only PNGWriter is changed. I did not see in existing 
>> PNGReader any exception being thrown for > 79 bytes. I see in reader, 
>> we have
>>
>> private void parse_iCCP_chunk(int chunkLength) throws IOException {
>>         String keyword = readNullTerminatedString("ISO-8859-1", 80);
>>         int compressedProfileLength = chunkLength - keyword.length() - 2;
>>         if (compressedProfileLength <= 0) {
>>             throw new IIOException("iCCP chunk length is not proper");
>>         }
>>
>> so chunkLength can be say, 100, keyWord.lenth = 79 so 
>> compressedProfileLength = 100-79-2=19 so we will not throw 
>> IOException. Am I missing something?
>>
>> Regards
>> Prasanta
>>
>> On 30-Apr-20 3:23 PM, Jayathirth D v wrote:
>>> Hi Prasanta,
>>>
>>> I didnt say reader will decode more than 79 bytes but user might 
>>> expect more than 79 bytes since we allowed more than 79 bytes write.
>>> While reading non-standard PNG chunks(having greater than 79 bytes 
>>> null terminated string) we will throw exception in case of reader 
>>> also. Its a stream, we will not know the length and we rely 
>>> completely on null termination and we expect it to be spec complaint 
>>> and not more than 79 bytes.
>>>
>>> Thanks,
>>> Jay
>>>
>>>> On 30-Apr-2020, at 2:51 PM, Prasanta Sadhukhan 
>>>> <prasanta.sadhukhan at oracle.com 
>>>> <mailto:prasanta.sadhukhan at oracle.com>> wrote:
>>>>
>>>> Hi Jay,
>>>>
>>>> But why reader will read more than 79 bytes from the chunk..It 
>>>> should also limit it's reading to 1st 79 bytes (if the chunks we 
>>>> get from non-oracle non-standard PNG writer is more than 79bytes) 
>>>> as per spec, no?
>>>>
>>>> Regards
>>>> Prasanta
>>>> On 30-Apr-20 2:01 PM, Jayathirth D v wrote:
>>>>> Hi Prasanta,
>>>>>
>>>>> Thanks for the review.
>>>>> If we consume only 79 bytes and continue writing the image, it 
>>>>> will lead to unexpected behaviour when user tries to read the 
>>>>> image expecting longer than 79 string data. Our reader is also 
>>>>> tightly spec compliant for null terminated strings after 
>>>>> JDK-8195841 <https://bugs.openjdk.java.net/browse/JDK-8195841> and 
>>>>> JDK-8191023 
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8191023>. Its better to 
>>>>> be spec compliant than leaving things open for interpretation.
>>>>>
>>>>> Regards,
>>>>> Jay
>>>>>
>>>>>
>>>>>> On 29-Apr-2020, at 1:22 PM, Prasanta Sadhukhan 
>>>>>> <prasanta.sadhukhan at oracle.com 
>>>>>> <mailto:prasanta.sadhukhan at oracle.com>> wrote:
>>>>>>
>>>>>> Hi Jay,
>>>>>>
>>>>>> Looks good to me . Although I have a question which is, instead 
>>>>>> of throwing Exception, can we proceed with 1st 79 bytes of these 
>>>>>> chunks and continue?
>>>>>>
>>>>>> Regards
>>>>>> Prasanta
>>>>>> On 27-Apr-20 10:51 PM, Philip Race wrote:
>>>>>>> I reviewed http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html
>>>>>>> and I think you covered all the cases.
>>>>>>>
>>>>>>> I also reviewed the reader and it seems we already check only up 
>>>>>>> to 80 chars there.
>>>>>>>
>>>>>>> I note we assume the same max length of 80 for the language tag 
>>>>>>> for iTxt.
>>>>>>> The spec. doesn't specify a limit but I think 80 is more than 
>>>>>>> generous here.
>>>>>>>
>>>>>>> +1
>>>>>>>
>>>>>>> -phil.
>>>>>>>
>>>>>>> On 4/27/20, 9:59 AM, Jayathirth D v wrote:
>>>>>>>> Hello All,
>>>>>>>>
>>>>>>>> Please review the following fix for JDK 15:
>>>>>>>>
>>>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8242557
>>>>>>>> Webrev : http://cr.openjdk.java.net/~jdv/8242557/webrev.00/ 
>>>>>>>> <http://cr.openjdk.java.net/%7Ejdv/8242557/webrev.00/>
>>>>>>>>
>>>>>>>> Issue : PNG specification restricts length of strings in some 
>>>>>>>> chunks to 
>>>>>>>> 79(http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html ) 
>>>>>>>> excluding null termination. But our writer implementation has 
>>>>>>>> no such check.
>>>>>>>> Solution : Add checks in different chunks where there must be 
>>>>>>>> restrictions.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jay
>>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20200430/2eabc8e9/attachment-0001.htm>