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
