RFR 8244565: Accept PKCS #8 with version number 1

Weijun Wang weijun.wang at oracle.com
Wed May 27 00:54:33 UTC 2020 

Can you please take a look (not a review) at an updated webrev at http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the code to inspect the extra fields. I haven't deal with the "..." here yet.

However, when I tried to consolidate the DER parsing into one place, I've made more and more changes everywhere. The major change now is a base constructor PKCS8Key(byte[]) and subclass constructors that call it and no more protected parseKeyBits(). Although I don't think there is any technical error here but at the end of the day I'm asking myself if this is too much code change for such a simple bug.

Do you like the overall structure? If yes, I'll continue this way. Otherwise, I can restrict the change only inside PKCS8Key.

Thanks,
Max


> On May 27, 2020, at 7:14 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
> 
> I suppose we can allow trailing data to conform to "..." then.
> 
> But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the publicKey should not be present for V1? This should be checked?
> 
> Valerie
> 
> On 5/25/2020 9:02 PM, Weijun Wang wrote:
>> The new definition at https://tools.ietf.org/html/rfc5958 shows,
>> 
>>      OneAsymmetricKey ::= SEQUENCE {
>>        version                   Version,
>>        privateKeyAlgorithm       PrivateKeyAlgorithmIdentifier,
>>        privateKey                PrivateKey,
>>        attributes            [0] Attributes OPTIONAL,
>>        ...,
>>        [[2: publicKey        [1] PublicKey OPTIONAL ]],
>>        ...
>>      }
>> 
>> According to https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
>> 
>>     The string "..." in ASN.1 is called an extension marker. The extension marker,
>>     ellipsis, is an indication that extension additions are expected. It makes no
>>     statement as to how such additions should be handled. However they shall not be
>>     treated as an error during the decoding process.
>> 
>> So there might be more fields in the future. Do you still insist we need more validation?
>> 
>> Thanks,
>> Max
>> 
>> 
>>> On May 20, 2020, at 1:37 PM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>> 
>>> 
>>> True, the current handling of the extra bytes in parseKey() and decode() are kind of opposite, one does not allow any extra bytes but the other ignores all. The trailing bytes may look harmless but simply ignores it may open up something unexpected.
>>> 
>>> Given that this is key related class, I think it's important to do reasonable validation even though we currently do not use these trailing bytes. It'd also be good if the handling can be consistent regardless of the call path.
>>> 
>>> Thanks,
>>> Valerie
>>> On 5/18/2020 9:36 PM, Weijun Wang wrote:
>>>>> On May 19, 2020, at 1:41 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>>>> 
>>>>> Hi Max,
>>>>> 
>>>>> I saw in the bug description that handling and parsing of the public key will be resolved in a separate enhancement which is fine with me.
>>>>> 
>>>>> In addition to relaxing the PKCS8 version check to accept 1, the current webrev does not check for the trailing bytes and its validity. Perhaps we should consider adding necessary checks to ensure spec conformance? Perhaps some utility method like: (This includes basic checks plus checks for multiple attributes and version 1 w/ public key. )
>>>>> 
>>>>>     private static void checkTrailingBytes(DerInputStream derIn, int version)
>>>>>             throws IOException {
>>>>>         boolean hasAttributes = false;
>>>>>         boolean hasPublicKey = false;
>>>>>         while (derIn.available () != 0) {
>>>>>             // check for OPTIONAL attributes and/or publicKey
>>>>>             DerValue tmp = derIn.getDerValue();
>>>>>             if (tmp.isContextSpecific((byte)0) && !hasAttributes) {
>>>>>                 // OPTIONAL attributes not supported yet
>>>>>                 // discard for now and move on
>>>>>                 hasAttributes = true;
>>>>>             } else if (version == V2 && tmp.isContextSpecific((byte)1) &&
>>>>>                 !hasPublicKey) {
>>>>>                 // OPTIONAL v2 public key not supported yet
>>>>>                 // discard for now and move on
>>>>>                 hasPublicKey = true;
>>>>>             } else {
>>>>>                 throw new IOException ("illegal encoding in private key");
>>>>>             }
>>>>>         }
>>>>>     }
>>>> I wonder if this is necessary. The extra bytes are quite harmless, and we didn't check it in decode().
>>>> 
>>>>> Besides the encoding parsing check above, maybe V1, V2 can be made private static?
>>>> OK.
>>>> 
>>>>> I noticed that the PKCS8Key class has a block of code under the comments "Try again using JDK1.1-style...", however the provider property "PrivateKey.PKCS#8.<alg>" seems long gone? Without these property, this block of code seems useless and probably should be cleaned up as well.
>>>> Yes.
>>>> 
>>>>> As for the test, the existing comments for the EXPECTED bytes are off and plain misleading. Could you please fix them or at least remove them. For example, the comment of "integer 3" should be associated with "0x02, 0x01, 0x03," bytes instead of "0x01, 0x02, 0x02,". In the updated test, NEW_ENCODED_KEY_INTS is PKCS8 v1 key and NEW_ENCODED_KEY_INTS_2 is PKCS8 v2 key w/ public key. Since the version value is always at index 4, we can either clone the existing bytes or directly override the version value in NEW_ENCODED_KEY_INTS or NEW_ENCODED_KEY_INTS_2 to add additional negative tests, e.g. encoding w/ the version value 2 (anything besides the allowed 0 and 1), version value 0 w/ trailing public key. It'd be nice to test version 1 w/ trailing bytes w/ invalid tag value as well.
>>>> If you still want checkTrailingBytes, then yes.
>>>> 
>>>> Thanks,
>>>> Max
>>>> 
>>>>> Thanks,
>>>>> 
>>>>> Valerie
>>>>> 
>>>>> 
>>>>> On 5/13/2020 5:16 PM, Valerie Peng wrote:
>>>>>> I will take a look.
>>>>>> 
>>>>>> Valerie
>>>>>> 
>>>>>> On 5/8/2020 3:39 AM, Weijun Wang wrote:
>>>>>>> Found an existing test I can update. Webrev updated at
>>>>>>> 
>>>>>>>     http://cr.openjdk.java.net/~weijun/8244565/webrev.01/
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Max
>>>>>>> 
>>>>>>>> On May 8, 2020, at 5:41 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>>>>>>> 
>>>>>>>> Please take a review at
>>>>>>>> 
>>>>>>>>    http://cr.openjdk.java.net/~weijun/8244565/webrev.00/
>>>>>>>> 
>>>>>>>> Now we accepts a PKCS8 file with version being 0 or 1. The publicKey and attributes are still not parsed.
>>>>>>>> 
>>>>>>>> I also take this chance to make some format change.
>>>>>>>> 
>>>>>>>> There is no regression test and I'll add one. I can generate a PKCS8 key and then modify the version from 0 to 1 and try to read it. Or I can find a PKCS8 generated by some other tool and embed it the test to read it. Please advise which is better.
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Max
>>>>>>>>

More information about the security-dev mailing list