Re: [PATCH v2 6/9] staging: ccree: add FIPS support

[Stephan Müller]

Am Montag, 24. April 2017, 09:07:45 CEST schrieb Gilad Ben-Yossef:

Hi Gilad,

> I guess we could change the function to indicate that a key is valid
> for decryption but not encryption
> and have the implementation limiting based on that if there is an
> interest in SP800-131A compliance.

I would be delighted to see and review a patch.

Ciao
Stephan

Re: [PATCH v2 6/9] staging: ccree: add FIPS support

[Stephan Müller]

Am Montag, 24. April 2017, 09:04:13 CEST schrieb Gilad Ben-Yossef:

Hi Gilad,

> 
> Thanks you for the clarification. As I think is obvious by now I am
> not a FIPS expert by any stretch.
> 
> Isn't the requirements on DRBG or KDF invocations pertain to key
> generation only?
> What happens if you don't derive the keys on the system, but wish to
> use keys derived elsewhere?
> I assumed the limitations on weak keys etc. were meant to protect
> against that scenario and are
> therefore independent from key generation requirements, but I may have
> misunderstood that.

That is exactly an important question. NIST lately moved away from a pure 
cipher-only view of cryptography to a more holistic view (i.e. where are 
ciphers used).

That said, for 3DES there is no formal requirement that the 3 keys must be 
checked. NIST is fine when documentation says how the keys are generated by 
some logic outside the module.
> 
> Anyway, if I understand you correctly, what you are saying is that
> these checks might make an
> implementation RFC 2451 and SP800-131A compliant respectively but they
> are not required for
> FIPS 140-2 compliance? did I understand that correctly?

Correct. Regarding SP800-131A, it only says that a full 3key 3DES is required. 
It does not say whether/how the key shall be enforced not being identical.
> 
> If so, since two 3DES implementation in the kernel already do the RFC
> 2451 compliant check I assume
> you would not object to the ccree driver using the same function,
> disconnect from FIPS being set or
> not, just as is being done with the other 3DES implementation.

Absolutely. If possible, all 3DES implementations should use the same checking 
functions. The existing checking function regarding the prevention of 1key 
3DES should be used by your code too.

All I am saying that from a FIPS perspective, there is no need to extent the 
common function by a 3key 3DES enforcer.
> 
> In addition, would it be OK if we did an extra check in the ccree
> driver for SP800-131A key compliance
> and disable encryption (but allow decryption) if the key fails the
> check? again, this would be independent
> from FIPS mode?

My personal taste would be: changes that makes sense for all 3DES 
implementations should go to a common function. Otherwise, 3DES implementation 
A behaves differently than impl. B.

That said, having a check that all three keys are non-identical would 
certainly be good (see my Ack to the patch from a year ago). But you cannot 
use the argument that FIPS mandates it to push it through. :-)

Ciao
Stephan

PS: There is currently a new requirement being discussed for FIPS: 3DES 
operations should not allow more than 4GB of data to be encrypted with one 
key. This requirement would need technical enforcement. I am looking into this 
one for some time now.

Re: [PATCH v2 6/9] staging: ccree: add FIPS support

[Gilad Ben-Yossef]

On Mon, Apr 24, 2017 at 9:16 AM, Stephan Müller  wrote:
> Am Montag, 24. April 2017, 08:06:09 CEST schrieb Gilad Ben-Yossef:
>
> Hi Gilad,
>>
>> Well, it turns out there is and we do :-)
>>
>> This is from crypto/des_generic.c:
>>
>> /*
>>  * RFC2451:
>>  *
>>  *   For DES-EDE3, there is no known need to reject weak or
>>  *   complementation keys.  Any weakness is obviated by the use of
>>  *   multiple keys.
>>  *
>>  *   However, if the first two or last two independent 64-bit keys are
>>  *   equal (k1 == k2 or k2 == k3), then the DES3 operation is simply the
>>  *   same as DES.  Implementers MUST reject keys that exhibit this
>>  *   property.
>>  *
>>  */
>> int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key,
>>   unsigned int keylen)
>>
>> However, this does not check that k1 == k3. In this case DES3
>> becomes 2DES (2-keys TDEA), the use of which was dropped post 2015
>> by NIST Special Publication 800-131A*.
>
> It is correct that the RFC wants at least a 2key 3DES. And it is correct that
> SP800-131A mandates 3key 3DES post 2015. All I am saying is that FIPS 140-2
> does *not* require a technical verification of the 3 keys being not identical.
>
> Note, formally, FIPS 140-2 requires that the 3 keys (i.e. all 192 bits) must
> be obtained from *one* call to a DRBG or KDF (separate independent calls to,
> say, obtain one key at a time is *not* permitted). Of course, fixing the
> parity bits is allowed after obtaining the random number.
>>
>> Would it be acceptable if I offer a patch adding this check to
>> __des3_ede_setkey()
>> and use that in the ccree driver?
>
> I am not sure it makes sense as the core requirement is the *one* invocation
> of the DRBG.


