Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Eric Biggers]

On Fri, Jul 14, 2017 at 09:24:40AM -0700, Michael Halcrow wrote:
> > +static int hkdf_expand(struct crypto_shash *hmac, u8 context,
> > +  const u8 *info, unsigned int infolen,
> > +  u8 *okm, unsigned int okmlen)
> > +{
> > +   SHASH_DESC_ON_STACK(desc, hmac);
> > +   int err;
> > +   const u8 *prev = NULL;
> > +   unsigned int i;
> > +   u8 counter = 1;
> > +   u8 tmp[HKDF_HASHLEN];
> > +
> > +   desc->tfm = hmac;
> > +   desc->flags = 0;
> > +
> > +   if (unlikely(okmlen > 255 * HKDF_HASHLEN))
> > +   return -EINVAL;
> > +
> > +   for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
> > +
> > +   err = crypto_shash_init(desc);
> > +   if (err)
> > +   goto out;
> > +
> > +   if (prev) {
> > +   err = crypto_shash_update(desc, prev, HKDF_HASHLEN);
> > +   if (err)
> > +   goto out;
> > +   }
> > +
> > +   err = crypto_shash_update(desc, , 1);
> 
> One potential shortcut would be to just increment context on each
> iteration rather than maintain the counter.
> 

That's not a good idea because then it wouldn't be standard HKDF, and it would
be relying on the "feedback" mode to keep the HMAC inputs unique which isn't
guaranteed to be sufficient.

> >  
> > -   res = validate_user_key(crypt_info, , raw_key, FS_KEY_DESC_PREFIX,
> > -   keysize);
> > -   if (res && inode->i_sb->s_cop->key_prefix) {
> > -   int res2 = validate_user_key(crypt_info, , raw_key,
> > -inode->i_sb->s_cop->key_prefix,
> > -keysize);
> > -   if (res2) {
> > -   if (res2 == -ENOKEY)
> > -   res = -ENOKEY;
> > +   if (ctx.version == FSCRYPT_CONTEXT_V1) {
> > +   res = find_and_derive_key_v1(inode, , derived_key,
> > +derived_keysize);
> 
> Why not make this consistent with the else clause, i.e. doing
> load_master_key_from_keyring() followed by derive_key_v1()?
> 

struct fscrypt_master_key contains the HMAC transform but not the raw master
key.  For the v1 key derivation we need the raw master key.  We could put it in
the fscrypt_master_key and then try to allow fscrypt_master_key's both with and
without HMAC transforms depending on the policy versions they are used for, but
there's no point in doing so currently.

Eric

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Michael Halcrow]

On Wed, Jul 12, 2017 at 02:00:32PM -0700, Eric Biggers wrote:
> From: Eric Biggers 
> 
> By design, the keys which userspace provides in the keyring are not used
> to encrypt data directly.  Instead, a KDF (Key Derivation Function) is
> used to derive a unique encryption key for each inode, given a "master"
> key and a nonce.  The current KDF encrypts the master key with
> AES-128-ECB using the nonce as the AES key.  This KDF is ad-hoc and is
> not specified in any standard.  While it does generate unique derived
> keys with sufficient entropy, it has several disadvantages:
> 
> - It's reversible: an attacker who compromises a derived key, e.g. using
>   a side channel attack, can "decrypt" it to get back to the master key.
> 
> - It's not very extensible because it cannot easily be used to derive
>   other key material that may be needed and it ties the length of the
>   derived key closely to the length of the master key.
> 
> - It doesn't evenly distribute the entropy from the master key.  For
>   example, the first 16 bytes of each derived key depend only on the
>   first 16 bytes of the master key.
> 
> - It uses a public value as an AES key, which is unusual.  Ciphers are
>   rarely evaluated under a threat model where the keys are public and
>   the messages are secret.
> 
> Solve all these problems for v2 encryption policies by changing the KDF
> to HKDF with SHA-512 as the underlying hash function.  To derive each
> inode's encryption key, HKDF is executed with the master key as the
> input key material, a fixed salt, and the per-inode nonce prefixed with
> a context byte as the application-specific information string.  Unlike
> the current KDF, HKDF has been formally published and standardized
> [1][2], is nonreversible, can be used to derive any number and length of
> secret and/or non-secret keys, and evenly distributes the entropy from
> the master key (excepting limits inherent to not using a random salt).
> 
> Note that this introduces a dependency on the security and
> implementation of SHA-512, whereas before we were using only AES for
> both key derivation and encryption.  However, by using HMAC rather than
> the hash function directly, HKDF is designed to remain secure even if
> various classes of attacks, e.g. collision attacks, are found against
> the underlying unkeyed hash function.  Even HMAC-MD5 is still considered
> secure in practice, despite MD5 itself having been heavily compromised.
> 
> We *could* avoid introducing a hash function by instantiating
> HKDF-Expand with CMAC-AES256 as the pseudorandom function rather than
> HMAC-SHA512.  This would work; however, the HKDF specification doesn't
> explicitly allow a non-HMAC pseudorandom function, so it would be less
> standard.  It would also require skipping HKDF-Extract and changing the
> API to accept only 32-byte master keys (since otherwise HKDF-Extract
> using CMAC-AES would produce a pseudorandom key only 16 bytes long which
> is only enough for AES-128, not AES-256).
> 
> HKDF-SHA512 can require more "crypto work" per key derivation when
> compared to the current KDF.  However, later in this series, we'll start
> caching the HMAC transform for each master key, which will actually make
> the real-world performance about the same or even significantly better
> than the AES-based KDF as currently implemented.  Also, each KDF can
> actually be executed on the order of 1 million times per second, so KDF
> performance probably isn't actually the bottleneck in practice anyway.
> 
> References:
>   [1] Krawczyk (2010). "Cryptographic Extraction and Key Derivation: The
>   HKDF Scheme".  https://eprint.iacr.org/2010/264.pdf
> 
>   [2] RFC 5869. "HMAC-based Extract-and-Expand Key Derivation Function
>   (HKDF)".  https://tools.ietf.org/html/rfc5869
> 
> Signed-off-by: Eric Biggers 
> ---
>  fs/crypto/Kconfig   |   2 +
>  fs/crypto/fscrypt_private.h |  14 ++
>  fs/crypto/keyinfo.c | 485 
> +++-
>  3 files changed, 405 insertions(+), 96 deletions(-)
> 
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index 02b7d91c9231..bbd4e38b293c 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -8,6 +8,8 @@ config FS_ENCRYPTION
>   select CRYPTO_CTS
>   select CRYPTO_CTR
>   select CRYPTO_SHA256
> + select CRYPTO_SHA512
> + select CRYPTO_HMAC
>   select KEYS
>   help
> Enable encryption of files and directories.  This
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 5470aac82cab..095e7c16483a 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -86,6 +86,14 @@ fscrypt_valid_context_format(const struct fscrypt_context 
> *ctx, int size)
>   return size >= 1 && size == fscrypt_context_size(ctx);
>  }
>  
> +/*
> + * fscrypt_master_key - an in-use master key
> + */
> +struct fscrypt_master_key {
> + struct crypto_shash   

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Stephan Müller]

Am Donnerstag, 13. Juli 2017, 20:10:57 CEST schrieb Eric Biggers:

Hi Eric,

> Hi Stephan,
> 
> On Thu, Jul 13, 2017 at 04:54:55PM +0200, Stephan Müller wrote:
> > Am Mittwoch, 12. Juli 2017, 23:00:32 CEST schrieb Eric Biggers:
> > 
> > Hi Herbert,
> > 
> > This patch adds a second KDF to the kernel -- the first is found in the
> > keys subsystem.
> > 
> > The next KDF that may come in is in the TLS scope.
> > 
> > Would it make sense to warm up the KDF patches adding generic KDF support
> > to the kernel crypto API that I supplied some time ago? The advantages
> > would be to have one location of KDF implementations and the benefit of
> > the testmgr.
> That may be a good idea.  Looking at the old thread, I share Herbert's
> concern (http://www.spinics.net/lists/linux-crypto/msg21231.html) about
> there likely not being more than one implementation of each KDF algorithm. 
> So, perhaps some simple helper functions would be more appropriate. 
> However, making the KDFs be covered by self-tests would be very nice.

I agree that it is likely that specific KDF implementations may only be used 
once. But still, I would recommend to maintain those implementation under the 
crypto API umbrella, as KDFs are cryptographic operations.

> 
> Also, it seems your patch
> (http://www.spinics.net/lists/linux-crypto/msg21137.html) doesn't allow a
> salt to be passed in.  In order to fully support HKDF, crypto_rng_reset()
> (which as I understand would be the way to invoke the "extract" step) would
> somehow need to accept both the input keying material and salt, both of
> which are arbitrary length binary.

I concur with you. I have implemented the HKDF in my libkcapi as well and saw 
the need for a salt.

Let me work on an update to the KDF patch for the kernel crypto API.

Ciao
Stephan

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Eric Biggers]

Hi Stephan,

On Thu, Jul 13, 2017 at 04:54:55PM +0200, Stephan Müller wrote:
> Am Mittwoch, 12. Juli 2017, 23:00:32 CEST schrieb Eric Biggers:
> 
> Hi Herbert,
> 
> This patch adds a second KDF to the kernel -- the first is found in the keys 
> subsystem.
> 
> The next KDF that may come in is in the TLS scope.
> 
> Would it make sense to warm up the KDF patches adding generic KDF support to 
> the kernel crypto API that I supplied some time ago? The advantages would be 
> to have one location of KDF implementations and the benefit of the testmgr.
> 

That may be a good idea.  Looking at the old thread, I share Herbert's concern
(http://www.spinics.net/lists/linux-crypto/msg21231.html) about there likely not
being more than one implementation of each KDF algorithm.  So, perhaps some
simple helper functions would be more appropriate.  However, making the KDFs be
covered by self-tests would be very nice.

Also, it seems your patch
(http://www.spinics.net/lists/linux-crypto/msg21137.html) doesn't allow a salt
to be passed in.  In order to fully support HKDF, crypto_rng_reset() (which as I
understand would be the way to invoke the "extract" step) would somehow need to
accept both the input keying material and salt, both of which are arbitrary
length binary.

Eric

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Stephan Müller]

Am Donnerstag, 13. Juli 2017, 18:07:54 CEST schrieb Herbert Xu:

Hi Herbert,

> Sure.  Though I'd like to see what it looks like before I commit :)

Naturally. :-)

The patches would create an RNG template support. KDFs are not more than 
special-purpose RNGs.

Ciao
Stephan

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Herbert Xu]

On Thu, Jul 13, 2017 at 04:54:55PM +0200, Stephan Müller wrote:
> Am Mittwoch, 12. Juli 2017, 23:00:32 CEST schrieb Eric Biggers:
> 
> Hi Herbert,
> 
> This patch adds a second KDF to the kernel -- the first is found in the keys 
> subsystem.
> 
> The next KDF that may come in is in the TLS scope.
> 
> Would it make sense to warm up the KDF patches adding generic KDF support to 
> the kernel crypto API that I supplied some time ago? The advantages would be 
> to have one location of KDF implementations and the benefit of the testmgr.

Sure.  Though I'd like to see what it looks like before I commit :)

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Stephan Müller]

Am Mittwoch, 12. Juli 2017, 23:00:32 CEST schrieb Eric Biggers:

Hi Herbert,

This patch adds a second KDF to the kernel -- the first is found in the keys 
subsystem.

The next KDF that may come in is in the TLS scope.

Would it make sense to warm up the KDF patches adding generic KDF support to 
the kernel crypto API that I supplied some time ago? The advantages would be 
to have one location of KDF implementations and the benefit of the testmgr.

Ciao
Stephan