[13] RFR JDK-8080462: Update SunPKCS11 provider with PKCS11 v2.40 support
Valerie Peng
valerie.peng at oracle.com
Mon Jun 10 21:18:42 UTC 2019
Hi Jamil,
Sure, I will add these two OIDs as well. If there is discussion on oid
registry, I'd like to join to explore potential ideas.
Will re-test w/ mach5 and update the webrev in place and integrate once
the mach5 job is finished.
Thanks!
Valerie
On 6/10/2019 9:07 AM, Jamil Nimeh wrote:
> 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/96341236/attachment.htm>
More information about the security-dev
mailing list