Re: RFR: JDK-6676643: Improve current C_GetAttributeValue native implementation
On Tue, 27 Apr 2021 02:41:12 GMT, Valerie Peng wrote: > Anyone can help review this somewhat trivial fix? The main change is inside > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c. This is to > help better troubleshooting by reporting the type of unavailable attributes > in PKCS11 exception message when C_GetAttributeValue(...) call failed. The > java file changes are just cleanup for consolidating the CKR_* constants > definition into PKCS11Exception class. > > Thanks, > Valerie Marked as reviewed by salusasecon...@github.com (no known OpenJDK username). - PR: https://git.openjdk.java.net/jdk/pull/3709
Re: RFR: JDK-6676643: Improve current C_GetAttributeValue native implementation
On Tue, 27 Apr 2021 18:36:28 GMT, Valerie Peng wrote:
>> src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c line 252:
>>
>>> 250:
>>> 251: if (rv != CKR_OK) {
>>> 252: if (rv == CKR_ATTRIBUTE_SENSITIVE || rv ==
>>> CKR_ATTRIBUTE_TYPE_INVALID) {
>>
>> According to the PKCS#11v2.40 spec, `CKR_BUFFER_TOO_SMALL` should be handled
>> in the same special ways as these too (in that it isn't a "true error").
>
> For this particular call, the pValue field is null, it's meant to query the
> exact length of the specified attribute. Thus, CKR_BUFFER_TOO_SMALL should
> not be returned.
> Afterwards, we then allocate the buffer based on this queried result, so
> CKR_BUFFER_TOO_SMALL should also not occur.
> So, based on the current API usage, CKR_BUFFER_TOO_SMALL should not happen.
All my concerns are addressed then. So, while my review doesn't count towards
acceptance of this change, everything LGTM.
-
PR: https://git.openjdk.java.net/jdk/pull/3709
Re: RFR: JDK-6676643: Improve current C_GetAttributeValue native implementation
On Tue, 27 Apr 2021 04:28:26 GMT, Greg Rubin
wrote:
>> Anyone can help review this somewhat trivial fix? The main change is inside
>> src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c. This is to
>> help better troubleshooting by reporting the type of unavailable attributes
>> in PKCS11 exception message when C_GetAttributeValue(...) call failed. The
>> java file changes are just cleanup for consolidating the CKR_* constants
>> definition into PKCS11Exception class.
>>
>> Thanks,
>> Valerie
>
> src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c line 262:
>
>> 260: temp1 = msg;
>> 261: temp2 = msg + 80;
>> 262: for (i = 0; i < ckAttributesLength && temp1 < temp2; i++) {
>
> I think that this loop will append at most 11 bytes to the string each time
> (is this correct?), if so, we can make the check `temp1 < temp2 - 12` to
> count the final null and avoid running off the end with a buffer overflow.
I apologize. This counting code is correct and doesn't have any of the issues I
claimed. `snprintf` takes care of everything and I just missed it last night.
-
PR: https://git.openjdk.java.net/jdk/pull/3709
Re: RFR: JDK-6676643: Improve current C_GetAttributeValue native implementation
On Tue, 27 Apr 2021 02:41:12 GMT, Valerie Peng wrote:
> Anyone can help review this somewhat trivial fix? The main change is inside
> src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c. This is to
> help better troubleshooting by reporting the type of unavailable attributes
> in PKCS11 exception message when C_GetAttributeValue(...) call failed. The
> java file changes are just cleanup for consolidating the CKR_* constants
> definition into PKCS11Exception class.
>
> Thanks,
> Valerie
src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c line 252:
> 250:
> 251: if (rv != CKR_OK) {
> 252: if (rv == CKR_ATTRIBUTE_SENSITIVE || rv ==
> CKR_ATTRIBUTE_TYPE_INVALID) {
According to the PKCS#11v2.40 spec, `CKR_BUFFER_TOO_SMALL` should be handled in
the same special ways as these too (in that it isn't a "true error").
src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c line 253:
> 251: if (rv != CKR_OK) {
> 252: if (rv == CKR_ATTRIBUTE_SENSITIVE || rv ==
> CKR_ATTRIBUTE_TYPE_INVALID) {
> 253: msg = malloc(80); // should be more than sufficient
I'm worried about asserting that 80 is sufficient especially as extremely large
numbers of attributes could be retrieved and the limit check later on seems a
bit lax.
src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c line 262:
> 260: temp1 = msg;
> 261: temp2 = msg + 80;
> 262: for (i = 0; i < ckAttributesLength && temp1 < temp2; i++) {
I think that this loop will append at most 11 bytes to the string each time (is
this correct?), if so, we can make the check `temp1 < temp2 - 12` to count the
final null and avoid running off the end with a buffer overflow.
src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c line 189:
> 187: * returnValue is CKR_OK. Otherwise, it returns the returnValue as a
> jLong.
> 188: *
> 189: * @param env - used to call JNI funktions and to get the Exception class
NitPick: here and above some German seems to have slipped in. I think we want
"functions"
-
PR: https://git.openjdk.java.net/jdk/pull/3709
Re: RFR: 8248268: Support KWP in addition to KW [v4]
I agree that the response from Housley certainly supports that
"AutoPadding" is likely a safe mode to use. I still would prefer not to see
it (keeping things simple) but don't really have any objections to it.
For KW+PKCS5, I have (unfortunately) seen this deployed in the real world
and had to assist Java developers in manually removing PKCS5 padding from
the result of KW decryption. (No OIDs were used in any parts of these
designs, so I cannot say what would have been used.) So, since I had to
help multiple people write this code already, I really cannot object to
adding support for it. The JCA supports many algorithms which shouldn't be
used but exist for compatibility and interactions with other systems (DES,
RC4, etc.). This would be yet another algorithm of that type. (Arguably,
both KW and KWP should probably be replaced by AES-GCM for modern systems,
but that is an entirely different discussion.)
As for OIDs, that seems somewhat unrelated and I don't think we need
something new. I've rarely needed to use the OIDs for KW or KWP and
suspect that we could simply choose the one that corresponds to the
algorithm we actually used.
I've also encountered HSMs which added PKCS5 padding prior to KW so that
all output was 8-byte aligned. That was very frustrating to deal with as it
was not clearly documented at the time.
Finally, I believe that the encoding question is separate from the wrapping
question. Each key type should (and generally does) define how to encode it
as an octet string. Then you apply the relevant wrapping/unwrapping
algorithm to that encoding. KW/KWP should not define how to encode keys any
more than RSA should define how to wrap a serialized RSA key. (However, I
may have misunderstood your comment "... RFC written that specifies the
default encodings for keys wrapped by this algorithm.")
Greg
On Wed, Apr 7, 2021 at 11:51 AM Michael StJohns
wrote:
> *sigh* Minor correction in line.
>
> On 4/7/2021 2:49 PM, Michael StJohns wrote:
>
> On 4/7/2021 1:28 PM, Greg Rubin wrote:
>
> Mike,
>
> Yes, this was in response to your comment.
>
> I'm aware that the IV really serves more as an integrity check and mode
> signalling mechanism than anything else. My concern is that in the past few
> years I've seen various issues related to "in band signalling" where
> something about the ciphertext (or directly associated metadata) changes
> how the data is decrypted and authenticated. This has reached the level
> where several cryptographic forums I participate in are starting to
> consider it a full anti-pattern.
>
> The proposed "AutoPadding" mode is an example of in-band signalling in
> which an externally provided ciphertext changes how it is interpreted.
> While I cannot personally think of a specific risk in this case, I would be
> inclined not to include this mode unless there is a strong driving need
> from our users. While I have definitely seen people not knowing if their
> data was encrypted with KW or KW+PKCS5/7, I haven't personally seen
> uncertainty between KW and KWP. (I also haven't worked with all possible
> HSMs, just a few of them.) So, from a position of caution, I'd avoid
> "AutoPadding", but this is a preference based on current best-practice
> rather than a strong objection based on specific concerns or risks.
>
>
> I sent a note off to the original mode inventor - Russ Housley:
>
> Can you think of any reason why there might be an issue with providing an
> autopadding mode for KW/KWP (e.g. select which to use based on the input
> data for encrypt and determine which was used after running the unwrap
> function but before removing the initial block and any padding)?
>
> I got back:
>
> As long as every party supports both modes, you could use KW id [sic - I
> think he meant "is"]
>
> "if" not "is"
>
> the inout is a multiple of 64 bits, otherwise use KWP. Of course, the
> algorithm identifier needs to be set appropriately.
>
> Which sort of confirms what I thought, but added a question: Are there
> algorithm OIDs for KW with PKCS5 padding or do people just use the KW OID(
> 2.16.840.1.101.3.4.1.{5,25,45}? As far as I can tell, there are no OIDs
> for KW with PKCS5.
>
> Does there need to be an autopad OID?
>
> If it were me, I'd be avoiding implementing the PKCS5 padding mode here.
> I can't actually find a specification that includes it and it looks like a
> hack that was fixed by the specification of KWP. I'd prefer not to extend
> the hack's lifetime, given that RFC5649 is 10+ years old.
>
> WRT to HSM uncertainty, I ran into problems especially trying to wrap RSA
> private keys. Turned out that some encoded as 8 byte multiples and some
> did not. In any event, I mentioned HSMs, but I really care a
Re: RFR: 8248268: Support KWP in addition to KW [v4]
Mike, Yes, this was in response to your comment. I'm aware that the IV really serves more as an integrity check and mode signalling mechanism than anything else. My concern is that in the past few years I've seen various issues related to "in band signalling" where something about the ciphertext (or directly associated metadata) changes how the data is decrypted and authenticated. This has reached the level where several cryptographic forums I participate in are starting to consider it a full anti-pattern. The proposed "AutoPadding" mode is an example of in-band signalling in which an externally provided ciphertext changes how it is interpreted. While I cannot personally think of a specific risk in this case, I would be inclined not to include this mode unless there is a strong driving need from our users. While I have definitely seen people not knowing if their data was encrypted with KW or KW+PKCS5/7, I haven't personally seen uncertainty between KW and KWP. (I also haven't worked with all possible HSMs, just a few of them.) So, from a position of caution, I'd avoid "AutoPadding", but this is a preference based on current best-practice rather than a strong objection based on specific concerns or risks. Thank you, Greg On Sat, Apr 3, 2021 at 4:38 PM Michael StJohns wrote: > On 4/3/2021 11:35 AM, Greg Rubin wrote: > > I'd advise against the AutoPadding scheme without more careful analysis > and discussion. Have we seen either KW or KWP specifications which > recommend that behavior? > > > > My concern is that we've seen cases before where two different > cryptographic algorithms could be selected transparently upon decryption > and it lowers the security of the overall system. (A variant of in-band > signalling.) The general consensus that I've been seeing in the (applied) > cryptographic community is strongly away from in-band signalling and > towards the decryptor fully specifying the algorithms and behavior prior to > attempting decryption. > > I think this is in response to my comment? > > The wrap function can take a Key as an input and can have the unwrap > method produce a Key as an output - indeed it should be used primarily > for this rather than the more general encrypt/decrypt functions. The > problem is that the encoding of the key may not be known prior to the > attempt to wrap it - hence it's not known whether or not padding need be > applied. This is especially problematic with HSMs. Providing an > AutoPadding mode would allow the wrapping algorithm to decide whether to > use either of the RFC 3394 (AKA KW) Integrity Check Value (ICV) or the > RFC5649 (aka KWP) value and padding length. > > The key thing to remember here is that the IV (initial value - RFC > language) /ICV (integrity check value - NIST language)actually isn't an > IV(initialization vector) in the ordinary meaning, it's a flag, padding > and integrity indicator and will be fixed for all keys of the same > length that use the specified values. E.g. unlike other modes that > require an initialization vector, you don't need to know the ICV to > decrypt the underlying key stream, but you can (and for that matter > MUST) easily test the recovered first block against the expected ICV to > determine whether the output needs padding removed or not. > > FWIW, the actual cryptographic operations between padded data and > non-padded data (of the right multiple length) are identical. It's only > the pre or post processing that's looking for different data. > > Obviously, this doesn't work if someone provides their own IV - but > that's fairly unlikely. CF CCM and its non-normative example formatting > function appendix A - each and every implementation I've seen uses that > formatting function, even though it isn't actually required by the > standard. I'd be surprised if anyone decided to use a different set of > non-standard IV values. > > If an AutoPadding mode were implemented, I'd throw exceptions if someone > tried to set the IV. > > Later, Mike > > >
Re: RFR: 8248268: Support KWP in addition to KW [v4]
On Sat, 27 Mar 2021 00:25:09 GMT, Valerie Peng wrote:
>> This change updates SunJCE provider as below:
>> - updated existing AESWrap support with AES/KW/NoPadding cipher
>> transformation.
>> - added support for AES/KWP/NoPadding and AES/KW/PKCS5Padding.
>>
>> Existing AESWrap impl, i.e. AESWrapCipher class, is re-factored and renamed
>> to KeyWrapCipher class. The W and W_inverse functions are moved to KWUtil
>> class. The KW and KWP support are in the new AESKeyWrap and AESKeyWrapPadded
>> classes which extend FeedbackCipher and used in KeyWrapCipher class. To
>> minimize data copying, AESKeyWrap and AESKeyWrapPadded will do the crypto
>> operation over the same input buffer which is allocated and managed by
>> KeyWrapCipher class.
>>
>> Also note that existing AESWrap impl does not take IV. However, the
>> corresponding PKCS#11 mechanisms do, so I added support for accepting IVs to
>> both KW and KWP.
>>
>> Thanks,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refactor code to reduce code duplication
> Address review comments
> Add more test vectors
I'd advise against the AutoPadding scheme without more careful analysis and
discussion. Have we seen either KW or KWP specifications which recommend that
behavior?
My concern is that we've seen cases before where two different cryptographic
algorithms could be selected transparently upon decryption and it lowers the
security of the overall system. (A variant of in-band signalling.) The general
consensus that I've been seeing in the (applied) cryptographic community is
strongly away from in-band signalling and towards the decryptor fully
specifying the algorithms and behavior prior to attempting decryption.
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line
71:
> 69: match &= (ivAndLen[i] == iv[i]);
> 70: }
> 71: if (!match) {
True nitpick (thus ignorable): I believe that using bitwise math is slightly
more resistant to compiler and/or CPU optimization to defend against
timing-attacks. (Since I haven't even seen an attack against KW or KWP, this is
simply a note in general rather than something which needs to be fixed.)
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line
78:
> 76: for (int k = 5; k < SEMI_BLKSIZE; k++) {
> 77: if (outLen != 0) {
> 78: outLen <<= SEMI_BLKSIZE;
While technically, this is correct (as it is shifting by 8 bits), it is pure
coincidence that `SEMI_BLKSIZE` (8 bytes) happens to have the name integer
value as the number of bits in one byte. It took me more reads than I care to
admit to understand why this worked. Could we just replace this one with an `8`
as it is clearer and more accurate?
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v6]
On Wed, 24 Mar 2021 22:35:13 GMT, Valerie Peng wrote: >> Ziyi Luo has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Remove trailing whitespace >> - Refactor P11RSAKeyFactory and improve tests > > test/jdk/sun/security/pkcs11/nss/p11-nss-sensitive.txt line 32: > >> 30: CKM_ECDSA_SHA3_384 >> 31: CKM_ECDSA_SHA3_512 >> 32: } > > I assume this is to avoid using this special provider for non-RSA impls? Add > a comment? This file is copy/pasted from `p11-nss.txt` with only the sensitive modifications at the end. I do not know why those disabled mechanisms are present in the source file. - PR: https://git.openjdk.java.net/jdk/pull/2949
Re: RFR: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec [v6]
On Wed, 24 Mar 2021 22:17:56 GMT, Valerie Peng wrote:
>> Ziyi Luo has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Remove trailing whitespace
>> - Refactor P11RSAKeyFactory and improve tests
>
> test/jdk/sun/security/pkcs11/rsa/TestP11KeyFactoryGetRSAKeySpec.java line 91:
>
>> 89: if (testingSensitiveKeys) {
>> 90: // Expected exception so swallow it
>> 91: ex.printStackTrace();
>
> Add a System.out.println() to clarify that this exception stack trace is
> expected?
Putting it in `System.err.println()` so that it goes to the same output as the
stack trace.
-
PR: https://git.openjdk.java.net/jdk/pull/2949
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Mon, 22 Mar 2021 21:43:31 GMT, Valerie Peng wrote:
>> This change updates SunJCE provider as below:
>> - updated existing AESWrap support with AES/KW/NoPadding cipher
>> transformation.
>> - added support for AES/KWP/NoPadding and AES/KW/PKCS5Padding.
>>
>> Existing AESWrap impl, i.e. AESWrapCipher class, is re-factored and renamed
>> to KeyWrapCipher class. The W and W_inverse functions are moved to KWUtil
>> class. The KW and KWP support are in the new AESKeyWrap and AESKeyWrapPadded
>> classes which extend FeedbackCipher and used in KeyWrapCipher class. To
>> minimize data copying, AESKeyWrap and AESKeyWrapPadded will do the crypto
>> operation over the same input buffer which is allocated and managed by
>> KeyWrapCipher class.
>>
>> Also note that existing AESWrap impl does not take IV. However, the
>> corresponding PKCS#11 mechanisms do, so I added support for accepting IVs to
>> both KW and KWP.
>>
>> Thanks,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
> AES/KWP/NoPadding
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrap.java line 41:
> 39: * and represents AES cipher in KW mode.
> 40: */
> 41: class AESKeyWrap extends FeedbackCipher {
I see lots of unsupported operations and `encrypt/decryptFinal` ignores their
output parameters. Should we be extending `FeedbackCipher` if so much doesn't
seem to quite fit?
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrap.java line 155:
> 153: " be at least 16 bytes and multiples of 8");
> 154: }
> 155: // assert ptOfs == 0; ct == pt; ctOfs == 0;
Is this a missing code assertion?
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrap.java line 193:
> 191: if (!Arrays.equals(ivOut, 0, ICV1.length, this.iv, 0,
> ICV1.length)) {
> 192: throw new IllegalBlockSizeException("Integrity check
> failed");
> 193: }
It feels like an odd asymmetry that we validate the IV upon decryption in
`AESKeyWrap` but the IV is prepended in `KeyWrapCipher.writeIvSemiBlock`. I'm
worried that by separating this logic like this it is easier for us to get it
wrong (or break it in the future).
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line
69:
> 67: if (!Arrays.equals(ivAndLen, 0, ICV2.length, icv, 0,
> ICV2.length)) {
> 68: throw new IllegalBlockSizeException("Integrity check failed");
> 69: }
While I cannot find any public discussion of this, I'm always uncomfortable
checking the plaintext (prior to authentication) against a known value in
non-constant time. I'm worried that this (and the equivalent in the unpadded
version) might be a problem in the future.
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line
246:
> 244: int outLen = validateIV(ivAndLen, this.iv);
> 245: // check padding bytes
> 246: int padLen = ctLen - outLen;
Can we add an explicit check that `0 <= padLen < SEMI_BLKSIZE`?
src/java.base/share/classes/com/sun/crypto/provider/KWUtil.java line 87:
> 85: */
> 86: static final void W_INV(byte[] in, int inLen, byte[] ivOut,
> 87: SymmetricCipher cipher) {
The asymmetry between `W` not taking an IV but `W_INV` returning an IV also
bothers me and seems to make this harder to use safely.
src/java.base/share/classes/com/sun/crypto/provider/KeyWrapCipher.java line 507:
> 505: } else {
> 506: outLen = cipher.encryptFinal(dataBuf, 0, dataIdx, null,
> -1);
> 507: }
Can we extract this shared logic (among the different versions of
`engineDoFinal`) into a separate helper method so that the different
`engineDoFinal` methods just need to handle their specific differences (such as
returning `Arrays.copyOf(dataBuf, outLen)` or calling
`System.arraycopy(dataBuf, 0, out, outOfs, outLen); return outLen;`).
src/java.base/share/classes/com/sun/crypto/provider/KeyWrapCipher.java line 660:
> 658: @Override
> 659: protected byte[] engineWrap(Key key)
> 660: throws IllegalBlockSizeException, InvalidKeyException {
Is it okay that we can call `cipher.init(Cipher.ENCRYPT_MODE, ...)` and then
use it with `cipher.wrap()`?
Also, can we simply delegate the heavy lifting to `engineDoFinal()` (or the new
suggested helper method) rather than duplicating this logic here?
test/jdk/com/sun/crypto/provider/Cipher/KeyWrap/NISTWrapKAT.java line 105:
> 103:
> 104: @DataProvider
> 105: public Object[][] testData() {
Can we please add test cases for `AES/KWP/NoPadding` where the input is an even
multiple of 8? We've [seen bugs in this case
before](https://github.com/pyca/cryptography/issues/4156).
test/jdk/com/sun/crypto/provider/Cipher/KeyWrap/TestGeneral.java line 50:
> 48:
> 49:
