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

[Jamil Nimeh]

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.
      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.
  * 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)?
  * 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"?
  * Functions.java
      o 412: Typo: Vender -> Vendor
  * 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?
  * 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.
  * 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.
      o 797: You don't need a return here, do you?
  * p11crypt.c
      o 146-7: Still need these lines?
  * 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?
  * 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.
      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?
      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.
  * 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.
  * 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.

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