Re: [PATCH 1/6] fscrypt: add v2 encryption context and policy

[Andreas Dilger]

On Jul 13, 2017, at 3:58 PM, Eric Biggers  wrote:
> 
> Hi Michael,
> 
> On Thu, Jul 13, 2017 at 03:29:44PM -0700, Michael Halcrow wrote:
>> On Wed, Jul 12, 2017 at 02:00:30PM -0700, Eric Biggers wrote:
>>> From: Eric Biggers 
>>> 
>>> Currently, the fscrypt_context (i.e. the encryption xattr) does not
>>> contain a cryptographically secure identifier for the master key's
>>> payload.  Therefore it's not possible to verify that the correct key was
>>> supplied, which is problematic in multi-user scenarios.  To make this
>>> possible, define a new fscrypt_context version (v2) which includes a
>>> key_hash field, and allow userspace to opt-in to it when setting an
>>> encryption policy by setting fscrypt_policy.version to 2.  For now just
>>> zero the new field; a later patch will start setting it for real.
>> 
>> The main concern that comes to mind is potentially blowing past the
>> inline xattr size limit and allocating a new inode block.  The
>> security benefit probably outweighs that concern in this case.
>> 
> 
> The way it adds up now for ext4 is:
> 
> 128 bytes for base inode
> + 32 bytes for i_extra fields
> + 4 bytes for in-inode xattrs header
> + 20 bytes for encryption xattr header + name
> + 28 bytes for encryption xattr value
> --
> = 212 bytes total.
> 
> By adding the 16-byte 'key_hash' field it grows to 228 bytes total.  So it 
> still
> fits in a 256-byte inode, though it's getting closer to the limit.  We could
> save 8 bytes by instead using the design where master_key_descriptor is 
> extended
> to 16 bytes and redefined as a cryptographically secure hash.  But as noted,
> that has some significant disadvantages.
> 
> Also note that we don't really have to worry about leaving space for a SELinux
> xattr anymore because with 256-byte inodes + encryption the SELinux xattr is
> already being written to an external block, given that it requires about 52-62
> bytes (at least when using Android's SELinux policy; different SELinux 
> policies
> may use different values), and 212 + 52 > 256.  So if someone wants both 
> xattrs
> in-inode they need to use 512-byte inodes already.

It is probably time to consider changing to a default of 512-byte inodes for
larger filesystems anyway.  In our testing, this affected performance only by
a couple of percent under normal usage, and avoided a significant performance
drop if the xattrs ever fall out of the inode.

Cheers, Andreas







signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 4/6] fscrypt: verify that the correct master key was supplied

[Michael Halcrow]

On Wed, Jul 12, 2017 at 02:00:33PM -0700, Eric Biggers wrote:
> From: Eric Biggers 
> 
> Currently, while a fscrypt master key is required to have a certain
> description in the keyring, its payload is never verified to be correct.
> While sufficient for well-behaved userspace, this is insecure in a
> multi-user system where a user has been given only read-only access to
> an encrypted file or directory.  Specifically, if an encrypted file or
> directory does not yet have its key cached by the kernel, the first user
> who accesses it can provide an arbitrary key in their own keyring, which
> the kernel will then associate with the inode and use for read(),
> write(), readdir(), etc. by other users as well.
> 
> Consequently, it's trivial for a user with *read-only* access to an
> encrypted file or directory to make it appear as garbage to everyone.
> Creative users might be able to accomplish more sophisticated attacks by
> careful choice of the key, e.g. choosing a key causes certain bytes of
> file contents to have certain values or that causes filenames containing
> the '/' character to appear.
> 
> Solve the problem for v2 encryption policies by storing a "hash" of the
> master encryption key in the encryption xattr and verifying it before
> accepting the user-provided key.  We generate the "hash" using
> HKDF-SHA512 by passing a distinct application-specific info string.
> This produces a value which is cryptographically isolated and can be
> stored in the clear without leaking any information about the master key
> or any other derived keys (in a computational sense).  Reusing HKDF is
> better than doing e.g. SHA-512(master_key) because it avoids passing the
> same key into different cryptographic primitives.
> 
> We make the hash field 16 bytes long, as this should provide sufficient
> collision and preimage resistance while not wasting too much space for
> the encryption xattr.
> 
> Signed-off-by: Eric Biggers 

Acked-by: Michael Halcrow 

