[13] RFR JDK-8080462: Update SunPKCS11 provider with PKCS11 v2.40 support

Mon Jun 10 16:07:34 UTC 2019

Hi Valerie,

Sorry I didn't answer your question regarding where I found the 
dsa-with-sha384 and dsa-with-sha512 OIDs.  I usually check OIDs from 
http://www.oid-info.com.  I grabbed the OID without the leaf node 
(2.16.840.1.101.3.4.3) and checked your sha224 and sha256, which were 
correct.  Then I noticed the registry had 384 and 512 which were missing 
in your definitions and figured I'd mention it.

I agree with you about having an OID registry.  In fact, working on 
8076999 I came to a similar conclusion and Weijun and I have been 
throwing around some ideas on how to do that.  We haven't gone too far 
into just talk mostly about how best to set it up and handle 
unknown/supported OIDs.

Regardless of whether you wish to add those two OIDs or not, the review 
as a whole looks good to me.

--Jamil

On 6/6/2019 7:50 PM, Valerie Peng wrote:
>
> Webrev updated: http://cr.openjdk.java.net/~valeriep/8080462/webrev.02/
>
> Mach5 run looks clean.
>
> Thanks,
> Valerie
> On 6/5/2019 7:42 PM, Valerie Peng wrote:
>>
>> Hi Jamil,
>>
>> Thanks much for reviewing this~
>>
>> On 6/5/2019 9:21 AM, Jamil Nimeh wrote:
>>> Hi Valerie, on the whole it looks really good.  I do have some 
>>> comments below:
>>>
>>>   * SunPKCS11.java
>>>       o 728-738: I think you could add 2.16.840.1.101.3.4.3.3 and .4
>>>         for dsa-with-sha384 and dsa-with-sha512, respectively.
>>>
>> Hmm, I didn't find the oids defined for DSA signature with SHA384 and 
>> SHA512 digests off oid registry search. Where did you find the oids, 
>> just curious?
>>
>>>       o 790-792: Are you sure that's the right OID?  OID lookup
>>>         shows it as PKCS10 and has 1.2.840.113549.1.1.10 as rsassa-pss.
>>>
>> Good catch, I missed one component ".1". In hindsight, it'd be nice 
>> to have a sun.security,util.OidMapping utility class which handles 
>> the oid and string name aliasing. It's a pain and error prone to 
>> repeat these inside the providers. Or, maybe JCA can handle this 
>> instead of each provider.
>>
>>>   * CK_MECHANISM.java
>>>       o 171: Can a CK_MECHANISM object be reused after the action
>>>         that calls freeHandle() is completed?  If so, would it be a
>>>         good idea to return the value of this.pHandle back to 0
>>>         (assuming it is nonzero, of course)?
>>>
>> The CK_MECHANISM object itself may be reused. However, the parameters 
>> it contained may change depending on whether the 
>> engineSetParameter(...) is called again with different value. So, the 
>> current model is to allocate/set the pHandle for every init call and 
>> free it after sign/verify call. When it is freed, it must be reset to 
>> 0. Otherwise it may lead to runtime crash later.
>>>
>>>   * CK_RSA_PKCS_PSS_PARAMS.java
>>>       o 55-61: Seems like you could replace all of removeDash() with
>>>         just String's replaceFirst("-", "") method.
>>>       o 74: I see in a lot of equals methods an identity check as
>>>         well, like "if (this == o) { return true; }" maybe add that
>>>         in before you check the contents of "o"?
>>>
>> Sure, changed.
>>>
>>>   * Functions.java
>>>       o 412: Typo: Vender -> Vendor
>>>
>> Fixed.
>>>
>>>   * PKCS11.java
>>>       o 747, 825: Looks like there's a bit of header comment rot. 
>>>         But I'm guessing that could be said of other methods in this
>>>         file that you have not modified.  Think it's worth updating
>>>         the comments?
>>>
>> Ok, I updated the comments for whose are modified by this change.
>>>
>>>   * PKCS11Constants.java
>>>       o 362-363: [Nit] Looks like you've been adding deprecated
>>>         markings for other attributes. According to the header file
>>>         I'm looking at CKA_SECONDARY_AUTH and CKA_AUTH_PIN_FLAGS are
>>>         deprecated.
>>>
>> Added.
>>>
>>>   * p11convert.c
>>>       o 594-613, 1562-1574, 1691, et al.: For the cases where you
>>>         are freeing certain fields within the ckParamPtr before
>>>         returning, what happens to the CK_TLS_PRF_PARAMS structure
>>>         once it has been returned to the caller with those fields
>>>         freed?  Is there any chance that the struct is reused?  If
>>>         so, it might be a good idea to NULL those freed pointers
>>>         out.  If the struct is done away with after this function
>>>         exits then it's fine as-is.  It looks like these branches
>>>         happen on cases where an exception is ultimately thrown, but
>>>         I figured I'd ask to be sure.
>>>
>> When there is a pending exception, we should free natively allocated 
>> memory inside the same method and then return. The structure won't be 
>> used.
>>>
>>>      o
>>>
>>>
>>>       o 797: You don't need a return here, do you?
>>>
>> Removed.
>>>
>>>   * p11crypt.c
>>>       o 146-7: Still need these lines?
>>>
>> I removed all the commented out debugging printf calls. Hopefully we 
>> won't need these again. ;)
>>
>>>   * p11digest.c
>>>       o 102: Style nit, can we get a newline in here and break up
>>>         the parameter list as you've done in other .c files?
>>>
>> Fixed.
>>>
>>>   * p11sign.c
>>>       o 95: I notice in certain places long/jlong values are
>>>         referenced in the format string as %X and sometimes as %lX. 
>>>         Should we standardize on the latter?  Maybe no big deal if
>>>         you aren't seeing compiler warnings.
>>>
>> Done.
>>>
>>>       o 136: You might want to make that %u (or maybe %lu) so the
>>>         data length prints as an unsigned value.  It's unlikely to
>>>         see an overflow here, but who knows?
>>>
>> Done.
>>>
>>>       o 530: Just curious: why do you need a return here?  Isn't
>>>         this a void function?  I don't see it in some of the other
>>>         void functions here.
>>>
>> Removed.
>>>
>>>   * GCMParameters.java
>>>       o 49: Is the @since 1.8 correct here? Not sure you need @since
>>>         for a sun.* family class, but it's also not JDK 8.
>>>
>> This file is mostly lifted from the one inside SunJCE provider. I 
>> have changed the @since as well as the copyright years. Was debating 
>> whether to remove the one in SunJCE provider, but ended up just copy 
>> it over since this RFE is for PKCS#11 provider and want to keep the 
>> scope of changes on PKCS11 only.
>>
>>>   * P11PSSSignature.java
>>>       o isDigestEqual(): It seems like you could simplify this a bit
>>>         by "flattening" both the stdAlg and givenAlg, removing the
>>>         first instance of the "-" and then do a case-ignore
>>>         comparison. Something like flatStdAlg =
>>>         stdAlg.replaceFirst("-", "") and the same with
>>>         flatGivenAlg.  Then just "return
>>>         flatStdAlg.equalsIgnoreCase(flatGivenAlg);"  Maybe I'm
>>>         missing an edge case here, but it seems like it could work
>>>         for all the digest strings you reference in the static
>>>         initializer above.
>>>
>> Well, this isDigestEqual() method is mostly for comparing the edge 
>> case of "SHA-1"/"SHA"/"SHA1". For all other digest algorithms, what 
>> you suggested would work.
>>
>> Will re-test everything and update webrev once the testing passes.
>>
>> Thanks,
>> Valerie
>>>
>>> --Jamil
>>>
>>>
>>> On 4/12/2019 5:05 PM, Valerie Peng wrote:
>>>>
>>>> Anyone has time to review this? Besides the header files update, I 
>>>> added support for AES/GCM/NoPadding support. Ran into some strange 
>>>> NSS error with RSASSA-PSS signature mechanism, so I have not 
>>>> included the PSS signature impl here.
>>>>
>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8080462
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8080462/webrev.00/
>>>>
>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8221442
>>>>
>>>> Thanks,
>>>> Valerie
>>>>
>>>>
>>>

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