Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

> On 15 Jun 2017, at 22:48, Eric Biggers  wrote:
> 
> On Thu, Jun 15, 2017 at 01:41:29PM -0700, Michael Halcrow wrote:
>>> static int validate_user_key(struct fscrypt_info *crypt_info,
>>> struct fscrypt_context *ctx, u8 *raw_key,
>>> -   const char *prefix)
>>> +   const char *prefix, int min_keysize)
>>> {
>>> char *description;
>>> struct key *keyring_key;
>>> @@ -111,50 +116,60 @@ static int validate_user_key(struct fscrypt_info 
>>> *crypt_info,
>>> master_key = (struct fscrypt_key *)ukp->data;
>>> BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>>> 
>>> -   if (master_key->size != FS_AES_256_XTS_KEY_SIZE) {
>>> +   if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
>>> +   || master_key->size % AES_BLOCK_SIZE != 0) {
>> 
>> I suggest validating the provided key size directly against the mode.
>> Else, it looks to me that this code will accept a 128-bit key for
>> AES-256.
>> 
> 
> It's doing that already; min_keysize depends on the mode.

We are a bit more forgiving than the code was before: In case AES-128-CBC is
selected, we accept a longer key and use the first 128 bits of the derived key.
(see fscrypt_get_encryption_info())

The alternative is to make this check as strict as it was and just check for
master_key->size != min_keysize.

IMO the current check is okay. I will however add a comment that documents this.
We could also add a pr_warn_once(), but I don't think this is really necessary.

David

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

> On 15 Jun 2017, at 22:48, Eric Biggers  wrote:
> 
> On Thu, Jun 15, 2017 at 01:41:29PM -0700, Michael Halcrow wrote:
>>> static int validate_user_key(struct fscrypt_info *crypt_info,
>>> struct fscrypt_context *ctx, u8 *raw_key,
>>> -   const char *prefix)
>>> +   const char *prefix, int min_keysize)
>>> {
>>> char *description;
>>> struct key *keyring_key;
>>> @@ -111,50 +116,60 @@ static int validate_user_key(struct fscrypt_info 
>>> *crypt_info,
>>> master_key = (struct fscrypt_key *)ukp->data;
>>> BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>>> 
>>> -   if (master_key->size != FS_AES_256_XTS_KEY_SIZE) {
>>> +   if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
>>> +   || master_key->size % AES_BLOCK_SIZE != 0) {
>> 
>> I suggest validating the provided key size directly against the mode.
>> Else, it looks to me that this code will accept a 128-bit key for
>> AES-256.
>> 
> 
> It's doing that already; min_keysize depends on the mode.

We are a bit more forgiving than the code was before: In case AES-128-CBC is
selected, we accept a longer key and use the first 128 bits of the derived key.
(see fscrypt_get_encryption_info())

The alternative is to make this check as strict as it was and just check for
master_key->size != min_keysize.

IMO the current check is okay. I will however add a comment that documents this.
We could also add a pr_warn_once(), but I don't think this is really necessary.

David

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[Eric Biggers]

On Thu, Jun 15, 2017 at 01:41:29PM -0700, Michael Halcrow wrote:
> >  static int validate_user_key(struct fscrypt_info *crypt_info,
> > struct fscrypt_context *ctx, u8 *raw_key,
> > -   const char *prefix)
> > +   const char *prefix, int min_keysize)
> >  {
> > char *description;
> > struct key *keyring_key;
> > @@ -111,50 +116,60 @@ static int validate_user_key(struct fscrypt_info 
> > *crypt_info,
> > master_key = (struct fscrypt_key *)ukp->data;
> > BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
> >  
> > -   if (master_key->size != FS_AES_256_XTS_KEY_SIZE) {
> > +   if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> > +   || master_key->size % AES_BLOCK_SIZE != 0) {
> 
> I suggest validating the provided key size directly against the mode.
> Else, it looks to me that this code will accept a 128-bit key for
> AES-256.
> 

It's doing that already; min_keysize depends on the mode.

Eric

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[Eric Biggers]

On Thu, Jun 15, 2017 at 01:41:29PM -0700, Michael Halcrow wrote:
> >  static int validate_user_key(struct fscrypt_info *crypt_info,
> > struct fscrypt_context *ctx, u8 *raw_key,
> > -   const char *prefix)
> > +   const char *prefix, int min_keysize)
> >  {
> > char *description;
> > struct key *keyring_key;
> > @@ -111,50 +116,60 @@ static int validate_user_key(struct fscrypt_info 
> > *crypt_info,
> > master_key = (struct fscrypt_key *)ukp->data;
> > BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
> >  
> > -   if (master_key->size != FS_AES_256_XTS_KEY_SIZE) {
> > +   if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> > +   || master_key->size % AES_BLOCK_SIZE != 0) {
> 
> I suggest validating the provided key size directly against the mode.
> Else, it looks to me that this code will accept a 128-bit key for
> AES-256.
> 

It's doing that already; min_keysize depends on the mode.

Eric

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[Michael Halcrow]

On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
> From: Daniel Walter 
> 
> fscrypt provides facilities to use different encryption algorithms which
> are selectable by userspace when setting the encryption policy. Currently,
> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
> implemented. This is a clear case of kernel offers the mechanism and
> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
> 
> This patch adds support for using AES-128-CBC for file contents and
> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
> actually slightly less secure than AES-XTS from a security point of view,
> there is more widespread hardware support. Using AES-CBC gives us the
> acceptable performance while still providing a moderate level of security
> for persistent storage.
> 
> Especially low-powered embedded devices with crypto accelerators such as
> CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
> is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
> since it has less encryption rounds and yields noticeable better
> performance starting from a file size of just a few kB.
> 
> Signed-off-by: Daniel Walter 
> [da...@sigma-star.at: addressed review comments]
> Signed-off-by: David Gstir 
> ---
>  fs/crypto/Kconfig  |   1 +
>  fs/crypto/crypto.c |  23 --
>  fs/crypto/fscrypt_private.h|   9 ++-
>  fs/crypto/keyinfo.c| 171 
> -
>  fs/crypto/policy.c |   8 +-
>  include/linux/fscrypt_common.h |  16 ++--
>  include/uapi/linux/fs.h|   2 +
>  7 files changed, 172 insertions(+), 58 deletions(-)
> 
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index 08b46e6e3995..02b7d91c9231 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -7,6 +7,7 @@ config FS_ENCRYPTION
>   select CRYPTO_XTS
>   select CRYPTO_CTS
>   select CRYPTO_CTR
> + select CRYPTO_SHA256
>   select KEYS
>   help
> Enable encryption of files and directories.  This
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index 6d6eca394d4d..c7835df7e7b8 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "fscrypt_private.h"
>  
>  static unsigned int num_prealloc_crypto_pages = 32;
> @@ -147,8 +148,8 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
> fscrypt_direction_t rw,
>  {
>   struct {
>   __le64 index;
> - u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
> - } xts_tweak;
> + u8 padding[FS_IV_SIZE - sizeof(__le64)];
> + } iv;
>   struct skcipher_request *req = NULL;
>   DECLARE_FS_COMPLETION_RESULT(ecr);
>   struct scatterlist dst, src;
> @@ -158,6 +159,16 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
> fscrypt_direction_t rw,
>  
>   BUG_ON(len == 0);
>  
> + BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
> + BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
> + iv.index = cpu_to_le64(lblk_num);
> + memset(iv.padding, 0, sizeof(iv.padding));
> +
> + if (ci->ci_essiv_tfm != NULL) {
> + crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
> +   (u8 *));
> + }
> +
>   req = skcipher_request_alloc(tfm, gfp_flags);
>   if (!req) {
>   printk_ratelimited(KERN_ERR
> @@ -170,15 +181,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
> fscrypt_direction_t rw,
>   req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
>   page_crypt_complete, );
>  
> - BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
> - xts_tweak.index = cpu_to_le64(lblk_num);
> - memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
> -
>   sg_init_table(, 1);
>   sg_set_page(, dest_page, len, offs);
>   sg_init_table(, 1);
>   sg_set_page(, src_page, len, offs);
> - skcipher_request_set_crypt(req, , , len, _tweak);
> + skcipher_request_set_crypt(req, , , len, );
>   if (rw == FS_DECRYPT)
>   res = crypto_skcipher_decrypt(req);
>   else
> @@ -477,6 +484,8 @@ static void __exit fscrypt_exit(void)
>   destroy_workqueue(fscrypt_read_workqueue);
>   kmem_cache_destroy(fscrypt_ctx_cachep);
>   kmem_cache_destroy(fscrypt_info_cachep);
> +
> + fscrypt_essiv_cleanup();
>  }
>  module_exit(fscrypt_exit);
>  
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 1e1f8a361b75..a1d5021c31ef 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -12,10 +12,13 @@
>  #define _FSCRYPT_PRIVATE_H
>  
>  #include 
> +#include 
>  
>  /* Encryption parameters */
> 

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[Michael Halcrow]

On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
> From: Daniel Walter 
> 
> fscrypt provides facilities to use different encryption algorithms which
> are selectable by userspace when setting the encryption policy. Currently,
> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
> implemented. This is a clear case of kernel offers the mechanism and
> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
> 
> This patch adds support for using AES-128-CBC for file contents and
> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
> actually slightly less secure than AES-XTS from a security point of view,
> there is more widespread hardware support. Using AES-CBC gives us the
> acceptable performance while still providing a moderate level of security
> for persistent storage.
> 
> Especially low-powered embedded devices with crypto accelerators such as
> CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
> is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
> since it has less encryption rounds and yields noticeable better
> performance starting from a file size of just a few kB.
> 
> Signed-off-by: Daniel Walter 
> [da...@sigma-star.at: addressed review comments]
> Signed-off-by: David Gstir 
> ---
>  fs/crypto/Kconfig  |   1 +
>  fs/crypto/crypto.c |  23 --
>  fs/crypto/fscrypt_private.h|   9 ++-
>  fs/crypto/keyinfo.c| 171 
> -
>  fs/crypto/policy.c |   8 +-
>  include/linux/fscrypt_common.h |  16 ++--
>  include/uapi/linux/fs.h|   2 +
>  7 files changed, 172 insertions(+), 58 deletions(-)
> 
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index 08b46e6e3995..02b7d91c9231 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -7,6 +7,7 @@ config FS_ENCRYPTION
>   select CRYPTO_XTS
>   select CRYPTO_CTS
>   select CRYPTO_CTR
> + select CRYPTO_SHA256
>   select KEYS
>   help
> Enable encryption of files and directories.  This
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index 6d6eca394d4d..c7835df7e7b8 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "fscrypt_private.h"
>  
>  static unsigned int num_prealloc_crypto_pages = 32;
> @@ -147,8 +148,8 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
> fscrypt_direction_t rw,
>  {
>   struct {
>   __le64 index;
> - u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
> - } xts_tweak;
> + u8 padding[FS_IV_SIZE - sizeof(__le64)];
> + } iv;
>   struct skcipher_request *req = NULL;
>   DECLARE_FS_COMPLETION_RESULT(ecr);
>   struct scatterlist dst, src;
> @@ -158,6 +159,16 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
> fscrypt_direction_t rw,
>  
>   BUG_ON(len == 0);
>  
> + BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
> + BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
> + iv.index = cpu_to_le64(lblk_num);
> + memset(iv.padding, 0, sizeof(iv.padding));
> +
> + if (ci->ci_essiv_tfm != NULL) {
> + crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
> +   (u8 *));
> + }
> +
>   req = skcipher_request_alloc(tfm, gfp_flags);
>   if (!req) {
>   printk_ratelimited(KERN_ERR
> @@ -170,15 +181,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
> fscrypt_direction_t rw,
>   req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
>   page_crypt_complete, );
>  
> - BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
> - xts_tweak.index = cpu_to_le64(lblk_num);
> - memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
> -
>   sg_init_table(, 1);
>   sg_set_page(, dest_page, len, offs);
>   sg_init_table(, 1);
>   sg_set_page(, src_page, len, offs);
> - skcipher_request_set_crypt(req, , , len, _tweak);
> + skcipher_request_set_crypt(req, , , len, );
>   if (rw == FS_DECRYPT)
>   res = crypto_skcipher_decrypt(req);
>   else
> @@ -477,6 +484,8 @@ static void __exit fscrypt_exit(void)
>   destroy_workqueue(fscrypt_read_workqueue);
>   kmem_cache_destroy(fscrypt_ctx_cachep);
>   kmem_cache_destroy(fscrypt_info_cachep);
> +
> + fscrypt_essiv_cleanup();
>  }
>  module_exit(fscrypt_exit);
>  
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 1e1f8a361b75..a1d5021c31ef 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -12,10 +12,13 @@
>  #define _FSCRYPT_PRIVATE_H
>  
>  #include 
> +#include 
>  
>  /* Encryption parameters */
> -#define FS_XTS_TWEAK_SIZE16
> +#define FS_IV_SIZE 

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[Theodore Ts'o]

On Wed, May 31, 2017 at 05:57:22PM +0200, David Gstir wrote:
> > The 'keysize > sizeof(salt)' check is now pointless and should be removed, 
> > since
> > we decided not to key the ESSIV cipher with 'keysize' bytes, but rather with
> > sizeof(salt) bytes.  So this function is compatible with any 'keysize', not 
> > just
> > keysize <= sizeof(salt).
> 
> You're right. Just let me know if I should send a new version of this patch 
> with these minor issues fixed.

Please do, thanks!

- Ted

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[Theodore Ts'o]

On Wed, May 31, 2017 at 05:57:22PM +0200, David Gstir wrote:
> > The 'keysize > sizeof(salt)' check is now pointless and should be removed, 
> > since
> > we decided not to key the ESSIV cipher with 'keysize' bytes, but rather with
> > sizeof(salt) bytes.  So this function is compatible with any 'keysize', not 
> > just
> > keysize <= sizeof(salt).
> 
> You're right. Just let me know if I should send a new version of this patch 
> with these minor issues fixed.

Please do, thanks!

- Ted

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

Hi Eric,

> On 23 May 2017, at 21:00, Eric Biggers  wrote:
> 
> Hi David,
> 
> On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
>> From: Daniel Walter 
>> 
>> fscrypt provides facilities to use different encryption algorithms which
>> are selectable by userspace when setting the encryption policy. Currently,
>> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
>> implemented. This is a clear case of kernel offers the mechanism and
>> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
>> 
>> This patch adds support for using AES-128-CBC for file contents and
>> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
>> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
>> actually slightly less secure than AES-XTS from a security point of view,
>> there is more widespread hardware support. Using AES-CBC gives us the
>> acceptable performance while still providing a moderate level of security
>> for persistent storage.
>> 
>> Especially low-powered embedded devices with crypto accelerators such as
>> CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
>> is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
>> since it has less encryption rounds and yields noticeable better
>> performance starting from a file size of just a few kB.
>> 
>> Signed-off-by: Daniel Walter 
>> [da...@sigma-star.at: addressed review comments]
>> Signed-off-by: David Gstir 
> 
> Overall this looks good now; you can add
> 
> Reviewed-by: Eric Biggers 

Thanks! :)


> I did notice a couple minor improvements that can be made, though:
> 
>> 
>> +if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
>> +res = init_essiv_generator(crypt_info, raw_key, keysize);
>> +if (res) {
>> +pr_debug("%s: error %d (inode %lu) allocating essiv 
>> tfm\n",
>> + __func__, res, inode->i_ino);
>> +goto out;
>> +}
>> +}
> 
> Since the ESSIV generator is only needed for contents encryption, it should 
> only
> be initialized when both 'S_ISREG(inode->i_mode) && crypt_info->ci_data_mode 
> ==
> FS_ENCRYPTION_MODE_AES_128_CBC'.  Otherwise ->ci_essiv_tfm will be allocated 
> for
> directories and symlinks too, then never used.
> 
>> +static int init_essiv_generator(struct fscrypt_info *ci, const u8 *raw_key,
>> +int keysize)
>> +{
>> +int err;
>> +struct crypto_cipher *essiv_tfm;
>> +u8 salt[SHA256_DIGEST_SIZE];
>> +
>> +if (WARN_ON_ONCE(keysize > sizeof(salt)))
>> +return -EINVAL;
>> +
> 
> The 'keysize > sizeof(salt)' check is now pointless and should be removed, 
> since
> we decided not to key the ESSIV cipher with 'keysize' bytes, but rather with
> sizeof(salt) bytes.  So this function is compatible with any 'keysize', not 
> just
> keysize <= sizeof(salt).

You're right. Just let me know if I should send a new version of this patch 
with these minor issues fixed.


> You should also consider how it should be made possible to test these new
> encryption modes in xfstests.  Currently, while the "set_encpolicy" xfs_io
> command allows specifying different encryption modes and flags, in general the
> tests in the "encrypt" group are hardcoded to use AES_256_XTS and AES_256_CTS.
> Similarly, those modes are also used with the test_dummy_encryption mount
> option, which causes all new files to be automatically encrypted, and is used 
> by
> the "encrypt" config for kvm-xfstests and gce-xfstests (currently 
> ext4-specific,
> but other filesystems could support it too).

Sure! I'll do that.

Thanks,
David

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

Hi Eric,

> On 23 May 2017, at 21:00, Eric Biggers  wrote:
> 
> Hi David,
> 
> On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
>> From: Daniel Walter 
>> 
>> fscrypt provides facilities to use different encryption algorithms which
>> are selectable by userspace when setting the encryption policy. Currently,
>> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
>> implemented. This is a clear case of kernel offers the mechanism and
>> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
>> 
>> This patch adds support for using AES-128-CBC for file contents and
>> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
>> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
>> actually slightly less secure than AES-XTS from a security point of view,
>> there is more widespread hardware support. Using AES-CBC gives us the
>> acceptable performance while still providing a moderate level of security
>> for persistent storage.
>> 
>> Especially low-powered embedded devices with crypto accelerators such as
>> CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
>> is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
>> since it has less encryption rounds and yields noticeable better
>> performance starting from a file size of just a few kB.
>> 
>> Signed-off-by: Daniel Walter 
>> [da...@sigma-star.at: addressed review comments]
>> Signed-off-by: David Gstir 
> 
> Overall this looks good now; you can add
> 
> Reviewed-by: Eric Biggers 

Thanks! :)


> I did notice a couple minor improvements that can be made, though:
> 
>> 
>> +if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
>> +res = init_essiv_generator(crypt_info, raw_key, keysize);
>> +if (res) {
>> +pr_debug("%s: error %d (inode %lu) allocating essiv 
>> tfm\n",
>> + __func__, res, inode->i_ino);
>> +goto out;
>> +}
>> +}
> 
> Since the ESSIV generator is only needed for contents encryption, it should 
> only
> be initialized when both 'S_ISREG(inode->i_mode) && crypt_info->ci_data_mode 
> ==
> FS_ENCRYPTION_MODE_AES_128_CBC'.  Otherwise ->ci_essiv_tfm will be allocated 
> for
> directories and symlinks too, then never used.
> 
>> +static int init_essiv_generator(struct fscrypt_info *ci, const u8 *raw_key,
>> +int keysize)
>> +{
>> +int err;
>> +struct crypto_cipher *essiv_tfm;
>> +u8 salt[SHA256_DIGEST_SIZE];
>> +
>> +if (WARN_ON_ONCE(keysize > sizeof(salt)))
>> +return -EINVAL;
>> +
> 
> The 'keysize > sizeof(salt)' check is now pointless and should be removed, 
> since
> we decided not to key the ESSIV cipher with 'keysize' bytes, but rather with
> sizeof(salt) bytes.  So this function is compatible with any 'keysize', not 
> just
> keysize <= sizeof(salt).

You're right. Just let me know if I should send a new version of this patch 
with these minor issues fixed.


> You should also consider how it should be made possible to test these new
> encryption modes in xfstests.  Currently, while the "set_encpolicy" xfs_io
> command allows specifying different encryption modes and flags, in general the
> tests in the "encrypt" group are hardcoded to use AES_256_XTS and AES_256_CTS.
> Similarly, those modes are also used with the test_dummy_encryption mount
> option, which causes all new files to be automatically encrypted, and is used 
> by
> the "encrypt" config for kvm-xfstests and gce-xfstests (currently 
> ext4-specific,
> but other filesystems could support it too).

Sure! I'll do that.

Thanks,
David

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[Eric Biggers]

Hi David,

On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
> From: Daniel Walter 
> 
> fscrypt provides facilities to use different encryption algorithms which
> are selectable by userspace when setting the encryption policy. Currently,
> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
> implemented. This is a clear case of kernel offers the mechanism and
> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
> 
> This patch adds support for using AES-128-CBC for file contents and
> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
> actually slightly less secure than AES-XTS from a security point of view,
> there is more widespread hardware support. Using AES-CBC gives us the
> acceptable performance while still providing a moderate level of security
> for persistent storage.
> 
> Especially low-powered embedded devices with crypto accelerators such as
> CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
> is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
> since it has less encryption rounds and yields noticeable better
> performance starting from a file size of just a few kB.
> 
> Signed-off-by: Daniel Walter 
> [da...@sigma-star.at: addressed review comments]
> Signed-off-by: David Gstir 

Overall this looks good now; you can add

Reviewed-by: Eric Biggers 

I did notice a couple minor improvements that can be made, though:

>  
> + if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
> + res = init_essiv_generator(crypt_info, raw_key, keysize);
> + if (res) {
> + pr_debug("%s: error %d (inode %lu) allocating essiv 
> tfm\n",
> +  __func__, res, inode->i_ino);
> + goto out;
> + }
> + }

Since the ESSIV generator is only needed for contents encryption, it should only
be initialized when both 'S_ISREG(inode->i_mode) && crypt_info->ci_data_mode ==
FS_ENCRYPTION_MODE_AES_128_CBC'.  Otherwise ->ci_essiv_tfm will be allocated for
directories and symlinks too, then never used.

> +static int init_essiv_generator(struct fscrypt_info *ci, const u8 *raw_key,
> + int keysize)
> +{
> + int err;
> + struct crypto_cipher *essiv_tfm;
> + u8 salt[SHA256_DIGEST_SIZE];
> +
> + if (WARN_ON_ONCE(keysize > sizeof(salt)))
> + return -EINVAL;
> +

The 'keysize > sizeof(salt)' check is now pointless and should be removed, since
we decided not to key the ESSIV cipher with 'keysize' bytes, but rather with
sizeof(salt) bytes.  So this function is compatible with any 'keysize', not just
keysize <= sizeof(salt).

You should also consider how it should be made possible to test these new
encryption modes in xfstests.  Currently, while the "set_encpolicy" xfs_io
command allows specifying different encryption modes and flags, in general the
tests in the "encrypt" group are hardcoded to use AES_256_XTS and AES_256_CTS.
Similarly, those modes are also used with the test_dummy_encryption mount
option, which causes all new files to be automatically encrypted, and is used by
the "encrypt" config for kvm-xfstests and gce-xfstests (currently ext4-specific,
but other filesystems could support it too).

Eric

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[Eric Biggers]

Hi David,

On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
> From: Daniel Walter 
> 
> fscrypt provides facilities to use different encryption algorithms which
> are selectable by userspace when setting the encryption policy. Currently,
> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
> implemented. This is a clear case of kernel offers the mechanism and
> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
> 
> This patch adds support for using AES-128-CBC for file contents and
> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
> actually slightly less secure than AES-XTS from a security point of view,
> there is more widespread hardware support. Using AES-CBC gives us the
> acceptable performance while still providing a moderate level of security
> for persistent storage.
> 
> Especially low-powered embedded devices with crypto accelerators such as
> CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
> is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
> since it has less encryption rounds and yields noticeable better
> performance starting from a file size of just a few kB.
> 
> Signed-off-by: Daniel Walter 
> [da...@sigma-star.at: addressed review comments]
> Signed-off-by: David Gstir 

Overall this looks good now; you can add

Reviewed-by: Eric Biggers 

I did notice a couple minor improvements that can be made, though:

>  
> + if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
> + res = init_essiv_generator(crypt_info, raw_key, keysize);
> + if (res) {
> + pr_debug("%s: error %d (inode %lu) allocating essiv 
> tfm\n",
> +  __func__, res, inode->i_ino);
> + goto out;
> + }
> + }

Since the ESSIV generator is only needed for contents encryption, it should only
be initialized when both 'S_ISREG(inode->i_mode) && crypt_info->ci_data_mode ==
FS_ENCRYPTION_MODE_AES_128_CBC'.  Otherwise ->ci_essiv_tfm will be allocated for
directories and symlinks too, then never used.

> +static int init_essiv_generator(struct fscrypt_info *ci, const u8 *raw_key,
> + int keysize)
> +{
> + int err;
> + struct crypto_cipher *essiv_tfm;
> + u8 salt[SHA256_DIGEST_SIZE];
> +
> + if (WARN_ON_ONCE(keysize > sizeof(salt)))
> + return -EINVAL;
> +

The 'keysize > sizeof(salt)' check is now pointless and should be removed, since
we decided not to key the ESSIV cipher with 'keysize' bytes, but rather with
sizeof(salt) bytes.  So this function is compatible with any 'keysize', not just
keysize <= sizeof(salt).

You should also consider how it should be made possible to test these new
encryption modes in xfstests.  Currently, while the "set_encpolicy" xfs_io
command allows specifying different encryption modes and flags, in general the
tests in the "encrypt" group are hardcoded to use AES_256_XTS and AES_256_CTS.
Similarly, those modes are also used with the test_dummy_encryption mount
option, which causes all new files to be automatically encrypted, and is used by
the "encrypt" config for kvm-xfstests and gce-xfstests (currently ext4-specific,
but other filesystems could support it too).

Eric