> ---
>  fs/crypto/fscrypt_private.h |  4 
>  fs/crypto/keyinfo.c | 46 +
>  fs/crypto/policy.c  | 55 
> -
>  3 files changed, 95 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 095e7c16483a..a7baeac92575 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -92,6 +92,7 @@ fscrypt_valid_context_format(const struct fscrypt_context 
> *ctx, int size)
>  struct fscrypt_master_key {
>   struct crypto_shash *mk_hmac;
>   unsigned intmk_size;
> + u8  mk_hash[FSCRYPT_KEY_HASH_SIZE];
>  };
>  
>  /*
> @@ -155,6 +156,9 @@ extern struct page *fscrypt_alloc_bounce_page(struct 
> fscrypt_ctx *ctx,
> gfp_t gfp_flags);
>  
>  /* keyinfo.c */
> +extern int fscrypt_compute_key_hash(const struct inode *inode,
> + const struct fscrypt_policy *policy,
> + u8 hash[FSCRYPT_KEY_HASH_SIZE]);
>  extern void __exit fscrypt_essiv_cleanup(void);
>  
>  #endif /* _FSCRYPT_PRIVATE_H */
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 7ed1a7fb1308..12a60eacf819 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -39,8 +39,11 @@ static struct crypto_shash *essiv_hash_tfm;
>   *
>   * Keys derived with different info strings are cryptographically isolated 
> from
>   * each other --- knowledge of one derived key doesn't reveal any others.
> + * (This property is particularly important for the derived key used as the
> + * "key hash", as that is stored in the clear.)
>   */
>  #define HKDF_CONTEXT_PER_FILE_KEY1
> +#define HKDF_CONTEXT_KEY_HASH2
>  
>  /*
>   * HKDF consists of two steps:
> @@ -212,6 +215,12 @@ alloc_master_key(const struct fscrypt_key *payload)
>   err = crypto_shash_setkey(k->mk_hmac, prk, sizeof(prk));
>   if (err)
>   goto fail;
> +
> + /* Calculate the "key hash" */
> + err = hkdf_expand(k->mk_hmac, HKDF_CONTEXT_KEY_HASH, NULL, 0,
> +   k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
> + if (err)
> + goto fail;
>  out:
>   memzero_explicit(prk, sizeof(prk));
>   return k;
> @@ -537,6 +546,31 @@ void __exit fscrypt_essiv_cleanup(void)
>   crypto_free_shash(essiv_hash_tfm);
>  }
>  
> +int fscrypt_compute_key_hash(const struct inode *inode,
> +  const struct fscrypt_policy *policy,
> +  u8 hash[FSCRYPT_KEY_HASH_SIZE])
> +{
> + struct fscrypt_master_key *k;
> + unsigned int min_keysize;
> +
> + /*
> +  * Require that the master key be long enough for both the
> +  * contents and filenames encryption modes.
> +  */
> + min_keysize =
> + 

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 2/6] fscrypt: rename ->ci_master_key to ->ci_master_key_descriptor

[Michael Halcrow]

On Wed, Jul 12, 2017 at 02:00:31PM -0700, Eric Biggers wrote:
> From: Eric Biggers 
> 
> In struct fscrypt_info, ->ci_master_key is the master key descriptor,
> not the master key itself.  In preparation for introducing a struct
> fscrypt_master_key and making ->ci_master_key point to it, rename the
> existing ->ci_master_key to ->ci_master_key_descriptor.
> 
> Signed-off-by: Eric Biggers 

Acked-by: Michael Halcrow 

