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