Thanks you for the clarification. As I think is obvious by now I am
not a FIPS expert by any stretch.

Isn't the requirements on DRBG or KDF invocations pertain to key
generation only?
What happens if you don't derive the keys on the system, but wish to
use keys derived elsewhere?
I assumed the limitations on weak keys etc. were meant to protect
against that scenario and are
therefore independent from key generation requirements, but I may have
misunderstood that.

Anyway, if I understand you correctly, what you are saying is that
these checks might make an
implementation RFC 2451 and SP800-131A compliant respectively but they
are not required for
FIPS 140-2 compliance? did I understand that correctly?

If so, since two 3DES implementation in the kernel already do the RFC
2451 compliant check I assume
you would not object to the ccree driver using the same function,
disconnect from FIPS being set or
not, just as is being done with the other 3DES implementation.

In addition, would it be OK if we did an extra check in the ccree
driver for SP800-131A key compliance
and disable encryption (but allow decryption) if the key fails the
check? again, this would be independent
from FIPS mode?

Thanks again,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH v2 6/9] staging: ccree: add FIPS support

[Stephan Müller]

Am Montag, 24. April 2017, 08:16:50 CEST schrieb Stephan Müller:

Hi Gilad,

> > 
> > int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key,
> > 
> >   unsigned int keylen)
> > 
> > However, this does not check that k1 == k3. In this case DES3
> > becomes 2DES (2-keys TDEA), the use of which was dropped post 2015
> > by NIST Special Publication 800-131A*.
> 
> It is correct that the RFC wants at least a 2key 3DES. And it is correct
> that SP800-131A mandates 3key 3DES post 2015. All I am saying is that FIPS
> 140-2 does *not* require a technical verification of the 3 keys being not
> identical.

One side note: we had discussed a patch to this function in the past, see [1].

[1] https://patchwork.kernel.org/patch/8293441/

Ciao
Stephan

Re: [PATCH v2 6/9] staging: ccree: add FIPS support

[Stephan Müller]

Am Montag, 24. April 2017, 08:06:09 CEST schrieb Gilad Ben-Yossef:

Hi Gilad,
> 
> Well, it turns out there is and we do :-)
> 
> This is from crypto/des_generic.c:
> 
> /*
>  * RFC2451:
>  *
>  *   For DES-EDE3, there is no known need to reject weak or
>  *   complementation keys.  Any weakness is obviated by the use of
>  *   multiple keys.
>  *
>  *   However, if the first two or last two independent 64-bit keys are
>  *   equal (k1 == k2 or k2 == k3), then the DES3 operation is simply the
>  *   same as DES.  Implementers MUST reject keys that exhibit this
>  *   property.
>  *
>  */
> int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key,
>   unsigned int keylen)
> 
> However, this does not check that k1 == k3. In this case DES3
> becomes 2DES (2-keys TDEA), the use of which was dropped post 2015
> by NIST Special Publication 800-131A*.

It is correct that the RFC wants at least a 2key 3DES. And it is correct that 
SP800-131A mandates 3key 3DES post 2015. All I am saying is that FIPS 140-2 
does *not* require a technical verification of the 3 keys being not identical.

