RFR 8157404: Unable to read certain PKCS12 keystores from SequenceInputStream

[Svetlana Nikandrova]

Hi Max,

thank you for your replay. Please see inline.

On 30.08.2016 5:43, Weijun Wang wrote:
> readAllBytes() wants to real *all* data. If this is during a long time 
> communication, and the peer sends a BER using indefinite length, and 
> then wait for the next round of dialog without closing the stream, 
> will readAllBytes() hang?

readAllBytes() blocks until all remaining data has been read and end of 
stream is detected.
So if I understood your scenario correctly, yes, if EOF is not present 
it will hang. AFAIK when KeyStore.load completes successfully it 
consumes the input stream until EOF, so isn't it ok to expect it?

> Is it possible to create a new method readAllAvailables() here? It 
> kept read data until available() is zero and return the concatenated 
> byte array. I think this method does not block and is able to read 
> everything available from a SequenceInputStream.

Yes, I think it is possible to read data by portions, but unfortunately 
straight forward approach like "while(is.available() !=0)" won't work. 
As soon as we reach the end of the first stream in SequenceInputStream 
sequence is.available() will return 0 until we switch to the next stream 
by calling read(). I'm afraid it will look a little bit confusing.

> In fact, IMO the DerValue is not implemented correctly. It should have 
> been able to read the input data chunk by chunk until a BER is fully 
> consumed, leaving data after the BER untouched, and block if the BER 
> is not complete. To achieve this, it should read the header, read zero 
> or more sub-data, read the EOC, recursively if necessary.

It asks for some investigation. Should I create a separate issue?

Thank you,
Svetlana

>
> Thanks
> Max
>
> On 8/30/2016 2:52, Artem Smotrakov wrote:
>> Hi Svetlana,
>>
>> It looks fine, but I am not an official reviewer.
>>
>> "keystorePath" in readTest() can be a static field.
>>
>> I also meant that one test with SequenceInputStream seems to be enough,
>> so you could just add a new test case to ReadP12Test.java. But it's 
>> fine.
>>
>> I am not sure how DerIndefLenConverter works, but it looks a little
>> strange to me that it needs to extend an array before passing it to
>> DerIndefLenConverter. I see that convert() method also uses arraycopy()
>> method. But it seems to be out of scope here.
>>
>> Artem
>>
>>
>> On 08/29/2016 11:23 AM, Svetlana Nikandrova wrote:
>>> Hi Artem,
>>>
>>> thank you for your replay. I've updated copyright and made separate
>>> test for this bug.
>>> As for Arrays.copyOfRange() unfortunately it won't simplify code in my
>>> case. I need to extend an array, not to get a sub-array of existing 
>>> one.
>>>
>>> http://cr.openjdk.java.net/~snikandrova/8157404/webrev.01/
>>> <http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.01/>
>>>
>>> Thanks,
>>> Svetlana
>>>
>>> On 26.08.2016 23:48, Artem Smotrakov wrote:
>>>>
>>>> Hi Svetlana,
>>>>
>>>> DerValue class may be implicitly used in different areas (x509,
>>>> SSL/TLS, keystores, maybe krb5, etc). Please make sure that tests
>>>> from jdk_security pass.
>>>>
>>>> I'll leave the main review to someone who is more knowledgeable in
>>>> this area, here are a couple of comments:
>>>> - Please update copyright year
>>>> - You may want to replace new byte[] + System.arraycopy() by
>>>> Arrays.copyOfRange()
>>>> - It may be better to add a separate test case in ReadP12Test.java
>>>> for SequenceInputStream instead of loading a keystore twice in each
>>>> call to readTest(). One test with SequenceInputStream seems to be
>>>> enough, and it would make the logic of readTest() clearer.
>>>>
>>>> Artem
>>>>
>>>> On 08/26/2016 10:58 AM, Svetlana Nikandrova wrote:
>>>>> Hello,
>>>>>
>>>>> please review this fix. It's not possible to read PKCS12 keystore
>>>>> with big undefined length DER value in it from SequenceInputStream.
>>>>> Root cause of the problem is that sun.security.util.DerValue relays
>>>>> on InputStream.available() to get a complete 'indefinite.length'
>>>>> section length and then read it, but for SequenceInputStream this
>>>>> method returns number of available bytes only for current input
>>>>> stream, not the whole sequence. Fixed to read all available data.
>>>>>
>>>>> JBS:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8157404
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~snikandrova/8157404/webrev.00/
>>>>> <http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.00/>
>>>>>
>>>>> Thanks,
>>>>> Svetlana
>>>>>
>>>>
>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20160830/6b9004eb/attachment.htm>