RFR 8157404: Unable to read certain PKCS12 keystores from SequenceInputStream

[Weijun Wang]

On 8/31/2016 0:46, Svetlana Nikandrova wrote:
> 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?

The KeyStore::load method makes no assumption on the InputStream 
parameter. More important, DerValue is used much wider than pkcs12 keystore.

>
>> 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.

Well, this is unfortunate.

I also cannot think of a simple fix now. Maybe after see that "not all 
indef len BER resolved" error you can try read more.

>
>> 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?

You can, but please set the target version to major-tbd unless you want 
to work on it now.

Thanks
Max

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