[14] RFR JDK-8176837 "SunPKCS11 provider needs to check more details on PKCS11 Mechanism"

Valerie Peng valerie.peng at oracle.com
Fri Sep 20 00:57:23 UTC 2019


Hi Xuelei,

I have added the debugging output for the "unknown" case as you suggested.

Webrev updated in case you feel like taking another look.
http://cr.openjdk.java.net/~valeriep/8176837/webrev.01/

Thanks for the review~
Valerie

----- Original Message -----
From: xuelei.fan at oracle.com
To: valerie.peng at oracle.com, security-dev at openjdk.java.net
Sent: Wednesday, September 18, 2019 6:47:31 PM GMT -08:00 US/Canada Pacific
Subject: Re: [14] RFR JDK-8176837 "SunPKCS11 provider needs to check more details on PKCS11 Mechanism"

Just a very minor nit.

In the initToken() implementation, there is a check for the upper 32 
bits set of a mech and may continue to next mech.  The continue may 
ignore the showinfo dump for some cases in the patch.

Maybe, it might be more straightforward if we don't want to merge the 
log in one place:
1. always show the mech info log for every supported mechanism.
2. if not enabled, log with "DISABLED ...".
3. if upper 32 bits set, log with "UNKNOWN ...".
4. if legacy, log with "Legacy ...".

    for each mech {
        if (showInfo) {
            System.out.println("Mechanism " +
                Functions.getMechanismName(longMech) + ": " +
                p11.C_GetMechanismInfo(slotID, longMech);
        }

        if (!config.isEnabled(longMech)) {
            System.out.println("DISABLED in configuration");

            continue;
        }

        if (longMech >>> 32 != 0) {
            System.out.println("Unknown mechanism in configuration");

            continue;
        }

        int mech = (int)longMech;
        if (isLegacy(token, mech)) {
            System.out.println("Legacy mechanism in configuration");

            continue;
        }

        ...
    }

Otherwise, looks good to me.  Just a nit, no more review cycle is needed 
for me.

Thanks,
Xuelei

On 9/18/2019 5:56 PM, Valerie Peng wrote:
> 
> Can someone help reviewing this fix? AFAIK, only S11.4 have such 
> mechanisms, e.g. RC4 which only does decryption but not encryption.
> But the checks seems reasonable to apply to all PKCS11 libraries when 
> querying mechanisms.
> No new regression test needed as this is already caught by running 
> existing regression tests on S11.4.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8176837
> Webrev: http://cr.openjdk.java.net/~valeriep/8176837/webrev.00/
> 
> Thanks,
> Valerie


More information about the security-dev mailing list