RFR 8157404: Unable to read certain PKCS12 keystores from SequenceInputStream

[Artem Smotrakov]

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/20160829/f82bf5f5/attachment.htm>