Note, formally, FIPS 140-2 requires that the 3 keys (i.e. all 192 bits) must 
be obtained from *one* call to a DRBG or KDF (separate independent calls to, 
say, obtain one key at a time is *not* permitted). Of course, fixing the 
parity bits is allowed after obtaining the random number.
> 
> Would it be acceptable if I offer a patch adding this check to
> __des3_ede_setkey()
> and use that in the ccree driver?

I am not sure it makes sense as the core requirement is the *one* invocation 
of the DRBG.

Ciao
Stephan

Re: [PATCH v2 6/9] staging: ccree: add FIPS support

[Gilad Ben-Yossef]

On Sun, Apr 23, 2017 at 12:48 PM, Gilad Ben-Yossef  wrote:
> Hi,
>
> Thank you for the review.
>
> On Thu, Apr 20, 2017 at 4:39 PM, Stephan Müller  wrote:
>
>>> +/* The function verifies that tdes keys are not weak.*/
>>> +static int ssi_fips_verify_3des_keys(const u8 *key, unsigned int keylen)
>>> +{
>>> +#ifdef CCREE_FIPS_SUPPORT
>>> +tdes_keys_t *tdes_key = (tdes_keys_t*)key;
>>> +
>>> + /* verify key1 != key2 and key3 != key2*/
>>
>> I do not think that this check is necessary. There is no FIPS requirement or
>> IG that mandates this (unlike the XTS key check).
>>
>> If there would be such requirement, we would need a common service function
>> for all TDES implementations


Well, it turns out there is and we do :-)

This is from crypto/des_generic.c:

/*
 * RFC2451:
 *
 *   For DES-EDE3, there is no known need to reject weak or
 *   complementation keys.  Any weakness is obviated by the use of
 *   multiple keys.
 *
 *   However, if the first two or last two independent 64-bit keys are
 *   equal (k1 == k2 or k2 == k3), then the DES3 operation is simply the
 *   same as DES.  Implementers MUST reject keys that exhibit this
 *   property.
 *
 */
int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key,
  unsigned int keylen)

However, this does not check that k1 == k3. In this case DES3
becomes 2DES (2-keys TDEA), the use of which was dropped post 2015
by NIST Special Publication 800-131A*.

Would it be acceptable if I offer a patch adding this check to
__des3_ede_setkey()
and use that in the ccree driver?

* http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar1.pdf


Many thanks,
Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH v2 6/9] staging: ccree: add FIPS support

[Stephan Müller]

Am Sonntag, 23. April 2017, 11:48:58 CEST schrieb Gilad Ben-Yossef:

Hi Gilad,

> I do wonder if there is value in alternate behavior of stopping crypto
> API on FIPS error rather than a panic though. I will try to get an
> explanation why we do it this way.

In FIPS, all crypto function must cease if a self test fails. This can be done 
by instrumenting the crypto API calls with a check to a global flag or by 
simply terminating the entire "FIPS module".

The panic() is the simplest approach to meet that requirement.

Ciao
Stephan

Re: [PATCH v2 6/9] staging: ccree: add FIPS support

[Gilad Ben-Yossef]

Hi,

Thank you for the review.

On Thu, Apr 20, 2017 at 4:39 PM, Stephan Müller  wrote:

