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

Thu Jun 6 02:42:10 UTC 2019

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/20190605/2f7368e8/attachment.htm>