Re: RFR: 8248268: Support KWP in addition to KW [v4]
On Sat, 27 Mar 2021 03:51:40 GMT, Greg Rubin
wrote:
>> 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
>
> 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.)
Sure, I can change to below:
Suggestion:
int match = 0;
for (int i = 0; i < ICV2.length; i++) {
match |= (ivAndLen[i] ^ iv[i]);
}
if (match != 0) {
throw new IllegalBlockSizeException("Integrity check failed");
}
Is this what you have in mind?
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v4]
On Sat, 27 Mar 2021 03:54:24 GMT, Greg Rubin
wrote:
>> 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
>
> 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?
Yes, makes sense. I will update it.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
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 about the
> general model for the JCE. I'd *really* like to avoid having to have to
> first figure out the private key encoding length (which may be difficult as
> a provider may not choose to export an unwrapped private key even if its a
> software provider) before
Re: RFR: 8248268: Support KWP in addition to KW [v4]
*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
about the general model for the JCE. I'd *really* like to avoid having
to have to first figure out the private key encoding length (which may
be difficult as a provider may not choose to export an unwrapped
private key even if its a software provider) before choosing the
wrapping algorithm. Doing it that way just fits the JCE model better.
At some point, there needs to be an RFC written that specifies the
default encodings for keys wrapped by this algorithm.
Later, Mike
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,
Re: RFR: 8248268: Support KWP in addition to KW [v4]
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"] 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 about
the general model for the JCE. I'd *really* like to avoid having to
have to first figure out the private key encoding length (which may be
difficult as a provider may not choose to export an unwrapped private
key even if its a software provider) before choosing the wrapping
algorithm. Doing it that way just fits the JCE model better.
At some point, there needs to be an RFC written that specifies the
default encodings for keys wrapped by this algorithm.
Later, Mike
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
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 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: 8248268: Support KWP in addition to KW [v4]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2404/files - new: https://git.openjdk.java.net/jdk/pull/2404/files/c90fdb1e..e4fd6ea2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2404=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2404=02-03 Stats: 465 lines in 6 files changed: 202 ins; 107 del; 156 mod Patch: https://git.openjdk.java.net/jdk/pull/2404.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2404/head:pull/2404 PR: https://git.openjdk.java.net/jdk/pull/2404
