RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]

[Martin Balao]

On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao <mbalao at openjdk.org> wrote:

>> As described in JDK-8271566 [1], this patch proposal is intended to fix a problem that arises when using DSA keys that have a 256-bits (or larger) G parameter for signatures (either signing or verifying). There were some incorrect assumptions and hard-coded length values in the code before. Please note that, for example, the tuple (2048, 256) for DSA is valid according to FIPS PUB 186-4.
>> 
>> Beyond the specific issues in signatures, I decided to provide a broader solution and enable key parameter retrieval for other key types (EC, DH) when possible. This is, when the key is not sensitive. One thing that I should note here is that token keys (those that have the CKA_TOKEN attribute equal to 'true') are considered sensitive in this regard, at least by the NSS Software Token implementation. I don't have access to other vendor implementations but if there is any concern, we can adjust the constraint to NSS-only. However, I'm not sure which use-case would require to get private keys out of a real token, weakening its security. I'd be more conservative here and not query the values if not sure that it will succeed.
>> 
>> No regressions found in jdk/sun/security/pkcs11. A new test added: LargerDSAKey.
>> 
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566
>
> Martin Balao has updated the pull request incrementally with one additional commit since the last revision:
> 
>   P11Key static inner classes refactorings.

Hi Valerie,

Apologies for the delay; I've intention to resume this work if you can.

I had a look at your Webrev.00 but I'm not really convinced, at least when comparing it to the Fetcher approach. I'll share my comments below:

 * Changing P11PrivateKey::getFormat to return 'PKCS#8' and not overriding this method on opaque/sensitive private key classes will be an observable change, and it does not match what's specified in Key::getFormat: 'Returns the name of the primary encoding format of this key, or null if this key does not support encoding.'. My thinking here is that an opaque key does not support encoding because its value is not even accessible.

 * P11PrivateKeyRSA and P11RSAPrivateKey class names look confusing to me (where 'RSA' is located does not say anything about the class to me at least). The 'Opaque' suffix for the one that is package-private might be a better choice. I used the suffice 'Internal' before but now I like 'Opaque' more.

 * P11PrivateKeyRSA::of duplicates the attributes retrieval logic (session.token.p11::C_GetAttributeValue call), which is there and in P11Key::fetchAttributes. Sensitive RSA keys get the attributes from one place and non-sensitive from the other. This does not look to me an advantage when compared to the Fetcher, which does the same for RSA and uses P11Key::fetchAttributes for the others. However, having all the RSA-related logic in P11RSAAttributesFetcher::doFetchValues makes it a bit easier for me to reason about the 4 different scenarios to get the data: CRT + sensitive, non-CRT + sensitive, CRT + non-sensitive and non-CRT + non-sensitive.

 * By eliminating P11RSAPrivateKey::getModulus, looks to me that P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now called, leading to an unnecessary call to the native library as the modulus was already received on P11RSAPrivateKey constructor. This happens to P11RSAPrivateNonCRTKey as well.

 * P11PrivateKeyRSA does not make the CRT/non-CRT distinction for sensitive keys, so the public exponent is not obtained when it could be. This is a bit less functionality than what the Fetcher provides, and would require a few changes to fit it into the design.

>From comment #Aug 17:
"(...) and you just cover it up with different wrapper classes which seems algorithm specific but yet do not contain any algorithm specific fields and the logic is all inside the corresponding Fetcher"

 * In my view, it's true that the Fetcher has the logic to get and hold the data. But the classes that use it are not just wrapper classes covering it up: they implement different interfaces and override/implement methods differently. In example, P11RSAPrivateNonCRTKey implements RSAPrivateKey and decides not to provide an implementation of methods such as getPublicExponent, getPrimeP, getPrimeQ, etc. On the other hand, having the logic for the different RSA data-availability scenarios in a single place (CRT sensitive, non-CRT sensitive, CRT non-sensitive and non-CRT non-sensitive) makes it a bit easier to reason about. Otherwise, this is scattered all over the place.

If you still want to take the non-fetcher approach and address the issues mentioned above, I can have a look.

Kind regards,
Martin.-

-------------

PR: https://git.openjdk.java.net/jdk/pull/4961