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.p...@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.p...@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.p...@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.w...@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 >>>>>>>>
