On Tue, 23 May 2023 14:00:30 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> We found several more cases of passwords and encoded keys not cleared that >> were addressed in out Iteration # 2 commit. These cases were both in Java >> and native code. We still have doubts about the effectiveness and need for >> these actions, but prefer to have the discussion on a different channel. >> >> We also found that invalid keys (not starting with the name "PBE") passed to >> PBES2Core::engineInit or P11PBECipher::engineInit were being cleared and >> this could be unexpected to the caller. This is related to my comment >> [here](https://github.com/openjdk/jdk/pull/12396#discussion_r1199513839). >> For these cases, we decided not to invoke ::getEncoded and not to clean. As >> said, we have concerns about these undocumented mutations to objects passed >> by argument. >> >> No test regressions observed in jdk/com/sun/crypto/provider or >> jdk/sun/security/pkcs11. > > @martinuy I am adding a CSR requirement for this issue. In this Enhancement, > you are adding support for new algorithms in a JCE provider. This type of > change requires a CSR as you are adding support for algorithms that can be > used by applications and not just inside the JDK, thus it is a type of > exported interface of JDK scope. For an example of a similar issue, see > [JDK-8274174](https://bugs.openjdk.org/browse/JDK-8274174). The CSR should > also include details about any behavioral changes or differences that affect > use of the algorithms through the standard JCE APIs. >> @seanjmullan @valeriepeng , can you please take a look at the proposed >> [JDK-8308719](https://bugs.openjdk.org/browse/JDK-8308719) CSR? > > Under the "Specification" section, the Cipher ones list out the required > mech(s). But the rest, e.g. SecretKeyFactory and Mac, have X and Y. Based on > the code in SunPKCS11.java, it seems Y is the required one and X is the one > which is used. @valeriepeng: I think there are a couple of things worth clarifying. Let's take one example of each service type. ## Cipher.PBEWithHmacSHA1AndAES_128 https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java#L883-L885 Here, one of `CKM_AES_CBC_PAD` or `CKM_AES_CBC` must be present for the _encryption/decryption_ operation (performed by the [underlying _Cipher_](https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PBECipher.java#L66)). Additionally, we require `CKM_PKCS5_PBKD2` for the _derivation_ operation, and `CKM_SHA_1_HMAC` as way to anticipate the `CKP_PKCS5_PBKD2_HMAC_SHA1` availability: https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java#L198-L199 A totally explicit entry would be: | Java Algorithm | PKCS<span>#</span>11 Mechanisms | |:--------------------------------:|:-------------------------------------------------------------------------------:| | Cipher.PBEWithHmacSHA1AndAES_128 | (`CKM_AES_CBC_PAD` or `CKM_AES_CBC`) and `CKM_PKCS5_PBKD2` and `CKM_SHA_1_HMAC` | But we decided to follow the _comma ⟹ "or"_ convention, previously established in the table, so we added the additionally required mechanisms as a parenthesised note: | Java Algorithm | PKCS<span>#</span>11 Mechanisms | |:--------------------------------:|:----------------------------------------------------------------------------------------------:| | Cipher.PBEWithHmacSHA1AndAES_128 | `CKM_AES_CBC_PAD`, `CKM_AES_CBC` (`CKM_PKCS5_PBKD2` and `CKM_SHA_1_HMAC` required in any case) | ## Mac.HmacPBESHA1 https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java#L620-L621 Here, `CKM_SHA_1_HMAC` must be present for the _MAC generation/verification_ operation (performed by [letting the derived key continue to the _Mac_ implementation](https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java#L227-L251)). Additionally, we require `CKM_PBA_SHA1_WITH_SHA1_HMAC` for the _derivation_ operation: https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java#L230-L231 A totally explicit entry would be: | Java Algorithm | PKCS<span>#</span>11 Mechanisms | |:---------------:|:--------------------------------------------------:| | Mac.HmacPBESHA1 | `CKM_SHA_1_HMAC` and `CKM_PBA_SHA1_WITH_SHA1_HMAC` | This is what we wrote (in the inverse order). ## SecretKeyFactory.PBEWithHmacSHA1AndAES_128 https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java#L736-L737 Since the SecretKeyFactory service just derives, we require `CKM_PKCS5_PBKD2` for the _derivation_ operation, and `CKM_SHA_1_HMAC` as way to anticipate the `CKP_PKCS5_PBKD2_HMAC_SHA1` availability. A totally explicit entry would be: | Java Algorithm | PKCS<span>#</span>11 Mechanisms | |:------------------------------------------:|:--------------------------------------:| | SecretKeyFactory.PBEWithHmacSHA1AndAES_128 | `CKM_PKCS5_PBKD2` and `CKM_SHA_1_HMAC` | This is what we wrote. ---- # Proposal > We should either follow the convention as in Cipher or explain what "X and Y" > means. Rest looks good to me. We are more or less following the same convention as in Cipher, using _comma_ instead of _"and"_ for Mac.HmacPBESHA1 and SecretKeyFactory.PBEWithHmacSHA1AndAES_128 would wrongly imply that any of the mechanisms is enough. With the above explanations, do you still think that this table needs more clarification? Would it improve if we add a trailing _"required in any case"_ at the end, just like the parenthesised notes? Something like: | Java Algorithm | PKCS<span>#</span>11 Mechanisms | |:------------------------------------------:|:----------------------------------------------------------------------------------------------:| | […] | […] | | Cipher.PBEWithHmacSHA1AndAES_128 | `CKM_AES_CBC_PAD`, `CKM_AES_CBC` (`CKM_PKCS5_PBKD2` and `CKM_SHA_1_HMAC` required in any case) | | […] | […] | | Mac.HmacPBESHA1 | `CKM_SHA_1_HMAC` and `CKM_PBA_SHA1_WITH_SHA1_HMAC` required in any case | | […] | […] | | SecretKeyFactory.PBEWithHmacSHA1AndAES_128 | `CKM_PKCS5_PBKD2` and `CKM_SHA_1_HMAC` required in any case | ------------- PR Comment: https://git.openjdk.org/jdk/pull/12396#issuecomment-1570972521