>> +/* The function verifies that tdes keys are not weak.*/
>> +static int ssi_fips_verify_3des_keys(const u8 *key, unsigned int keylen)
>> +{
>> +#ifdef CCREE_FIPS_SUPPORT
>> +tdes_keys_t *tdes_key = (tdes_keys_t*)key;
>> +
>> + /* verify key1 != key2 and key3 != key2*/
>
> I do not think that this check is necessary. There is no FIPS requirement or
> IG that mandates this (unlike the XTS key check).
>
> If there would be such requirement, we would need a common service function
> for all TDES implementations

I am not sure. I have forwarded a question internally and based on the
answer will either drop this or add a common function and post a patch
add the check to all 3DES implementation.

This has been added to the staging TODO list for the driver.

>
>> +if (unlikely( (memcmp((u8*)tdes_key->key1, (u8*)tdes_key->key2,
>> sizeof(tdes_key->key1)) == 0) || +  
>> (memcmp((u8*)tdes_key->key3,
>> (u8*)tdes_key->key2, sizeof(tdes_key->key3)) == 0) )) { +
>> return -ENOEXEC;
>> +}
>> +#endif /* CCREE_FIPS_SUPPORT */
>> +
>> +return 0;
>> +}
>> +
>> +/* The function verifies that xts keys are not weak.*/
>> +static int ssi_fips_verify_xts_keys(const u8 *key, unsigned int keylen)
>> +{
>> +#ifdef CCREE_FIPS_SUPPORT
>> +/* Weak key is define as key that its first half (128/256 lsb)
>> equals its second half (128/256 msb) */ +int singleKeySize = keylen
>> >> 1;
>> +
>> + if (unlikely(memcmp(key, [singleKeySize], singleKeySize) == 0)) {
>> + return -ENOEXEC;
>
> Use xts_check_key.

Will fix. Added to TODO staging list for the driver.

>
>> +The test vectors were taken from:
>> +
>> +* AES
>> +NIST Special Publication 800-38A 2001 Edition
>> +Recommendation for Block Cipher Modes of Operation
>> +http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a.pdf
>> +Appendix F: Example Vectors for Modes of Operation of the AES
>> +
>> +* AES CTS
>> +Advanced Encryption Standard (AES) Encryption for Kerberos 5
>> +February 2005
>> +https://tools.ietf.org/html/rfc3962#appendix-B
>> +B.  Sample Test Vectors
>> +
>> +* AES XTS
>> +http://csrc.nist.gov/groups/STM/cavp/#08
>> +http://csrc.nist.gov/groups/STM/cavp/documents/aes/XTSTestVectors.zip
>> +
>> +* AES CMAC
>> +http://csrc.nist.gov/groups/STM/cavp/index.html#07
>> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/cmactestvectors.zip
>> +
>> +* AES-CCM
>> +http://csrc.nist.gov/groups/STM/cavp/#07
>> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/ccmtestvectors.zip
>> +
>> +* AES-GCM
>> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/gcmtestvectors.zip
>> +
>> +* Triple-DES
>> +NIST Special Publication 800-67 January 2012
>> +Recommendation for the Triple Data Encryption Algorithm (TDEA) Block Cipher
>> +http://csrc.nist.gov/publications/nistpubs/800-67-Rev1/SP-800-67-Rev1.pdf
>> +APPENDIX B: EXAMPLE OF TDEA FORWARD AND INVERSE CIPHER OPERATIONS +and
>> +http://csrc.nist.gov/groups/STM/cavp/#01
>> +http://csrc.nist.gov/groups/STM/cavp/documents/des/tdesmct_intermediate.zip
>> +
>> +* HASH
>> +http://csrc.nist.gov/groups/STM/cavp/#03
>> +http://csrc.nist.gov/groups/STM/cavp/documents/shs/shabytetestvectors.zip
>> +
>> +* HMAC
>> +http://csrc.nist.gov/groups/STM/cavp/#07
>> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/hmactestvectors.zip
>> +
>> +*/
>
> Is this test vector business really needed? Why do you think that testmgr.c is
> not sufficient? Other successful FIPS validations of the kernel crypto API
> managed without such special code.

That is a very good question. I am guessing this has something to do
to with this driver spending its life out of tree and being maintained
against old kernel versions that may have had some gaps in FIPS
testing and since fixed.

I will review what, if at all, is missing from testmgr and fold those
missing parts (if found) there and drop this from the driver.

>
> Also, your entire API seems to implement the approach that if there is a self
> test error, you disable the cipher functions, but leave the rest in-tact. The
> standard kernel crypto API handling logic is to simply panic the kernel. Is it
> really necessary to implement a special case for your driver?
>
>

No it isn't. What ever the behavior we need it should be added,
pending review of course, to the generic FIPS logic handling.

I do wonder if there is value in alternate behavior of stopping crypto
API on FIPS error rather than a panic though. I will try to get an
explanation why we do it this way.

Handling all these has been added to the driver staging TODO list and
will be handled before it matures into drivers/crypto/

Many thanks for the review!

Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH v2 6/9] staging: ccree: add FIPS support

[Stephan Müller]

Am Donnerstag, 20. April 2017, 15:13:00 CEST schrieb Gilad Ben-Yossef:

Hi Gilad,

> +/* The function verifies that tdes keys are not weak.*/
> +static int ssi_fips_verify_3des_keys(const u8 *key, unsigned int keylen)
> +{
> +#ifdef CCREE_FIPS_SUPPORT
> +tdes_keys_t *tdes_key = (tdes_keys_t*)key;
> +
> + /* verify key1 != key2 and key3 != key2*/

I do not think that this check is necessary. There is no FIPS requirement or 
IG that mandates this (unlike the XTS key check).

If there would be such requirement, we would need a common service function 
for all TDES implementations

> +if (unlikely( (memcmp((u8*)tdes_key->key1, (u8*)tdes_key->key2,
> sizeof(tdes_key->key1)) == 0) || +  
> (memcmp((u8*)tdes_key->key3,
> (u8*)tdes_key->key2, sizeof(tdes_key->key3)) == 0) )) { +   
> return -ENOEXEC;
> +}
> +#endif /* CCREE_FIPS_SUPPORT */
> +
> +return 0;
> +}
> +
> +/* The function verifies that xts keys are not weak.*/
> +static int ssi_fips_verify_xts_keys(const u8 *key, unsigned int keylen)
> +{
> +#ifdef CCREE_FIPS_SUPPORT
> +/* Weak key is define as key that its first half (128/256 lsb)
> equals its second half (128/256 msb) */ +int singleKeySize = keylen
> >> 1;
> +
> + if (unlikely(memcmp(key, [singleKeySize], singleKeySize) == 0)) {
> + return -ENOEXEC;

Use xts_check_key.

> +The test vectors were taken from:
> +
> +* AES
> +NIST Special Publication 800-38A 2001 Edition
> +Recommendation for Block Cipher Modes of Operation
> +http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a.pdf
> +Appendix F: Example Vectors for Modes of Operation of the AES
> +
> +* AES CTS
> +Advanced Encryption Standard (AES) Encryption for Kerberos 5
> +February 2005
> +https://tools.ietf.org/html/rfc3962#appendix-B
> +B.  Sample Test Vectors
> +
> +* AES XTS
> +http://csrc.nist.gov/groups/STM/cavp/#08
> +http://csrc.nist.gov/groups/STM/cavp/documents/aes/XTSTestVectors.zip
> +
> +* AES CMAC
> +http://csrc.nist.gov/groups/STM/cavp/index.html#07
> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/cmactestvectors.zip
> +
> +* AES-CCM
> +http://csrc.nist.gov/groups/STM/cavp/#07
> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/ccmtestvectors.zip
> +
> +* AES-GCM
> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/gcmtestvectors.zip
> +
> +* Triple-DES
> +NIST Special Publication 800-67 January 2012
> +Recommendation for the Triple Data Encryption Algorithm (TDEA) Block Cipher
> +http://csrc.nist.gov/publications/nistpubs/800-67-Rev1/SP-800-67-Rev1.pdf
> +APPENDIX B: EXAMPLE OF TDEA FORWARD AND INVERSE CIPHER OPERATIONS +and
> +http://csrc.nist.gov/groups/STM/cavp/#01
> +http://csrc.nist.gov/groups/STM/cavp/documents/des/tdesmct_intermediate.zip
> +
> +* HASH
> +http://csrc.nist.gov/groups/STM/cavp/#03
> +http://csrc.nist.gov/groups/STM/cavp/documents/shs/shabytetestvectors.zip
> +
> +* HMAC
> +http://csrc.nist.gov/groups/STM/cavp/#07
> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/hmactestvectors.zip
> +
> +*/

Is this test vector business really needed? Why do you think that testmgr.c is 
not sufficient? Other successful FIPS validations of the kernel crypto API 
managed without such special code.

Also, your entire API seems to implement the approach that if there is a self 
test error, you disable the cipher functions, but leave the rest in-tact. The 
standard kernel crypto API handling logic is to simply panic the kernel. Is it 
really necessary to implement a special case for your driver?


Ciao
Stephan