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

Fri Jun 7 02:50:42 UTC 2019

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/20190606/7c672d06/attachment.htm>