> ---
>  fs/crypto/fscrypt_private.h | 2 +-
>  fs/crypto/keyinfo.c | 4 ++--
>  fs/crypto/policy.c  | 5 +++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index ef6909035823..5470aac82cab 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -107,7 +107,7 @@ struct fscrypt_info {
>   u8 ci_data_mode;
>   u8 ci_filename_mode;
>   u8 ci_flags;
> - u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
> + u8 ci_master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
>  };
>  
>  typedef enum {
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 7e664a11340a..5591fd24e4b2 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -293,8 +293,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
>   crypt_info->ci_data_mode = ctx.contents_encryption_mode;
>   crypt_info->ci_filename_mode = ctx.filenames_encryption_mode;
>   crypt_info->ci_flags = ctx.flags;
> - memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
> - sizeof(crypt_info->ci_master_key));
> + memcpy(crypt_info->ci_master_key_descriptor, ctx.master_key_descriptor,
> +FS_KEY_DESCRIPTOR_SIZE);
>  
>   res = determine_cipher_type(crypt_info, inode, _str, );
>   if (res)
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 044f23fadb5a..81c59f8e45c0 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -224,7 +224,8 @@ int fscrypt_has_permitted_context(struct inode *parent, 
> struct inode *child)
>   child_ci = child->i_crypt_info;
>  
>   if (parent_ci && child_ci) {
> - return memcmp(parent_ci->ci_master_key, child_ci->ci_master_key,
> + return memcmp(parent_ci->ci_master_key_descriptor,
> +   child_ci->ci_master_key_descriptor,
> FS_KEY_DESCRIPTOR_SIZE) == 0 &&
>   (parent_ci->ci_context_version ==
>child_ci->ci_context_version) &&
> @@ -282,7 +283,7 @@ int fscrypt_inherit_context(struct inode *parent, struct 
> inode *child,
>   ctx.contents_encryption_mode = ci->ci_data_mode;
>   ctx.filenames_encryption_mode = ci->ci_filename_mode;
>   ctx.flags = ci->ci_flags;
> - memcpy(ctx.master_key_descriptor, ci->ci_master_key,
> + memcpy(ctx.master_key_descriptor, ci->ci_master_key_descriptor,
>  FS_KEY_DESCRIPTOR_SIZE);
>   get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
>   if (ctx.version != FSCRYPT_CONTEXT_V1)
> -- 
> 2.13.2.932.g7449e964c-goog
> 

Re: [PATCH 00/12] staging: ccree: coding style fixes

[Gilad Ben-Yossef]

On Fri, Jul 14, 2017 at 2:39 PM, Greg Kroah-Hartman
 wrote:
> On Thu, Jul 13, 2017 at 11:19:50AM +0300, Gilad Ben-Yossef wrote:
>> Another batch of ccree coding style fixes.
>>
>> These goes on top of commit a8c4ae12 ("staging: ccree: Fix alignment issues 
>> in ssi_sysfs.c")
>> in staging-testing.
>
> Odd, some of these did not apply, but others did.  Please rebase your
> series and resend what I didn't pick up this time.

That is weird.

I am away about to board a flight and will be away for few days but
will check up on this when I'm back.

Sorry about that and 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

Crypto Fixes for 4.13

[Herbert Xu]

Hi Linus: 

This push fixes the following issues:

- New compiler warnings in cavium.
- Set post-op IV properly in caam (this fixes chaining).
- Fix potential use-after-free in atmel in case of EBUSY.
- Fix sleeping in softirq path in chcr.
- Disable buggy sha1-avx2 driver (may overread and page fault).
- Fix use-after-free on signals in caam.
 

Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus


Colin Ian King (1):
  crypto: cavium - make several functions static

David Gstir (1):
  crypto: caam - properly set IV after {en,de}crypt

Gilad Ben-Yossef (1):
  crypto: atmel - only treat EBUSY as transient if backlog

Harsh Jain (1):
  crypto: chcr - Avoid algo allocation in softirq.

Herbert Xu (2):
  crypto: sha1-ssse3 - Disable avx2

Horia Geantă (1):
  crypto: caam - fix signals handling

 arch/x86/crypto/sha1_ssse3_glue.c  |2 +-
 crypto/af_alg.c|2 +-
 drivers/crypto/atmel-sha.c |4 +++-
 drivers/crypto/caam/caamalg.c  |   20 ++--
 drivers/crypto/caam/caamhash.c |2 +-
 drivers/crypto/caam/key_gen.c  |2 +-
 drivers/crypto/cavium/cpt/cptvf_algs.c |8 
 drivers/crypto/chelsio/chcr_algo.c |   23 +++
 drivers/crypto/chelsio/chcr_crypto.h   |1 +
 9 files changed, 45 insertions(+), 19 deletions(-)

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

Re: [PATCH 00/12] staging: ccree: coding style fixes

[Greg Kroah-Hartman]

On Thu, Jul 13, 2017 at 11:19:50AM +0300, Gilad Ben-Yossef wrote:
> Another batch of ccree coding style fixes.
> 
> These goes on top of commit a8c4ae12 ("staging: ccree: Fix alignment issues 
> in ssi_sysfs.c")
> in staging-testing.

Odd, some of these did not apply, but others did.  Please rebase your
series and resend what I didn't pick up this time.

thanks,

greg k-h

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Fri, Jun 23, 2017 at 02:48:37PM +0800, Herbert Xu wrote:
> On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
> >
> > Since there are two different user of "crypto engine + ablkcipher", it will 
> > be not easy to convert them in one serie. (I could do it, but I simply 
> > could not test it for OMAP (lack of hw))
> > And any new user which want to use crypto engine+skcipher (like me with the 
> > sun8i-ce driver) are simply stuck.
> 
> You're right.  We'll need to do this in a backwards-compatible way.  In fact
> we already do something similar in skcipher.c itself.  Simply look at the
> cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
> legacy ablkcipher, otherwise it's skcipher.
> 
> Also the way crypto_engine looks at the request type in the data-path is
> suboptimal.  This should really be built into the cra_type object.  For
> example, we can have cra_type->engine->prepare_request which would just
> do the right thing.
> 

Not sure to have well understand what you want.
You want that I switch on cra_type instead of crypto_tfm_alg_type() ?