RFR JDK-8000415: Add support for SHA-3

[Jamil Nimeh]

On 5/12/2016 10:38 AM, Anthony Scarpino wrote:
> On 05/04/2016 07:08 PM, Valerie Peng wrote:
>> Hi,
>>
>> Can someone help reviewing the changes for SHA-3?
>>
>> The result has been validated against the NIST test vectors (for
>> BYTE-ONLY impls, i.g. input which are multiples of bytes).
>> The feature complete date is coming up in a week or two. So, if this can
>> be reviewed in a week or so, that'd be great.
>>
>> The changes for SUN providers are quite straight-forward, e.g. SHA-3
>> digest impls based on FIPS PUB 202.
>> As for OracleUcrypto provider, Solaris SHA-3 support is through new
>> libucrypto digest APIs (added in Solaris 12) instead of the libmd.
>> When running on Solaris 12, the new libucrypto APIs will be used.
>> Otherwise, libmd will be used.
>> Changes for OracleUcrypto providers:
>> - add JNI code for the new libucrypto digest APIs
>> - code refactoring, e.g. move the libmd-related code to classes with MD
>> suffix
>> - run-time mechanism number assignment (used to be hardcoded values)
>> - better error reporting
>>
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8000415
>> Webrev: http://cr.openjdk.java.net/~valeriep/8000415/webrev.00/
>>
>> Thanks,
>> Valerie
>
> Just a few minor things.
>
> UcryptoProvider.java:161
> Did you mean to only comment it out and not delete it?
>
> nativeFunc.c:
> I noticed it is checking the library functions via dlsym.  You could 
> have a conditional check on the version of the library via 
> ucrypto_version, then do the dlsym calls if it is supported.
>
>
> I think there is a performance enhancement that I would not hold up 
> for this changeset.  Changing the byte[]'s to long[] might give a good 
> performance gain.  A similar change was done to GHASH.java which 
> resulted in a 10x performance increase.  Given the internal methods 
> appear to use long[][], and lanes are in longs it would probably help. 
> But it most likely require a change to add 
> DigestBase.implCompress(long[], int) and DigestBase.and 
> implDigest(long[], int).  Probably a low priority RFE.
>
> Tony
>
Hi Valerie, just a few additional nits - not a big deal either way:

  * nativeCryptoMD.c:
      o 47, 53, 59, 65, etc.:  You don't need to cast the pointer from
        malloc since you're assigning it to a void *.  The casting only
        needs to happen when you go to use the pointer with a function
        that expects a concrete type.  I tried it with gcc and cc with
        full warnings just to be sure.  But it's no problem to leave it
        in there as-is.
      o 123, 125: I think these may be unnecessary casts, too.
      o 133,135,137: pretty sure free doesn't need casts.  It likes all
        pointers regardless of type.
  * nativeCrypto.c:
      o 40,42: I'm surprised you're not getting warnings on these, as
        "0x0x" isn't a valid format specifier so you have a mismatch in
        terms of arguments vs format specifiers.  Do you mean 0x%0x? 
        The latter works ok.
      o 357: Just a question, does ucryptoDigestFinal automatically free
        the context as part of the finalize?  Otherwise you're only
        freeing the context on a non-zero return value from that
        function.  I'm assuming that the context should be freed
        regardless of the outcome given the header comment.
      o 362: Since context is a pointer, better to use NULL rather than
        zero.

--Jamil




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