According to FIPS 140-3 IG C.I., only (total) key lengths of either
256 bits or 512 bits are allowed with xts(aes). Make xts_verify_key() to
reject anything else in FIPS mode.
As xts(aes) is the only approved xts() template instantiation in FIPS mode,
the new restriction implemented in xts_verify_key() effectively only
applies to this particular construction.
Signed-off-by: Nicolai Stange <[email protected]>
---
include/crypto/xts.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/crypto/xts.h b/include/crypto/xts.h
index 0f8dba69feb4..a233c1054df2 100644
--- a/include/crypto/xts.h
+++ b/include/crypto/xts.h
@@ -35,6 +35,13 @@ static inline int xts_verify_key(struct crypto_skcipher *tfm,
if (keylen % 2)
return -EINVAL;
+ /*
+ * In FIPS mode only a combined key length of either 256 or
+ * 512 bits is allowed, c.f. FIPS 140-3 IG C.I.
+ */
+ if (fips_enabled && keylen != 32 && keylen != 64)
+ return -EINVAL;
+
/* ensure that the AES and tweak key are not identical */
if ((fips_enabled || (crypto_skcipher_get_flags(tfm) &
CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) &&
--
2.38.0
> diff --git a/include/crypto/xts.h b/include/crypto/xts.h
...
> @@ -35,6 +35,13 @@ static inline int xts_verify_key(struct crypto_skcipher
> *tfm,
> if (keylen % 2)
> return -EINVAL;
>
> + /*
> + * In FIPS mode only a combined key length of either 256 or
> + * 512 bits is allowed, c.f. FIPS 140-3 IG C.I.
> + */
> + if (fips_enabled && keylen != 32 && keylen != 64)
> + return -EINVAL;
> +
> /* ensure that the AES and tweak key are not identical */
> if ((fips_enabled || (crypto_skcipher_get_flags(tfm) &
> CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) &&
> --
> 2.38.0
arch/s390/crypto/aes_s390.c has similar lines:
static int xts_aes_set_key(struct crypto_skcipher *tfm, const u8 *in_key,
unsigned int key_len)
{
struct s390_xts_ctx *xts_ctx = crypto_skcipher_ctx(tfm);
unsigned long fc;
int err;
err = xts_fallback_setkey(tfm, in_key, key_len);
if (err)
return err;
/* In fips mode only 128 bit or 256 bit keys are valid */
if (fips_enabled && key_len != 32 && key_len != 64)
return -EINVAL;
xts_fallback_setkey will now enforce that rule when setting up the
fallback algorithm keys, which makes the xts_aes_set_key check
unreachable.
If that fallback setup were not present, then a call to xts_verify_key
might be preferable to enforce any other rules like the WEAK_KEYS
rule.
> diff --git a/include/crypto/xts.h b/include/crypto/xts.h
...
> @@ -35,6 +35,13 @@ static inline int xts_verify_key(struct crypto_skcipher
> *tfm,
> if (keylen % 2)
> return -EINVAL;
>
> + /*
> + * In FIPS mode only a combined key length of either 256 or
> + * 512 bits is allowed, c.f. FIPS 140-3 IG C.I.
> + */
> + if (fips_enabled && keylen != 32 && keylen != 64)
> + return -EINVAL;
> +
> /* ensure that the AES and tweak key are not identical */
> if ((fips_enabled || (crypto_skcipher_get_flags(tfm) &
> CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) &&
> --
> 2.38.0
There's another function in the same file called xts_check_key()
that is used by some of the hardware drivers:
arch/s390/crypto/paes_s390.c: * xts_check_key verifies the key length is not odd and makes
[that references it in the comment but actually calls xts_verify_key in the code]
drivers/crypto/axis/artpec6_crypto.c: ret = xts_check_key(&cipher->base, key, keylen);
drivers/crypto/cavium/cpt/cptvf_algs.c: err = xts_check_key(tfm, key, keylen);
drivers/crypto/cavium/nitrox/nitrox_skcipher.c: ret = xts_check_key(tfm, key, keylen);
drivers/crypto/ccree/cc_cipher.c: xts_check_key(tfm, key, keylen)) {
drivers/crypto/marvell/octeontx/otx_cptvf_algs.c: ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen);
drivers/crypto/marvell/octeontx2/otx2_cptvf_algs.c: ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen);
drivers/crypto/atmel-aes.c: err = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen);
It already has one check qualified by fips_enabled:
/* ensure that the AES and tweak key are not identical */
if (fips_enabled && !crypto_memneq(key, key + (keylen / 2), keylen / 2))
return -EINVAL;
Should that implement the same key length restrictions?
"Elliott, Robert (Servers)" <[email protected]> writes:
>> diff --git a/include/crypto/xts.h b/include/crypto/xts.h
> ...
>> @@ -35,6 +35,13 @@ static inline int xts_verify_key(struct crypto_skcipher
>> *tfm,
>> if (keylen % 2)
>> return -EINVAL;
>>
>> + /*
>> + * In FIPS mode only a combined key length of either 256 or
>> + * 512 bits is allowed, c.f. FIPS 140-3 IG C.I.
>> + */
>> + if (fips_enabled && keylen != 32 && keylen != 64)
>> + return -EINVAL;
>> +
>> /* ensure that the AES and tweak key are not identical */
>> if ((fips_enabled || (crypto_skcipher_get_flags(tfm) &
>> CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) &&
>> --
>> 2.38.0
>
> There's another function in the same file called xts_check_key()
> that is used by some of the hardware drivers:
Right, thanks for spotting.
AFAICT, xts_check_key() is the older of the two variants,
xts_verify_key() had been introduced with commit f1c131b45410 ("crypto:
xts - Convert to skcipher"). There had initially only been a single
call from generic crypto/xts.c and the main difference to
xts_check_key() had been that it took a crypto_skcipher for its tfm
argument rather than a plain crypto_tfm as xts_check_key() did.
It seems that over time, xts crypto drivers adopted the newer
xts_verify_key() variant then.
>
> arch/s390/crypto/paes_s390.c: * xts_check_key verifies the key length is not odd and makes
> [that references it in the comment but actually calls xts_verify_key in the code]
> drivers/crypto/axis/artpec6_crypto.c: ret = xts_check_key(&cipher->base, key, keylen);
> drivers/crypto/cavium/cpt/cptvf_algs.c: err = xts_check_key(tfm, key, keylen);
> drivers/crypto/cavium/nitrox/nitrox_skcipher.c: ret = xts_check_key(tfm, key, keylen);
> drivers/crypto/ccree/cc_cipher.c: xts_check_key(tfm, key, keylen)) {
> drivers/crypto/marvell/octeontx/otx_cptvf_algs.c: ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen);
> drivers/crypto/marvell/octeontx2/otx2_cptvf_algs.c: ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen);
> drivers/crypto/atmel-aes.c: err = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen);
>
> It already has one check qualified by fips_enabled:
>
> /* ensure that the AES and tweak key are not identical */
> if (fips_enabled && !crypto_memneq(key, key + (keylen / 2), keylen / 2))
> return -EINVAL;
>
> Should that implement the same key length restrictions?
From a quick glance, all of the above drivers merely convert some
crypto_skcipher to a crypto_tfm before passing it to xts_check_key().
So I think these should all be made to call xts_verify_key() directly
instead, the former xts_check_key() could then get dropped. But that's
more of a cleanup IMO and would probably deserve a separate patch series
on its own.
Thanks!
Nicolai
--
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
(HRB 36809, AG Nürnberg)
"Elliott, Robert (Servers)" <[email protected]> writes:
>> diff --git a/include/crypto/xts.h b/include/crypto/xts.h
> ...
>> @@ -35,6 +35,13 @@ static inline int xts_verify_key(struct crypto_skcipher
>> *tfm,
>> if (keylen % 2)
>> return -EINVAL;
>>
>> + /*
>> + * In FIPS mode only a combined key length of either 256 or
>> + * 512 bits is allowed, c.f. FIPS 140-3 IG C.I.
>> + */
>> + if (fips_enabled && keylen != 32 && keylen != 64)
>> + return -EINVAL;
>> +
>> /* ensure that the AES and tweak key are not identical */
>> if ((fips_enabled || (crypto_skcipher_get_flags(tfm) &
>> CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) &&
>> --
>> 2.38.0
>
> arch/s390/crypto/aes_s390.c has similar lines:
>
> static int xts_aes_set_key(struct crypto_skcipher *tfm, const u8 *in_key,
> unsigned int key_len)
> {
> struct s390_xts_ctx *xts_ctx = crypto_skcipher_ctx(tfm);
> unsigned long fc;
> int err;
>
> err = xts_fallback_setkey(tfm, in_key, key_len);
> if (err)
> return err;
>
> /* In fips mode only 128 bit or 256 bit keys are valid */
> if (fips_enabled && key_len != 32 && key_len != 64)
> return -EINVAL;
>
>
> xts_fallback_setkey will now enforce that rule when setting up the
> fallback algorithm keys, which makes the xts_aes_set_key check
> unreachable.
Good finding!
>
> If that fallback setup were not present, then a call to xts_verify_key
> might be preferable to enforce any other rules like the WEAK_KEYS
> rule.
>
So if this patch here would get accepted, I'd propose to remove the then
dead code from aes_s390 afterwards and make an explicit call to
xts_verify_key() instead.
Or shall I split out the XTS patch from this series here and post these
two changes separately then? Herbert, any preferences?
Thanks!
Nicolai
--
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
(HRB 36809, AG Nürnberg)
On Wed, Nov 09, 2022 at 11:39:19AM +0100, Nicolai Stange wrote:
>
> Or shall I split out the XTS patch from this series here and post these
> two changes separately then? Herbert, any preferences?
You can do this as a follow-up.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Wed, Nov 09, 2022 at 11:06:17AM +0100, Nicolai Stange wrote:
>
> >From a quick glance, all of the above drivers merely convert some
> crypto_skcipher to a crypto_tfm before passing it to xts_check_key().
>
> So I think these should all be made to call xts_verify_key() directly
> instead, the former xts_check_key() could then get dropped. But that's
> more of a cleanup IMO and would probably deserve a separate patch series
> on its own.
We should make sure both do the same thing though. So either
change all the drivers or just change xts_check_key in your patch
in addition to xts_verify_key.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt