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

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

[Eric Biggers]

[+Cc linux-fscrypt]

Hi David and Daniel,

On Thu, Mar 30, 2017 at 07:38:40PM +0200, David Gstir wrote:
> From: Daniel Walter <dwalter@sigma-star.at>
> 
> 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.
> Which 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. Especially low-powered embedded devices crypto
> accelerators such as CAAM or CESA support only AES-CBC-128 with an acceptable
> speed. Using AES-CBC gives us the acceptable performance while still providing
> a moderate level of security for persistent storage.
> 

Thanks for sending this!  I can't object too much to adding AES-CBC-128 if you
find it useful, though of course AES-256-XTS will remain the recommendation for
general use.  And I don't suppose AES-256-CBC is an option for you?

Anyway, more comments below:

> @@ -147,17 +148,31 @@ 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)];
> +	} blk_desc;
>  	struct skcipher_request *req = NULL;
>  	DECLARE_FS_COMPLETION_RESULT(ecr);
>  	struct scatterlist dst, src;
>  	struct fscrypt_info *ci = inode->i_crypt_info;
>  	struct crypto_skcipher *tfm = ci->ci_ctfm;
> +	u8 iv[FS_IV_SIZE];
>  	int res = 0;
>  
>  	BUG_ON(len == 0);
>  
> +	BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
> +	BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
> +	blk_desc.index = cpu_to_le64(lblk_num);
> +	memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
> +
> +	if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
> +		BUG_ON(ci->ci_essiv_tfm == NULL);
> +		memset(iv, 0, sizeof(iv));
> +		crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, (u8 *)&blk_desc);
> +	} else {
> +		memcpy(iv, &blk_desc, sizeof(iv));
> +	}
> +

The ESSIV cipher operation should be done in-place, so that only one IV buffer
is needed.  See what dm-crypt does in crypt_iv_essiv_gen(), for example.

Also, it can just use ESSIV 'if (ci->ci_essiv_tfm != NULL)'.  That would avoid
the awkward BUG_ON() and hardcoding of a specific cipher mode.

> @@ -27,13 +28,13 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc)
>   * derive_key_aes() - Derive a key using AES-128-ECB
>   * @deriving_key: Encryption key used for derivation.
>   * @source_key:   Source key to which to apply derivation.
> - * @derived_key:  Derived key.
> + * @derived_key_raw:  Derived raw key.
>   *
>   * Return: Zero on success; non-zero otherwise.
>   */
>  static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
> -				u8 source_key[FS_AES_256_XTS_KEY_SIZE],
> -				u8 derived_key[FS_AES_256_XTS_KEY_SIZE])
> +				struct fscrypt_key *source_key,
> +				u8 derived_raw_key[FS_MAX_KEY_SIZE])

'const struct fscrypt_key *'.

>  {
>  	int res = 0;
>  	struct skcipher_request *req = NULL;
> @@ -60,10 +61,10 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
>  	if (res < 0)
>  		goto out;
>  
> -	sg_init_one(&src_sg, source_key, FS_AES_256_XTS_KEY_SIZE);
> -	sg_init_one(&dst_sg, derived_key, FS_AES_256_XTS_KEY_SIZE);
> -	skcipher_request_set_crypt(req, &src_sg, &dst_sg,
> -					FS_AES_256_XTS_KEY_SIZE, NULL);
> +	sg_init_one(&src_sg, source_key->raw, source_key->size);
> +	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
> +	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
> +			NULL);
>  	res = crypto_skcipher_encrypt(req);
>  	if (res == -EINPROGRESS || res == -EBUSY) {
>  		wait_for_completion(&ecr.completion);
> @@ -75,9 +76,28 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
>  	return res;
>  }
>  
> +static bool valid_key_size(struct fscrypt_info *ci, struct fscrypt_key *key,
> +			int reg_file)
> +{
> +	if (reg_file) {
> +		switch(ci->ci_data_mode) {
> +		case FS_ENCRYPTION_MODE_AES_256_XTS:
> +			return key->size >= FS_AES_256_XTS_KEY_SIZE;
> +		case FS_ENCRYPTION_MODE_AES_128_CBC:
> +			return key->size >= FS_AES_128_CBC_KEY_SIZE;
> +		}
> +	} else {
> +		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS)
> +			return key->size >= FS_AES_256_CTS_KEY_SIZE;
> +		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_128_CTS)
> +			return key->size >= FS_AES_128_CTS_KEY_SIZE;
> +	}
> +	return false;
> +}
> +

This is redundant with how the key size is determined in
determine_cipher_type().  How about passing the expected key size to
validate_user_key() (instead of 'reg_file') and verifying that key->size >=
keysize?

Also, it should be verified that key->size <= FS_MAX_KEY_SIZE (since that's how
large the buffer in fscrypt_key is) and key->size % AES_BLOCK_SIZE == 0 (since
derive_key_aes() assumes the key is evenly divisible into AES blocks).

> @@ -134,6 +154,11 @@ static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
>  			*keysize_ret = FS_AES_256_XTS_KEY_SIZE;
>  			return 0;
>  		}
> +		if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
> +			*cipher_str_ret = "cbc(aes)";
> +			*keysize_ret = FS_AES_128_CBC_KEY_SIZE;
> +			return 0;
> +		}

switch (ci->ci_data_mode) {
...
}

> +		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_128_CTS) {
> +			*cipher_str_ret = "cts(cbc(aes))";
> +			*keysize_ret = FS_AES_128_CTS_KEY_SIZE;
> +			return 0;
> +		}

switch (ci->ci_filename_mode) {
...
}

> +	if (ci->ci_essiv_tfm)
> +		crypto_free_cipher(ci->ci_essiv_tfm);

No need to check for NULL before calling crypto_free_cipher().

> +	if (ctx.contents_encryption_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
> +	    ctx.filenames_encryption_mode != FS_ENCRYPTION_MODE_AES_128_CTS)
> +		return -EINVAL;
> +

I think for now we should only allow the two pairs:

	contents_encryption_mode=FS_ENCRYPTION_MODE_AES_256_XTS
	filenames_encryption_mode=FS_ENCRYPTION_MODE_AES_256_CTS

and

	contents_encryption_mode=FS_ENCRYPTION_MODE_AES_128_CBC
	filenames_encryption_mode=FS_ENCRYPTION_MODE_AES_128_CTS

Other combinations like AES-256-XTS paired with AES-128-CTS should be forbidden.

This also needs to be enforced in create_encryption_context_from_policy() so
that FS_IOC_SET_ENCRYPTION_POLICY fails with bad combinations.

> +	if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
> +		/* init ESSIV generator */
> +		essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
> +		if (!essiv_tfm || IS_ERR(essiv_tfm)) {
> +			res = essiv_tfm ? PTR_ERR(essiv_tfm) : -ENOMEM;
> +			printk(KERN_DEBUG
> +			       "%s: error %d (inode %u) allocating essiv tfm\n",
> +			       __func__, res, (unsigned) inode->i_ino);
> +			goto out;
> +		}
> +		/* calc sha of key for essiv generation */
> +		memset(sha_ws, 0, sizeof(sha_ws));
> +		sha_init(essiv_key);
> +		sha_transform(essiv_key, raw_key, sha_ws);
> +		res = crypto_cipher_setkey(essiv_tfm, (u8 *)essiv_key, keysize);
> +		if (res)
> +			goto out;
> +
> +		crypt_info->ci_essiv_tfm = essiv_tfm;
> +	}

I think the ESSIV hash should be SHA-256 not SHA-1.  SHA-1 is becoming more and
more obsolete these days.  Another issue with SHA-1 is that it only produces a
20 byte hash, which means it couldn't be used if someone ever wanted to add
AES-256-CBC as another mode.

Also, the hash should be called through the crypto API.

Also, please factor creating the essiv_tfm into its own function.
fscrypt_get_encryption_info() is long enough already.

Something else to consider (probably for the future; this doesn't necessarily
have to be done yet) is that you really only need one essiv_tfm per *key*, not
one per inode.  To deduplicate them you'd need a hash table or LRU queue or
something to keep track of the keys in use.

- Eric

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

[Eric Biggers]

On Thu, Mar 30, 2017 at 11:21:49PM -0700, Eric Biggers wrote:
> 
> Something else to consider (probably for the future; this doesn't necessarily
> have to be done yet) is that you really only need one essiv_tfm per *key*, not
> one per inode.  To deduplicate them you'd need a hash table or LRU queue or
> something to keep track of the keys in use.
> 

Sorry, I screwed this up.  This wouldn't work because the ESSIV key is being
derived from the per-file key, not the master key.

- Eric

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

[David Gstir]

Hi Eric,

thanks for the feedback!

> On 31.03.2017, at 08:21, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> [+Cc linux-fscrypt]

Oh, I didn't know about that list. I think MAINTAINERS should be updated to reflect that. :)

> 
> Hi David and Daniel,
> 
> On Thu, Mar 30, 2017 at 07:38:40PM +0200, David Gstir wrote:
>> From: Daniel Walter <dwalter@sigma-star.at>
>> 
>> 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.
>> Which 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. Especially low-powered embedded devices crypto
>> accelerators such as CAAM or CESA support only AES-CBC-128 with an acceptable
>> speed. Using AES-CBC gives us the acceptable performance while still providing
>> a moderate level of security for persistent storage.
>> 
> 
> Thanks for sending this!  I can't object too much to adding AES-CBC-128 if you
> find it useful, though of course AES-256-XTS will remain the recommendation for
> general use.  

Yes, AES-256-XTS should definitely be the recommendation and default here!
AES-128-CBC is a last resort if XTS is not possible for whatever reason.


> And I don't suppose AES-256-CBC is an option for you?

We went for AES-128 since it has less rounds and yields better performance. At least on the hardware we looked at, there was quite a difference in speed between AES-128-CBC and AES-256-CBC.

Anyways, AES-256-CBC could be added with just a few lines after this patch. :)


> Anyway, more comments below:

[...]

>> +	if (ctx.contents_encryption_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
>> +	    ctx.filenames_encryption_mode != FS_ENCRYPTION_MODE_AES_128_CTS)
>> +		return -EINVAL;
>> +
> 
> I think for now we should only allow the two pairs:
> 
> 	contents_encryption_mode=FS_ENCRYPTION_MODE_AES_256_XTS
> 	filenames_encryption_mode=FS_ENCRYPTION_MODE_AES_256_CTS
> 
> and
> 
> 	contents_encryption_mode=FS_ENCRYPTION_MODE_AES_128_CBC
> 	filenames_encryption_mode=FS_ENCRYPTION_MODE_AES_128_CTS
> 
> Other combinations like AES-256-XTS paired with AES-128-CTS should be forbidden.

Yes, I agree.


> This also needs to be enforced in create_encryption_context_from_policy() so
> that FS_IOC_SET_ENCRYPTION_POLICY fails with bad combinations.
> 
>> +	if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
>> +		/* init ESSIV generator */
>> +		essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
>> +		if (!essiv_tfm || IS_ERR(essiv_tfm)) {
>> +			res = essiv_tfm ? PTR_ERR(essiv_tfm) : -ENOMEM;
>> +			printk(KERN_DEBUG
>> +			       "%s: error %d (inode %u) allocating essiv tfm\n",
>> +			       __func__, res, (unsigned) inode->i_ino);
>> +			goto out;
>> +		}
>> +		/* calc sha of key for essiv generation */
>> +		memset(sha_ws, 0, sizeof(sha_ws));
>> +		sha_init(essiv_key);
>> +		sha_transform(essiv_key, raw_key, sha_ws);
>> +		res = crypto_cipher_setkey(essiv_tfm, (u8 *)essiv_key, keysize);
>> +		if (res)
>> +			goto out;
>> +
>> +		crypt_info->ci_essiv_tfm = essiv_tfm;
>> +	}
> 
> I think the ESSIV hash should be SHA-256 not SHA-1.  SHA-1 is becoming more and
> more obsolete these days.  Another issue with SHA-1 is that it only produces a
> 20 byte hash, which means it couldn't be used if someone ever wanted to add
> AES-256-CBC as another mode.

Good point! We'll change this to always use sha-256.


I'll wait for some more feedback and will provide a v2 which includes all your comments.

Thanks,
David

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

[David Gstir]

From: Daniel Walter <dwalter@sigma-star.at>

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. Especially low-powered embedded
devices with crypto accelerators such as CAAM or CESA support only
AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
performance while still providing a moderate level of security for
persistent storage.

Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
[david@sigma-star.at: massaged commit message]
Signed-off-by: David Gstir <david@sigma-star.at>
---
v2: Compute ESSIV salt using SHA256 instead of SHA1 and improve style
    as pointed out by Eric Biggers [1].

    [1] https://lkml.kernel.org/r/20170331062149.GA32409%20()%20zzz

 fs/crypto/crypto.c             |  22 ++++--
 fs/crypto/fscrypt_private.h    |   5 +-
 fs/crypto/keyinfo.c            | 165 ++++++++++++++++++++++++++++++++++-------
 fs/crypto/policy.c             |   7 +-
 include/linux/fscrypt_common.h |  13 ++--
 include/uapi/linux/fs.h        |   2 +
 6 files changed, 169 insertions(+), 45 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca394d4d..9dd5f23a55a5 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include <linux/ratelimit.h>
 #include <linux/dcache.h>
 #include <linux/namei.h>
+#include <crypto/aes.h>
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,17 +148,28 @@ 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)];
+	} blk_desc;
 	struct skcipher_request *req = NULL;
 	DECLARE_FS_COMPLETION_RESULT(ecr);
 	struct scatterlist dst, src;
 	struct fscrypt_info *ci = inode->i_crypt_info;
 	struct crypto_skcipher *tfm = ci->ci_ctfm;
 	int res = 0;
+	u8 *iv = (u8 *)&blk_desc;
 
 	BUG_ON(len == 0);
 
+	BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
+	BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+	blk_desc.index = cpu_to_le64(lblk_num);
+	memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
+
+	if (ci->ci_essiv_tfm != NULL) {
+		memset(iv, 0, sizeof(blk_desc));
+		crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, iv);
+	}
+
 	req = skcipher_request_alloc(tfm, gfp_flags);
 	if (!req) {
 		printk_ratelimited(KERN_ERR
@@ -170,15 +182,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, &ecr);
 
-	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(&dst, 1);
 	sg_set_page(&dst, dest_page, len, offs);
 	sg_init_table(&src, 1);
 	sg_set_page(&src, src_page, len, offs);
-	skcipher_request_set_crypt(req, &src, &dst, len, &xts_tweak);
+	skcipher_request_set_crypt(req, &src, &dst, len, &iv);
 	if (rw == FS_DECRYPT)
 		res = crypto_skcipher_decrypt(req);
 	else
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index e39696e64494..81ce9af449cb 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -16,8 +16,10 @@
 #define FS_FNAME_CRYPTO_DIGEST_SIZE	32
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE		16
+#define FS_IV_SIZE			16
 #define FS_AES_128_ECB_KEY_SIZE		16
+#define FS_AES_128_CBC_KEY_SIZE		16
+#define FS_AES_128_CTS_KEY_SIZE		16
 #define FS_AES_256_GCM_KEY_SIZE		32
 #define FS_AES_256_CBC_KEY_SIZE		32
 #define FS_AES_256_CTS_KEY_SIZE		32
@@ -67,6 +69,7 @@ struct fscrypt_info {
 	u8 ci_filename_mode;
 	u8 ci_flags;
 	struct crypto_skcipher *ci_ctfm;
+	struct crypto_cipher *ci_essiv_tfm;
 	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
 };
 
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 8cdfddce2b34..590efb0a891d 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,6 +10,8 @@
 
 #include <keys/user-type.h>
 #include <linux/scatterlist.h>
+#include <crypto/aes.h>
+#include <crypto/hash.h>
 #include "fscrypt_private.h"
 
 static void derive_crypt_complete(struct crypto_async_request *req, int rc)
@@ -27,13 +29,13 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc)
  * derive_key_aes() - Derive a key using AES-128-ECB
  * @deriving_key: Encryption key used for derivation.
  * @source_key:   Source key to which to apply derivation.
- * @derived_key:  Derived key.
+ * @derived_key_raw:  Derived raw key.
  *
  * Return: Zero on success; non-zero otherwise.
  */
 static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
-				u8 source_key[FS_AES_256_XTS_KEY_SIZE],
-				u8 derived_key[FS_AES_256_XTS_KEY_SIZE])
+				const struct fscrypt_key *source_key,
+				u8 derived_raw_key[FS_MAX_KEY_SIZE])
 {
 	int res = 0;
 	struct skcipher_request *req = NULL;
@@ -60,10 +62,10 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 	if (res < 0)
 		goto out;
 
-	sg_init_one(&src_sg, source_key, FS_AES_256_XTS_KEY_SIZE);
-	sg_init_one(&dst_sg, derived_key, FS_AES_256_XTS_KEY_SIZE);
-	skcipher_request_set_crypt(req, &src_sg, &dst_sg,
-					FS_AES_256_XTS_KEY_SIZE, NULL);
+	sg_init_one(&src_sg, source_key->raw, source_key->size);
+	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
+	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
+			NULL);
 	res = crypto_skcipher_encrypt(req);
 	if (res == -EINPROGRESS || res == -EBUSY) {
 		wait_for_completion(&ecr.completion);
@@ -77,7 +79,7 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 
 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,14 +113,15 @@ 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) {
 		printk_once(KERN_WARNING
 				"%s: key size incorrect: %d\n",
 				__func__, master_key->size);
 		res = -ENOKEY;
 		goto out;
 	}
-	res = derive_key_aes(ctx->nonce, master_key->raw, raw_key);
+	res = derive_key_aes(ctx->nonce, master_key, raw_key);
 out:
 	up_read(&keyring_key->sem);
 	key_put(keyring_key);
@@ -129,27 +132,37 @@ static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
 				 const char **cipher_str_ret, int *keysize_ret)
 {
 	if (S_ISREG(inode->i_mode)) {
-		if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
+		switch (ci->ci_data_mode) {
+		case FS_ENCRYPTION_MODE_AES_256_XTS:
 			*cipher_str_ret = "xts(aes)";
 			*keysize_ret = FS_AES_256_XTS_KEY_SIZE;
 			return 0;
+		case FS_ENCRYPTION_MODE_AES_128_CBC:
+			*cipher_str_ret = "cbc(aes)";
+			*keysize_ret = FS_AES_128_CBC_KEY_SIZE;
+			return 0;
+		default:
+			pr_warn_once("fscrypto: unsupported contents encryption mode %d for inode %lu\n",
+				     ci->ci_data_mode, inode->i_ino);
+			return -ENOKEY;
 		}
-		pr_warn_once("fscrypto: unsupported contents encryption mode "
-			     "%d for inode %lu\n",
-			     ci->ci_data_mode, inode->i_ino);
-		return -ENOKEY;
 	}
 
 	if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
-		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
+		switch (ci->ci_filename_mode) {
+		case FS_ENCRYPTION_MODE_AES_256_CTS:
 			*cipher_str_ret = "cts(cbc(aes))";
 			*keysize_ret = FS_AES_256_CTS_KEY_SIZE;
 			return 0;
+		case FS_ENCRYPTION_MODE_AES_128_CTS:
+			*cipher_str_ret = "cts(cbc(aes))";
+			*keysize_ret = FS_AES_128_CTS_KEY_SIZE;
+			return 0;
+		default:
+			pr_warn_once("fscrypto: unsupported filenames encryption mode %d for inode %lu\n",
+				     ci->ci_filename_mode, inode->i_ino);
+			return -ENOKEY;
 		}
-		pr_warn_once("fscrypto: unsupported filenames encryption mode "
-			     "%d for inode %lu\n",
-			     ci->ci_filename_mode, inode->i_ino);
-		return -ENOKEY;
 	}
 
 	pr_warn_once("fscrypto: unsupported file type %d for inode %lu\n",
@@ -163,14 +176,100 @@ static void put_crypt_info(struct fscrypt_info *ci)
 		return;
 
 	crypto_free_skcipher(ci->ci_ctfm);
+	crypto_free_cipher(ci->ci_essiv_tfm);
 	kmem_cache_free(fscrypt_info_cachep, ci);
 }
 
+static int derive_essiv_salt(u8 *raw_key, int keysize, u8 **salt_out,
+			unsigned int *saltsize)
+{
+	int res;
+	struct crypto_ahash *hash_tfm;
+	unsigned int digestsize;
+	u8 *salt = NULL;
+	struct scatterlist sg;
+	struct ahash_request *req = NULL;
+
+	hash_tfm = crypto_alloc_ahash("sha256", 0, 0);
+	if (IS_ERR(hash_tfm))
+		return PTR_ERR(hash_tfm);
+
+	digestsize = crypto_ahash_digestsize(hash_tfm);
+	salt = kzalloc(digestsize, GFP_NOFS);
+	if (!salt) {
+		res = -ENOMEM;
+		goto out;
+	}
+
+	req = ahash_request_alloc(hash_tfm, GFP_NOFS);
+	if (!req) {
+		kfree(salt);
+		res = -ENOMEM;
+		goto out;
+	}
+
+	sg_init_one(&sg, raw_key, keysize);
+	ahash_request_set_callback(req,
+			CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
+			NULL, NULL);
+	ahash_request_set_crypt(req, &sg, salt, keysize);
+
+	res = crypto_ahash_digest(req);
+	if (res) {
+		kfree(salt);
+		goto out;
+	}
+
+	*salt_out = salt;
+	*saltsize = digestsize;
+
+out:
+	crypto_free_ahash(hash_tfm);
+	ahash_request_free(req);
+	return res;
+}
+
+static int init_essiv_generator(struct fscrypt_info *ci, u8 *raw_key,
+				int keysize)
+{
+	int res;
+	struct crypto_cipher *essiv_tfm;
+	u8 *salt = NULL;
+	unsigned int saltsize;
+
+	/* init ESSIV generator */
+	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
+	if (!essiv_tfm || IS_ERR(essiv_tfm)) {
+		res = essiv_tfm ? PTR_ERR(essiv_tfm) : -ENOMEM;
+		goto err;
+	}
+
+	res = derive_essiv_salt(raw_key, keysize, &salt, &saltsize);
+	if (res)
+		goto err;
+
+	res = crypto_cipher_setkey(essiv_tfm, salt, saltsize);
+	if (res)
+		goto err;
+
+	ci->ci_essiv_tfm = essiv_tfm;
+
+	return 0;
+
+err:
+	if (essiv_tfm && !IS_ERR(essiv_tfm))
+		crypto_free_cipher(essiv_tfm);
+
+	kzfree(salt);
+	return res;
+}
+
 int fscrypt_get_encryption_info(struct inode *inode)
 {
 	struct fscrypt_info *crypt_info;
 	struct fscrypt_context ctx;
 	struct crypto_skcipher *ctfm;
+
 	const char *cipher_str;
 	int keysize;
 	u8 *raw_key = NULL;
@@ -207,6 +306,10 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (ctx.flags & ~FS_POLICY_FLAGS_VALID)
 		return -EINVAL;
 
+	if (!fscrypt_valid_enc_modes(ctx.contents_encryption_mode,
+				ctx.filenames_encryption_mode))
+		return -EINVAL;
+
 	crypt_info = kmem_cache_alloc(fscrypt_info_cachep, GFP_NOFS);
 	if (!crypt_info)
 		return -ENOMEM;
@@ -215,6 +318,7 @@ 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_ctfm = NULL;
+	crypt_info->ci_essiv_tfm = NULL;
 	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
 				sizeof(crypt_info->ci_master_key));
 
@@ -231,10 +335,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (!raw_key)
 		goto out;
 
-	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX);
+	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
+				keysize);
 	if (res && inode->i_sb->s_cop->key_prefix) {
 		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
-					     inode->i_sb->s_cop->key_prefix);
+					     inode->i_sb->s_cop->key_prefix,
+					     keysize);
 		if (res2) {
 			if (res2 == -ENOKEY)
 				res = -ENOKEY;
@@ -246,9 +352,9 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	ctfm = crypto_alloc_skcipher(cipher_str, 0, 0);
 	if (!ctfm || IS_ERR(ctfm)) {
 		res = ctfm ? PTR_ERR(ctfm) : -ENOMEM;
-		printk(KERN_DEBUG
-		       "%s: error %d (inode %u) allocating crypto tfm\n",
-		       __func__, res, (unsigned) inode->i_ino);
+		pr_debug(
+		      "%s: error %d (inode %u) allocating crypto tfm\n",
+		      __func__, res, (unsigned int) inode->i_ino);
 		goto out;
 	}
 	crypt_info->ci_ctfm = ctfm;
@@ -258,6 +364,15 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (res)
 		goto out;
 
+	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 %u) allocating essiv tfm\n",
+			     __func__, res, (unsigned int) inode->i_ino);
+			goto out;
+		}
+	}
 	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) == NULL)
 		crypt_info = NULL;
 out:
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 4908906d54d5..bac8009245f2 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -41,11 +41,8 @@ static int create_encryption_context_from_policy(struct inode *inode,
 	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
 					FS_KEY_DESCRIPTOR_SIZE);
 
-	if (!fscrypt_valid_contents_enc_mode(
-				policy->contents_encryption_mode))
-		return -EINVAL;
-
-	if (!fscrypt_valid_filenames_enc_mode(
+	if (!fscrypt_valid_enc_modes(
+				policy->contents_encryption_mode,
 				policy->filenames_encryption_mode))
 		return -EINVAL;
 
diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt_common.h
index 10c1abfbac6c..a91ce99f6e19 100644
--- a/include/linux/fscrypt_common.h
+++ b/include/linux/fscrypt_common.h
@@ -102,14 +102,13 @@ static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
 	return false;
 }
 
-static inline bool fscrypt_valid_contents_enc_mode(u32 mode)
+static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
+					u32 filenames_mode)
 {
-	return (mode == FS_ENCRYPTION_MODE_AES_256_XTS);
-}
-
-static inline bool fscrypt_valid_filenames_enc_mode(u32 mode)
-{
-	return (mode == FS_ENCRYPTION_MODE_AES_256_CTS);
+	return ((contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
+		 filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS) ||
+		(contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
+		 filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS));
 }
 
 static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 048a85e9f017..231b15d4a90e 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -272,6 +272,8 @@ struct fsxattr {
 #define FS_ENCRYPTION_MODE_AES_256_GCM		2
 #define FS_ENCRYPTION_MODE_AES_256_CBC		3
 #define FS_ENCRYPTION_MODE_AES_256_CTS		4
+#define FS_ENCRYPTION_MODE_AES_128_CBC		5
+#define FS_ENCRYPTION_MODE_AES_128_CTS		6
 
 struct fscrypt_policy {
 	__u8 version;
-- 
2.12.0

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

[Eric Biggers]

Hi Daniel and David,

On Tue, Apr 25, 2017 at 04:41:00PM +0200, David Gstir wrote:
> @@ -147,17 +148,28 @@ 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)];
> +	} blk_desc;
>  	struct skcipher_request *req = NULL;
>  	DECLARE_FS_COMPLETION_RESULT(ecr);
>  	struct scatterlist dst, src;
>  	struct fscrypt_info *ci = inode->i_crypt_info;
>  	struct crypto_skcipher *tfm = ci->ci_ctfm;
>  	int res = 0;
> +	u8 *iv = (u8 *)&blk_desc;
>  
>  	BUG_ON(len == 0);
>  
> +	BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
> +	BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
> +	blk_desc.index = cpu_to_le64(lblk_num);
> +	memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
> +
> +	if (ci->ci_essiv_tfm != NULL) {
> +		memset(iv, 0, sizeof(blk_desc));
> +		crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, iv);
> +	}
> +
>  	req = skcipher_request_alloc(tfm, gfp_flags);
>  	if (!req) {
>  		printk_ratelimited(KERN_ERR
> @@ -170,15 +182,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, &ecr);
>  
> -	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(&dst, 1);
>  	sg_set_page(&dst, dest_page, len, offs);
>  	sg_init_table(&src, 1);
>  	sg_set_page(&src, src_page, len, offs);
> -	skcipher_request_set_crypt(req, &src, &dst, len, &xts_tweak);
> +	skcipher_request_set_crypt(req, &src, &dst, len, &iv);
>  	if (rw == FS_DECRYPT)
>  		res = crypto_skcipher_decrypt(req);
>  	else

There are two critical bugs here.  First the IV is being zeroed before being
encrypted with the ESSIV tfm, so the resulting IV will always be the same for a
given file.  It should be encrypting the block number instead.  Second what's
actually being passed into the crypto API is '&iv' where IV is a pointer to
something on the stack... I really doubt that does what's intended :)

Probably the cleanest way to do this correctly is to just have the struct be the
'iv', so there's no extra pointer to deal with:

	struct {
		__le64 index;
		u8 padding[FS_IV_SIZE - sizeof(__le64)];
	} iv;

	[...]

	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 *)&iv,
					  (u8 *)&iv);
	}

	[...]

	skcipher_request_set_crypt(req, &src, &dst, len, &iv);

> +static int derive_essiv_salt(u8 *raw_key, int keysize, u8 **salt_out,
> +			unsigned int *saltsize)
> +{
> +	int res;
> +	struct crypto_ahash *hash_tfm;
> +	unsigned int digestsize;
> +	u8 *salt = NULL;
> +	struct scatterlist sg;
> +	struct ahash_request *req = NULL;
> +
> +	hash_tfm = crypto_alloc_ahash("sha256", 0, 0);
> +	if (IS_ERR(hash_tfm))
> +		return PTR_ERR(hash_tfm);
> +
> +	digestsize = crypto_ahash_digestsize(hash_tfm);
> +	salt = kzalloc(digestsize, GFP_NOFS);
> +	if (!salt) {
> +		res = -ENOMEM;
> +		goto out;
> +	}
> +
> +	req = ahash_request_alloc(hash_tfm, GFP_NOFS);
> +	if (!req) {
> +		kfree(salt);
> +		res = -ENOMEM;
> +		goto out;
> +	}
> +
> +	sg_init_one(&sg, raw_key, keysize);
> +	ahash_request_set_callback(req,
> +			CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
> +			NULL, NULL);
> +	ahash_request_set_crypt(req, &sg, salt, keysize);
> +
> +	res = crypto_ahash_digest(req);
> +	if (res) {
> +		kfree(salt);
> +		goto out;
> +	}
> +
> +	*salt_out = salt;
> +	*saltsize = digestsize;
> +
> +out:
> +	crypto_free_ahash(hash_tfm);
> +	ahash_request_free(req);
> +	return res;
> +}
> +
> +static int init_essiv_generator(struct fscrypt_info *ci, u8 *raw_key,
> +				int keysize)
> +{
> +	int res;
> +	struct crypto_cipher *essiv_tfm;
> +	u8 *salt = NULL;
> +	unsigned int saltsize;
> +
> +	/* init ESSIV generator */
> +	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
> +	if (!essiv_tfm || IS_ERR(essiv_tfm)) {
> +		res = essiv_tfm ? PTR_ERR(essiv_tfm) : -ENOMEM;
> +		goto err;
> +	}
> +
> +	res = derive_essiv_salt(raw_key, keysize, &salt, &saltsize);
> +	if (res)
> +		goto err;
> +
> +	res = crypto_cipher_setkey(essiv_tfm, salt, saltsize);
> +	if (res)
> +		goto err;
> +
> +	ci->ci_essiv_tfm = essiv_tfm;
> +
> +	return 0;
> +
> +err:
> +	if (essiv_tfm && !IS_ERR(essiv_tfm))
> +		crypto_free_cipher(essiv_tfm);
> +
> +	kzfree(salt);
> +	return res;
> +}

There are a few issues with how the ESSIV generator is initialized:

1.) It's allocating a possibly asynchronous SHA-256 implementation but then not
    handling it actually being asynchronous.  I suggest using the 'shash' API
    which is easier to use.
2.) No one is going to change the digest size of SHA-256; it's fixed at 32
    bytes.  So just #include <crypto/sha.h> and allocate a 'u8
    salt[SHA256_DIGEST_SIZE];' on the stack.  Make sure to do
    memzero_explicit(salt, sizeof(salt)) in all cases.
3.) It's always keying the ESSIV transform using a 256-bit AES key.  It's still
    secure of course, but I'm not sure it's what you intended, given that it's
    used in combination with AES-128.  I really think that someone who's more of
    an expert on ESSIV really should weigh in, but my intuition is that you
    really only need to be using AES-128, using the first 'keysize' bytes of the
    hash.

You also don't really need to handle freeing the essiv_tfm on errors, as long as
you assign it to the fscrypt_info first.  Also crypto_alloc_cipher() returns an
ERR_PTR(), never NULL.

Also, fs/crypto/Kconfig needs a 'select CRYPTO_SHA256' now.

Here's a revised version to consider, not actually tested though:

static int derive_essiv_salt(const u8 *raw_key, int keysize,
			     u8 salt[SHA256_DIGEST_SIZE])
{
	struct crypto_shash *hash_tfm;

	hash_tfm = crypto_alloc_shash("sha256", 0, 0);
	if (IS_ERR(hash_tfm)) {
		pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld",
				    PTR_ERR(hash_tfm));
		return PTR_ERR(hash_tfm);
	} else {
		SHASH_DESC_ON_STACK(desc, hash_tfm);
		int err;

		desc->tfm = hash_tfm;
		desc->flags = 0;

		BUG_ON(crypto_shash_digestsize(hash_tfm) != SHA256_DIGEST_SIZE);

		err = crypto_shash_digest(desc, raw_key, keysize, salt);
		crypto_free_shash(hash_tfm);
		return err;
	}
}

static int init_essiv_generator(struct fscrypt_info *ci,
				const u8 *raw_key, int keysize)
{
	struct crypto_cipher *essiv_tfm;
	u8 salt[SHA256_DIGEST_SIZE];
	int err;

	if (WARN_ON_ONCE(keysize > sizeof(salt)))
		return -EINVAL;

	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
	if (IS_ERR(essiv_tfm))
		return PTR_ERR(essiv_tfm);

	ci->ci_essiv_tfm = essiv_tfm;

	err = derive_essiv_salt(raw_key, keysize, salt);
	if (err)
		goto out;

	err = crypto_cipher_setkey(essiv_tfm, salt, keysize);
out:
	memzero_explicit(salt, sizeof(salt));
	return err;
}

> +
>  int fscrypt_get_encryption_info(struct inode *inode)
>  {
>  	struct fscrypt_info *crypt_info;
>  	struct fscrypt_context ctx;
>  	struct crypto_skcipher *ctfm;
> +
>  	const char *cipher_str;
>  	int keysize;
>  	u8 *raw_key = NULL;
> @@ -207,6 +306,10 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	if (ctx.flags & ~FS_POLICY_FLAGS_VALID)
>  		return -EINVAL;
>  
> +	if (!fscrypt_valid_enc_modes(ctx.contents_encryption_mode,
> +				ctx.filenames_encryption_mode))
> +		return -EINVAL;
> +

Indent this properly

>  	crypt_info = kmem_cache_alloc(fscrypt_info_cachep, GFP_NOFS);
>  	if (!crypt_info)
>  		return -ENOMEM;
> @@ -215,6 +318,7 @@ 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_ctfm = NULL;
> +	crypt_info->ci_essiv_tfm = NULL;
>  	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
>  				sizeof(crypt_info->ci_master_key));
>  
> @@ -231,10 +335,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	if (!raw_key)
>  		goto out;
>  
> -	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX);
> +	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> +				keysize);
>  	if (res && inode->i_sb->s_cop->key_prefix) {
>  		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> -					     inode->i_sb->s_cop->key_prefix);
> +					     inode->i_sb->s_cop->key_prefix,
> +					     keysize);
>  		if (res2) {
>  			if (res2 == -ENOKEY)
>  				res = -ENOKEY;
> @@ -246,9 +352,9 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	ctfm = crypto_alloc_skcipher(cipher_str, 0, 0);
>  	if (!ctfm || IS_ERR(ctfm)) {
>  		res = ctfm ? PTR_ERR(ctfm) : -ENOMEM;
> -		printk(KERN_DEBUG
> -		       "%s: error %d (inode %u) allocating crypto tfm\n",
> -		       __func__, res, (unsigned) inode->i_ino);
> +		pr_debug(
> +		      "%s: error %d (inode %u) allocating crypto tfm\n",
> +		      __func__, res, (unsigned int) inode->i_ino);
>  		goto out;

If you're changing this line just make it print i_ino as an 'unsigned long', no
need to cast it.  Same below.

>  	}
>  	crypt_info->ci_ctfm = ctfm;
> @@ -258,6 +364,15 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	if (res)
>  		goto out;
>  
> +	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 %u) allocating essiv tfm\n",
> +			     __func__, res, (unsigned int) inode->i_ino);
> +			goto out;
> +		}
> +	}
>  	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) == NULL)
>  		crypt_info = NULL;
>  out:
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 4908906d54d5..bac8009245f2 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -41,11 +41,8 @@ static int create_encryption_context_from_policy(struct inode *inode,
>  	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
>  					FS_KEY_DESCRIPTOR_SIZE);
>  
> -	if (!fscrypt_valid_contents_enc_mode(
> -				policy->contents_encryption_mode))
> -		return -EINVAL;
> -
> -	if (!fscrypt_valid_filenames_enc_mode(
> +	if (!fscrypt_valid_enc_modes(
> +				policy->contents_encryption_mode,
>  				policy->filenames_encryption_mode))
>  		return -EINVAL;

Indent properly:

	if (!fscrypt_valid_enc_modes(policy->contents_encryption_mode,
				     policy->filenames_encryption_mode))

- Eric

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

[David Gstir]

Hi Eric!

Thanks for the feedback!

> On 25 Apr 2017, at 22:10, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> Hi Daniel and David,
> 
> On Tue, Apr 25, 2017 at 04:41:00PM +0200, David Gstir wrote:
>> @@ -147,17 +148,28 @@ 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)];
>> +	} blk_desc;
>> 	struct skcipher_request *req = NULL;
>> 	DECLARE_FS_COMPLETION_RESULT(ecr);
>> 	struct scatterlist dst, src;
>> 	struct fscrypt_info *ci = inode->i_crypt_info;
>> 	struct crypto_skcipher *tfm = ci->ci_ctfm;
>> 	int res = 0;
>> +	u8 *iv = (u8 *)&blk_desc;
>> 
>> 	BUG_ON(len == 0);
>> 
>> +	BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
>> +	BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
>> +	blk_desc.index = cpu_to_le64(lblk_num);
>> +	memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
>> +
>> +	if (ci->ci_essiv_tfm != NULL) {
>> +		memset(iv, 0, sizeof(blk_desc));
>> +		crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, iv);
>> +	}
>> +
>> 	req = skcipher_request_alloc(tfm, gfp_flags);
>> 	if (!req) {
>> 		printk_ratelimited(KERN_ERR
>> @@ -170,15 +182,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, &ecr);
>> 
>> -	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(&dst, 1);
>> 	sg_set_page(&dst, dest_page, len, offs);
>> 	sg_init_table(&src, 1);
>> 	sg_set_page(&src, src_page, len, offs);
>> -	skcipher_request_set_crypt(req, &src, &dst, len, &xts_tweak);
>> +	skcipher_request_set_crypt(req, &src, &dst, len, &iv);
>> 	if (rw == FS_DECRYPT)
>> 		res = crypto_skcipher_decrypt(req);
>> 	else
> 
> There are two critical bugs here.  First the IV is being zeroed before being
> encrypted with the ESSIV tfm, so the resulting IV will always be the same for a
> given file.  It should be encrypting the block number instead.  Second what's
> actually being passed into the crypto API is '&iv' where IV is a pointer to
> something on the stack... I really doubt that does what's intended :)
> 
> Probably the cleanest way to do this correctly is to just have the struct be the
> 'iv', so there's no extra pointer to deal with:
> 
> 	struct {
> 		__le64 index;
> 		u8 padding[FS_IV_SIZE - sizeof(__le64)];
> 	} iv;
> 
> 	[...]
> 
> 	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 *)&iv,
> 					  (u8 *)&iv);
> 	}
> 
> 	[...]
> 
> 	skcipher_request_set_crypt(req, &src, &dst, len, &iv);

You are totally right. Somehow I completely missed that. :-/



> 
>> +static int derive_essiv_salt(u8 *raw_key, int keysize, u8 **salt_out,
>> +			unsigned int *saltsize)
>> +{
>> +	int res;
>> +	struct crypto_ahash *hash_tfm;
>> +	unsigned int digestsize;
>> +	u8 *salt = NULL;
>> +	struct scatterlist sg;
>> +	struct ahash_request *req = NULL;
>> +
>> +	hash_tfm = crypto_alloc_ahash("sha256", 0, 0);
>> +	if (IS_ERR(hash_tfm))
>> +		return PTR_ERR(hash_tfm);
>> +
>> +	digestsize = crypto_ahash_digestsize(hash_tfm);
>> +	salt = kzalloc(digestsize, GFP_NOFS);
>> +	if (!salt) {
>> +		res = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	req = ahash_request_alloc(hash_tfm, GFP_NOFS);
>> +	if (!req) {
>> +		kfree(salt);
>> +		res = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	sg_init_one(&sg, raw_key, keysize);
>> +	ahash_request_set_callback(req,
>> +			CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
>> +			NULL, NULL);
>> +	ahash_request_set_crypt(req, &sg, salt, keysize);
>> +
>> +	res = crypto_ahash_digest(req);
>> +	if (res) {
>> +		kfree(salt);
>> +		goto out;
>> +	}
>> +
>> +	*salt_out = salt;
>> +	*saltsize = digestsize;
>> +
>> +out:
>> +	crypto_free_ahash(hash_tfm);
>> +	ahash_request_free(req);
>> +	return res;
>> +}
>> +
>> +static int init_essiv_generator(struct fscrypt_info *ci, u8 *raw_key,
>> +				int keysize)
>> +{
>> +	int res;
>> +	struct crypto_cipher *essiv_tfm;
>> +	u8 *salt = NULL;
>> +	unsigned int saltsize;
>> +
>> +	/* init ESSIV generator */
>> +	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
>> +	if (!essiv_tfm || IS_ERR(essiv_tfm)) {
>> +		res = essiv_tfm ? PTR_ERR(essiv_tfm) : -ENOMEM;
>> +		goto err;
>> +	}
>> +
>> +	res = derive_essiv_salt(raw_key, keysize, &salt, &saltsize);
>> +	if (res)
>> +		goto err;
>> +
>> +	res = crypto_cipher_setkey(essiv_tfm, salt, saltsize);
>> +	if (res)
>> +		goto err;
>> +
>> +	ci->ci_essiv_tfm = essiv_tfm;
>> +
>> +	return 0;
>> +
>> +err:
>> +	if (essiv_tfm && !IS_ERR(essiv_tfm))
>> +		crypto_free_cipher(essiv_tfm);
>> +
>> +	kzfree(salt);
>> +	return res;
>> +}
> 
> There are a few issues with how the ESSIV generator is initialized:
> 
> 1.) It's allocating a possibly asynchronous SHA-256 implementation but then not
>    handling it actually being asynchronous.  I suggest using the 'shash' API
>    which is easier to use.
> 2.) No one is going to change the digest size of SHA-256; it's fixed at 32
>    bytes.  So just #include <crypto/sha.h> and allocate a 'u8
>    salt[SHA256_DIGEST_SIZE];' on the stack.  Make sure to do
>    memzero_explicit(salt, sizeof(salt)) in all cases.
> 3.) It's always keying the ESSIV transform using a 256-bit AES key.  It's still
>    secure of course, but I'm not sure it's what you intended, given that it's
>    used in combination with AES-128.  I really think that someone who's more of
>    an expert on ESSIV really should weigh in, but my intuition is that you
>    really only need to be using AES-128, using the first 'keysize' bytes of the
>    hash.

My intention is to use all 256 bits we get from the hash. Yes, this will then use
AES-256 for the IV generation, but this will still yield just a 128 bit IV for
file contents encryption since the block size of AES is the same. So this is
just a case of using AES with a 256 bit key over a 128 bit one which is then
used for AES-128 computations.

On the other hand, as you pointed out, truncating the hash and using AES-128 *should*
suffice too. Especially since we are using AES-128 everywhere else!

I'm also no export on ESSIV, so I'm not a 100% sure if there is something we're
missing here. If using AES-128 is okay, I'll change it to truncate the hash.


> You also don't really need to handle freeing the essiv_tfm on errors, as long as
> you assign it to the fscrypt_info first.  Also crypto_alloc_cipher() returns an
> ERR_PTR(), never NULL.
> 
> Also, fs/crypto/Kconfig needs a 'select CRYPTO_SHA256' now.
> 
> Here's a revised version to consider, not actually tested though:
> 
> static int derive_essiv_salt(const u8 *raw_key, int keysize,
> 			     u8 salt[SHA256_DIGEST_SIZE])
> {
> 	struct crypto_shash *hash_tfm;
> 
> 	hash_tfm = crypto_alloc_shash("sha256", 0, 0);
> 	if (IS_ERR(hash_tfm)) {
> 		pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld",
> 				    PTR_ERR(hash_tfm));
> 		return PTR_ERR(hash_tfm);
> 	} else {
> 		SHASH_DESC_ON_STACK(desc, hash_tfm);
> 		int err;
> 
> 		desc->tfm = hash_tfm;
> 		desc->flags = 0;
> 
> 		BUG_ON(crypto_shash_digestsize(hash_tfm) != SHA256_DIGEST_SIZE);
> 
> 		err = crypto_shash_digest(desc, raw_key, keysize, salt);
> 		crypto_free_shash(hash_tfm);
> 		return err;
> 	}
> }
> 
> static int init_essiv_generator(struct fscrypt_info *ci,
> 				const u8 *raw_key, int keysize)
> {
> 	struct crypto_cipher *essiv_tfm;
> 	u8 salt[SHA256_DIGEST_SIZE];
> 	int err;
> 
> 	if (WARN_ON_ONCE(keysize > sizeof(salt)))
> 		return -EINVAL;
> 
> 	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
> 	if (IS_ERR(essiv_tfm))
> 		return PTR_ERR(essiv_tfm);
> 
> 	ci->ci_essiv_tfm = essiv_tfm;
> 
> 	err = derive_essiv_salt(raw_key, keysize, salt);
> 	if (err)
> 		goto out;
> 
> 	err = crypto_cipher_setkey(essiv_tfm, salt, keysize);
> out:
> 	memzero_explicit(salt, sizeof(salt));
> 	return err;
> }

Thanks a lot for that! Using shash and SHA256_DIGEST_SIZE makes this much cleaner.
I'll redo that for v3.

One optimization Richard pointed out is that we should do the
crypto_alloc_shash("sha256", 0, 0) just once and reuse hash_tfm for all sha256 computations.
This will save us some alloc/frees in derive_essiv_salt.

Thanks!
David

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

[Eric Biggers]

Hi David,

On Wed, Apr 26, 2017 at 08:18:51AM +0200, David Gstir wrote:
> > 3.) It's always keying the ESSIV transform using a 256-bit AES key.  It's still
> >    secure of course, but I'm not sure it's what you intended, given that it's
> >    used in combination with AES-128.  I really think that someone who's more of
> >    an expert on ESSIV really should weigh in, but my intuition is that you
> >    really only need to be using AES-128, using the first 'keysize' bytes of the
> >    hash.
> 
> My intention is to use all 256 bits we get from the hash. Yes, this will then use
> AES-256 for the IV generation, but this will still yield just a 128 bit IV for
> file contents encryption since the block size of AES is the same. So this is
> just a case of using AES with a 256 bit key over a 128 bit one which is then
> used for AES-128 computations.
> 
> On the other hand, as you pointed out, truncating the hash and using AES-128 *should*
> suffice too. Especially since we are using AES-128 everywhere else!
> 
> I'm also no export on ESSIV, so I'm not a 100% sure if there is something we're
> missing here. If using AES-128 is okay, I'll change it to truncate the hash.
> 

After thinking about it some more I'm actually slightly leaning towards AES-256,
since it matches the size of the message digest being used.  I think that's how
ESSIV was designed to work, since message digests are not necessarily designed
to be truncated.  Also I doubt there would be any noticable performance
difference from using AES-256 instead of AES-128, given that it's just for the
IV generation and not for the "real" encryption.

On the other hand, the message digest *is* hard-coded to SHA-256, and the
SHA-256 specification actually states that it can be truncated, with the
collision resistance decreased in the expected way.

> 
> One optimization Richard pointed out is that we should do the
> crypto_alloc_shash("sha256", 0, 0) just once and reuse hash_tfm for all sha256 computations.
> This will save us some alloc/frees in derive_essiv_salt.
> 

Yes, I had the same idea too.  I suggest allocating it only the first time it's
used rather than always doing it in fscrypt_init(), since not everyone will be
needing it.

- Eric

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

[David Gstir]

From: Daniel Walter <dwalter@sigma-star.at>

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. Especially low-powered embedded
devices with crypto accelerators such as CAAM or CESA support only
AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
performance while still providing a moderate level of security for
persistent storage.

Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
[david@sigma-star.at: addressed review comments]
Signed-off-by: David Gstir <david@sigma-star.at>
---
 fs/crypto/Kconfig              |   1 +
 fs/crypto/crypto.c             |  23 +++++--
 fs/crypto/fscrypt_private.h    |   9 ++-
 fs/crypto/keyinfo.c            | 147 ++++++++++++++++++++++++++++++++++-------
 fs/crypto/policy.c             |   8 +--
 include/linux/fscrypt_common.h |  13 ++--
 include/uapi/linux/fs.h        |   2 +
 7 files changed, 157 insertions(+), 46 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..0d4582c3aef1 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include <linux/ratelimit.h>
 #include <linux/dcache.h>
 #include <linux/namei.h>
+#include <crypto/aes.h>
 #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 *)&iv,
+					  (u8 *)&iv);
+	}
+
 	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, &ecr);
 
-	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(&dst, 1);
 	sg_set_page(&dst, dest_page, len, offs);
 	sg_init_table(&src, 1);
 	sg_set_page(&src, src_page, len, offs);
-	skcipher_request_set_crypt(req, &src, &dst, len, &xts_tweak);
+	skcipher_request_set_crypt(req, &src, &dst, len, &iv);
 	if (rw == FS_DECRYPT)
 		res = crypto_skcipher_decrypt(req);
 	else
@@ -388,6 +395,8 @@ static void fscrypt_destroy(void)
 	INIT_LIST_HEAD(&fscrypt_free_ctxs);
 	mempool_destroy(fscrypt_bounce_page_pool);
 	fscrypt_bounce_page_pool = NULL;
+
+	fscrypt_essiv_cleanup();
 }
 
 /**
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 1e1f8a361b75..68e605613352 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -12,10 +12,13 @@
 #define _FSCRYPT_PRIVATE_H
 
 #include <linux/fscrypt_supp.h>
+#include <crypto/hash.h>
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE		16
+#define FS_IV_SIZE			16
 #define FS_AES_128_ECB_KEY_SIZE		16
+#define FS_AES_128_CBC_KEY_SIZE		16
+#define FS_AES_128_CTS_KEY_SIZE		16
 #define FS_AES_256_GCM_KEY_SIZE		32
 #define FS_AES_256_CBC_KEY_SIZE		32
 #define FS_AES_256_CTS_KEY_SIZE		32
@@ -54,6 +57,7 @@ struct fscrypt_info {
 	u8 ci_filename_mode;
 	u8 ci_flags;
 	struct crypto_skcipher *ci_ctfm;
+	struct crypto_cipher *ci_essiv_tfm;
 	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
 };
 
@@ -87,4 +91,7 @@ extern int fscrypt_do_page_crypto(const struct inode *inode,
 extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
 					      gfp_t gfp_flags);
 
+/* keyinfo.c */
+extern void fscrypt_essiv_cleanup(void);
+
 #endif /* _FSCRYPT_PRIVATE_H */
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 179e578b875b..a09a4fa5ed52 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,8 +10,14 @@
 
 #include <keys/user-type.h>
 #include <linux/scatterlist.h>
+#include <linux/ratelimit.h>
+#include <crypto/aes.h>
+#include <crypto/sha.h>
 #include "fscrypt_private.h"
 
+static struct crypto_shash *essiv_hash_tfm;
+static DEFINE_MUTEX(essiv_hash_lock);
+
 static void derive_crypt_complete(struct crypto_async_request *req, int rc)
 {
 	struct fscrypt_completion_result *ecr = req->data;
@@ -27,13 +33,13 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc)
  * derive_key_aes() - Derive a key using AES-128-ECB
  * @deriving_key: Encryption key used for derivation.
  * @source_key:   Source key to which to apply derivation.
- * @derived_key:  Derived key.
+ * @derived_raw_key:  Derived raw key.
  *
  * Return: Zero on success; non-zero otherwise.
  */
 static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
-				u8 source_key[FS_AES_256_XTS_KEY_SIZE],
-				u8 derived_key[FS_AES_256_XTS_KEY_SIZE])
+				const struct fscrypt_key *source_key,
+				u8 derived_raw_key[FS_MAX_KEY_SIZE])
 {
 	int res = 0;
 	struct skcipher_request *req = NULL;
@@ -60,10 +66,10 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 	if (res < 0)
 		goto out;
 
-	sg_init_one(&src_sg, source_key, FS_AES_256_XTS_KEY_SIZE);
-	sg_init_one(&dst_sg, derived_key, FS_AES_256_XTS_KEY_SIZE);
-	skcipher_request_set_crypt(req, &src_sg, &dst_sg,
-					FS_AES_256_XTS_KEY_SIZE, NULL);
+	sg_init_one(&src_sg, source_key->raw, source_key->size);
+	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
+	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
+				   NULL);
 	res = crypto_skcipher_encrypt(req);
 	if (res == -EINPROGRESS || res == -EBUSY) {
 		wait_for_completion(&ecr.completion);
@@ -77,7 +83,7 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 
 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,14 +117,15 @@ 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) {
 		printk_once(KERN_WARNING
 				"%s: key size incorrect: %d\n",
 				__func__, master_key->size);
 		res = -ENOKEY;
 		goto out;
 	}
-	res = derive_key_aes(ctx->nonce, master_key->raw, raw_key);
+	res = derive_key_aes(ctx->nonce, master_key, raw_key);
 out:
 	up_read(&keyring_key->sem);
 	key_put(keyring_key);
@@ -129,27 +136,37 @@ static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
 				 const char **cipher_str_ret, int *keysize_ret)
 {
 	if (S_ISREG(inode->i_mode)) {
-		if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
+		switch (ci->ci_data_mode) {
+		case FS_ENCRYPTION_MODE_AES_256_XTS:
 			*cipher_str_ret = "xts(aes)";
 			*keysize_ret = FS_AES_256_XTS_KEY_SIZE;
 			return 0;
+		case FS_ENCRYPTION_MODE_AES_128_CBC:
+			*cipher_str_ret = "cbc(aes)";
+			*keysize_ret = FS_AES_128_CBC_KEY_SIZE;
+			return 0;
+		default:
+			pr_warn_once("fscrypto: unsupported contents encryption mode %d for inode %lu\n",
+				     ci->ci_data_mode, inode->i_ino);
+			return -ENOKEY;
 		}
-		pr_warn_once("fscrypto: unsupported contents encryption mode "
-			     "%d for inode %lu\n",
-			     ci->ci_data_mode, inode->i_ino);
-		return -ENOKEY;
 	}
 
 	if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
-		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
+		switch (ci->ci_filename_mode) {
+		case FS_ENCRYPTION_MODE_AES_256_CTS:
 			*cipher_str_ret = "cts(cbc(aes))";
 			*keysize_ret = FS_AES_256_CTS_KEY_SIZE;
 			return 0;
+		case FS_ENCRYPTION_MODE_AES_128_CTS:
+			*cipher_str_ret = "cts(cbc(aes))";
+			*keysize_ret = FS_AES_128_CTS_KEY_SIZE;
+			return 0;
+		default:
+			pr_warn_once("fscrypto: unsupported filenames encryption mode %d for inode %lu\n",
+				     ci->ci_filename_mode, inode->i_ino);
+			return -ENOKEY;
 		}
-		pr_warn_once("fscrypto: unsupported filenames encryption mode "
-			     "%d for inode %lu\n",
-			     ci->ci_filename_mode, inode->i_ino);
-		return -ENOKEY;
 	}
 
 	pr_warn_once("fscrypto: unsupported file type %d for inode %lu\n",
@@ -163,9 +180,75 @@ static void put_crypt_info(struct fscrypt_info *ci)
 		return;
 
 	crypto_free_skcipher(ci->ci_ctfm);
+	crypto_free_cipher(ci->ci_essiv_tfm);
 	kmem_cache_free(fscrypt_info_cachep, ci);
 }
 
+static int derive_essiv_salt(u8 *key, int keysize, u8 *salt)
+{
+	int err;
+
+	/* init hash transform on demand */
+	if (unlikely(essiv_hash_tfm == NULL)) {
+		mutex_lock(&essiv_hash_lock);
+		if (essiv_hash_tfm == NULL) {
+			essiv_hash_tfm = crypto_alloc_shash("sha256", 0, 0);
+			if (IS_ERR(essiv_hash_tfm)) {
+				pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld\n",
+						    PTR_ERR(essiv_hash_tfm));
+				err = PTR_ERR(essiv_hash_tfm);
+				essiv_hash_tfm = NULL;
+				mutex_unlock(&essiv_hash_lock);
+				return err;
+			}
+		}
+		mutex_unlock(&essiv_hash_lock);
+	}
+
+	{
+		SHASH_DESC_ON_STACK(desc, essiv_hash_tfm);
+		desc->tfm = essiv_hash_tfm;
+		desc->flags = 0;
+
+		return crypto_shash_digest(desc, key, keysize, salt);
+	}
+}
+
+static int init_essiv_generator(struct fscrypt_info *ci, 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;
+
+	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
+	if (IS_ERR(essiv_tfm))
+		return PTR_ERR(essiv_tfm);
+
+	ci->ci_essiv_tfm = essiv_tfm;
+
+	err = derive_essiv_salt(raw_key, keysize, salt);
+	if (err)
+		goto out;
+
+	err = crypto_cipher_setkey(essiv_tfm, salt, SHA256_DIGEST_SIZE);
+	if (err)
+		goto out;
+
+out:
+	memzero_explicit(salt, sizeof(salt));
+	return err;
+}
+
+void fscrypt_essiv_cleanup(void)
+{
+	crypto_free_shash(essiv_hash_tfm);
+	essiv_hash_tfm = NULL;
+}
+
 int fscrypt_get_encryption_info(struct inode *inode)
 {
 	struct fscrypt_info *crypt_info;
@@ -204,6 +287,10 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (ctx.flags & ~FS_POLICY_FLAGS_VALID)
 		return -EINVAL;
 
+	if (!fscrypt_valid_enc_modes(ctx.contents_encryption_mode,
+				     ctx.filenames_encryption_mode))
+		return -EINVAL;
+
 	crypt_info = kmem_cache_alloc(fscrypt_info_cachep, GFP_NOFS);
 	if (!crypt_info)
 		return -ENOMEM;
@@ -212,6 +299,7 @@ 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_ctfm = NULL;
+	crypt_info->ci_essiv_tfm = NULL;
 	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
 				sizeof(crypt_info->ci_master_key));
 
@@ -228,10 +316,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (!raw_key)
 		goto out;
 
-	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX);
+	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
+				keysize);
 	if (res && inode->i_sb->s_cop->key_prefix) {
 		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
-					     inode->i_sb->s_cop->key_prefix);
+					     inode->i_sb->s_cop->key_prefix,
+					     keysize);
 		if (res2) {
 			if (res2 == -ENOKEY)
 				res = -ENOKEY;
@@ -243,9 +333,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	ctfm = crypto_alloc_skcipher(cipher_str, 0, 0);
 	if (!ctfm || IS_ERR(ctfm)) {
 		res = ctfm ? PTR_ERR(ctfm) : -ENOMEM;
-		printk(KERN_DEBUG
-		       "%s: error %d (inode %u) allocating crypto tfm\n",
-		       __func__, res, (unsigned) inode->i_ino);
+		pr_debug("%s: error %d (inode %lu) allocating crypto tfm\n",
+			 __func__, res, inode->i_ino);
 		goto out;
 	}
 	crypt_info->ci_ctfm = ctfm;
@@ -255,6 +344,14 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (res)
 		goto out;
 
+	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;
+		}
+	}
 	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) == NULL)
 		crypt_info = NULL;
 out:
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 210976e7a269..9914d51dff86 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -38,12 +38,8 @@ static int create_encryption_context_from_policy(struct inode *inode,
 	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
 					FS_KEY_DESCRIPTOR_SIZE);
 
-	if (!fscrypt_valid_contents_enc_mode(
-				policy->contents_encryption_mode))
-		return -EINVAL;
-
-	if (!fscrypt_valid_filenames_enc_mode(
-				policy->filenames_encryption_mode))
+	if (!fscrypt_valid_enc_modes(policy->contents_encryption_mode,
+				     policy->filenames_encryption_mode))
 		return -EINVAL;
 
 	if (policy->flags & ~FS_POLICY_FLAGS_VALID)
diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt_common.h
index 0a30c106c1e5..982c08c4f2ac 100644
--- a/include/linux/fscrypt_common.h
+++ b/include/linux/fscrypt_common.h
@@ -91,14 +91,13 @@ static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
 	return false;
 }
 
-static inline bool fscrypt_valid_contents_enc_mode(u32 mode)
+static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
+					u32 filenames_mode)
 {
-	return (mode == FS_ENCRYPTION_MODE_AES_256_XTS);
-}
-
-static inline bool fscrypt_valid_filenames_enc_mode(u32 mode)
-{
-	return (mode == FS_ENCRYPTION_MODE_AES_256_CTS);
+	return ((contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
+		 filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS) ||
+		(contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
+		 filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS));
 }
 
 static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 24e61a54feaa..a2a3ffb06038 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -272,6 +272,8 @@ struct fsxattr {
 #define FS_ENCRYPTION_MODE_AES_256_GCM		2
 #define FS_ENCRYPTION_MODE_AES_256_CBC		3
 #define FS_ENCRYPTION_MODE_AES_256_CTS		4
+#define FS_ENCRYPTION_MODE_AES_128_CBC		5
+#define FS_ENCRYPTION_MODE_AES_128_CTS		6
 
 struct fscrypt_policy {
 	__u8 version;
-- 
2.12.0

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

[Eric Biggers]

Hi David, thanks for the update!

On Wed, May 17, 2017 at 01:21:04PM +0200, David Gstir wrote:
> From: Daniel Walter <dwalter@sigma-star.at>
> 
> 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. Especially low-powered embedded
> devices with crypto accelerators such as CAAM or CESA support only
> AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
> performance while still providing a moderate level of security for
> persistent storage.

You covered this briefly in an email, but can you include more detail in the
commit message on the reasoning behind choosing AES-128 instead of AES-256?
Note that this is independent of the decision of CBC vs. XTS.

> @@ -129,27 +136,37 @@ static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
>  				 const char **cipher_str_ret, int *keysize_ret)
>  {
>  	if (S_ISREG(inode->i_mode)) {
> -		if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
> +		switch (ci->ci_data_mode) {
> +		case FS_ENCRYPTION_MODE_AES_256_XTS:
>  			*cipher_str_ret = "xts(aes)";
>  			*keysize_ret = FS_AES_256_XTS_KEY_SIZE;
>  			return 0;
> +		case FS_ENCRYPTION_MODE_AES_128_CBC:
> +			*cipher_str_ret = "cbc(aes)";
> +			*keysize_ret = FS_AES_128_CBC_KEY_SIZE;
> +			return 0;
> +		default:
> +			pr_warn_once("fscrypto: unsupported contents encryption mode %d for inode %lu\n",
> +				     ci->ci_data_mode, inode->i_ino);
> +			return -ENOKEY;
>  		}
> -		pr_warn_once("fscrypto: unsupported contents encryption mode "
> -			     "%d for inode %lu\n",
> -			     ci->ci_data_mode, inode->i_ino);
> -		return -ENOKEY;
>  	}
>  
>  	if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
> -		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
> +		switch (ci->ci_filename_mode) {
> +		case FS_ENCRYPTION_MODE_AES_256_CTS:
>  			*cipher_str_ret = "cts(cbc(aes))";
>  			*keysize_ret = FS_AES_256_CTS_KEY_SIZE;
>  			return 0;
> +		case FS_ENCRYPTION_MODE_AES_128_CTS:
> +			*cipher_str_ret = "cts(cbc(aes))";
> +			*keysize_ret = FS_AES_128_CTS_KEY_SIZE;
> +			return 0;
> +		default:
> +			pr_warn_once("fscrypto: unsupported filenames encryption mode %d for inode %lu\n",
> +				     ci->ci_filename_mode, inode->i_ino);
> +			return -ENOKEY;
>  		}
> -		pr_warn_once("fscrypto: unsupported filenames encryption mode "
> -			     "%d for inode %lu\n",
> -			     ci->ci_filename_mode, inode->i_ino);
> -		return -ENOKEY;
>  	}

With the added call to fscrypt_valid_enc_modes() earlier, the warnings about
unsupported encryption modes are no longer reachable.  IMO, the
fscrypt_valid_enc_modes() check should be moved into this function, a proper
warning message added for it, and the redundant warnings removed.  Also now that
there will be more modes I think it would be appropriate to put the algorithm
names and key sizes in a table, to avoid the ugly switch statements.  Here's
what I came up with:

static const struct {
	const char *cipher_str;
	int keysize;
} available_modes[] = {
	[FS_ENCRYPTION_MODE_AES_256_XTS] = { "xts(aes)",
					     FS_AES_256_XTS_KEY_SIZE },
	[FS_ENCRYPTION_MODE_AES_256_CTS] = { "cts(cbc(aes))",
					     FS_AES_256_CTS_KEY_SIZE },
	[FS_ENCRYPTION_MODE_AES_128_CBC] = { "cbc(aes)",
					     FS_AES_128_CBC_KEY_SIZE },
	[FS_ENCRYPTION_MODE_AES_128_CTS] = { "cts(cbc(aes))",
					     FS_AES_128_CTS_KEY_SIZE },
};

static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
				 const char **cipher_str_ret, int *keysize_ret)
{
	u32 mode;

	if (!fscrypt_valid_enc_modes(ci->ci_data_mode, ci->ci_filename_mode)) {
		pr_warn_ratelimited("fscrypt: inode %lu uses unsupported encryption modes "
				    "(contents mode %d, filenames mode %d)\n",
				    inode->i_ino,
				    ci->ci_data_mode, ci->ci_filename_mode);
		return -EINVAL;
	}

	if (S_ISREG(inode->i_mode)) {
		mode = ci->ci_data_mode;
	} else if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
		mode = ci->ci_filename_mode;
	} else {
		WARN_ONCE(1, "fscrypt: filesystem tried to load encryption info for inode %lu, "
			  "which is not encryptable (file type %d)\n",
			  inode->i_ino, (inode->i_mode & S_IFMT));
		return -EINVAL;
	}

	*cipher_str_ret = available_modes[mode].cipher_str;
	*keysize_ret = available_modes[mode].keysize;
	return 0;
}


Note that I changed the 'invalid file type' warning to a WARN_ONCE(), since it
indicates a filesystem bug, unlike the 'unsupported encryption modes' warning
which can be triggered by unrecognized stuff on-disk.

>  
>  	pr_warn_once("fscrypto: unsupported file type %d for inode %lu\n",
> @@ -163,9 +180,75 @@ static void put_crypt_info(struct fscrypt_info *ci)
>  		return;
>  
>  	crypto_free_skcipher(ci->ci_ctfm);
> +	crypto_free_cipher(ci->ci_essiv_tfm);
>  	kmem_cache_free(fscrypt_info_cachep, ci);
>  }
>  
> +static int derive_essiv_salt(u8 *key, int keysize, u8 *salt)
> +{

const u8 *key

> +	int err;
> +
> +	/* init hash transform on demand */
> +	if (unlikely(essiv_hash_tfm == NULL)) {
> +		mutex_lock(&essiv_hash_lock);
> +		if (essiv_hash_tfm == NULL) {
> +			essiv_hash_tfm = crypto_alloc_shash("sha256", 0, 0);
> +			if (IS_ERR(essiv_hash_tfm)) {
> +				pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld\n",
> +						    PTR_ERR(essiv_hash_tfm));
> +				err = PTR_ERR(essiv_hash_tfm);
> +				essiv_hash_tfm = NULL;
> +				mutex_unlock(&essiv_hash_lock);
> +				return err;
> +			}
> +		}
> +		mutex_unlock(&essiv_hash_lock);
> +	}

There is a bug here: a thread can set essiv_hash_tfm to an ERR_PTR(), and
another thread can use it before it's set back to NULL.  Did you consider using
a cmpxchg-based solution instead, similar to what fscrypt_get_encryption_info()
does with ->i_crypt_info?  Then there would be no need for a mutex.  Something
like this:

static int derive_essiv_salt(const u8 *key, int keysize, u8 *salt)
{
        /* init hash transform on demand */
        struct crypto_shash *tfm = READ_ONCE(essiv_hash_tfm);

        if (unlikely(!tfm)) {
                struct crypto_shash *prev;

                tfm = crypto_alloc_shash("sha256", 0, 0);
                if (IS_ERR(tfm)) {
			pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld\n",
					    PTR_ERR(tfm));
                        return PTR_ERR(tfm);
                }
                prev = cmpxchg(&essiv_hash_tfm, NULL, tfm);
                if (prev) {
                        crypto_free_shash(tfm);
                        tfm = prev;
                }
        }

        {
                SHASH_DESC_ON_STACK(desc, tfm);
                desc->tfm = tfm;
                desc->flags = 0;

                return crypto_shash_digest(desc, key, keysize, salt);
        }
}

> +static int init_essiv_generator(struct fscrypt_info *ci, u8 *raw_key,
> +				int keysize)

const u8 *raw_key

> +{
> +	int err;
> +	struct crypto_cipher *essiv_tfm;
> +	u8 salt[SHA256_DIGEST_SIZE];
> +
> +	if (WARN_ON_ONCE(keysize > sizeof(salt)))
> +		return -EINVAL;
> +
> +	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
> +	if (IS_ERR(essiv_tfm))
> +		return PTR_ERR(essiv_tfm);
> +
> +	ci->ci_essiv_tfm = essiv_tfm;
> +
> +	err = derive_essiv_salt(raw_key, keysize, salt);
> +	if (err)
> +		goto out;
> +
> +	err = crypto_cipher_setkey(essiv_tfm, salt, SHA256_DIGEST_SIZE);
> +	if (err)
> +		goto out;

sizeof(salt) instead of hardcoding SHA256_DIGEST_SIZE.

I think there should also be a brief comment explaining that the ESSIV cipher
uses AES-256 so that its key size matches the size of the hash, even though the
"real" encryption may use AES-128.

> +void fscrypt_essiv_cleanup(void)
> +{
> +	crypto_free_shash(essiv_hash_tfm);
> +	essiv_hash_tfm = NULL;
> +}

This is called from fscrypt_destroy(), which is a little weird because
fscrypt_destroy() is meant to clean up only from "fscrypt_initialize()", which
only allocates certain things.  I think it should be called from
"fscrypt_exit()" instead.  Then you could also add the __exit annotation, and
remove setting essiv_hash_tfm to NULL which would clearly be unnecessary.

> +
>  int fscrypt_get_encryption_info(struct inode *inode)
>  {
>  	struct fscrypt_info *crypt_info;
> @@ -204,6 +287,10 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	if (ctx.flags & ~FS_POLICY_FLAGS_VALID)
>  		return -EINVAL;
>  
> +	if (!fscrypt_valid_enc_modes(ctx.contents_encryption_mode,
> +				     ctx.filenames_encryption_mode))
> +		return -EINVAL;
> +

As noted earlier I think this should be moved into determine_cipher_type(), to
avoid redundancy when interpreting the encryption modes.

> diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt_common.h
> index 0a30c106c1e5..982c08c4f2ac 100644
> --- a/include/linux/fscrypt_common.h
> +++ b/include/linux/fscrypt_common.h
> @@ -91,14 +91,13 @@ static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
>  	return false;
>  }
>  
> -static inline bool fscrypt_valid_contents_enc_mode(u32 mode)
> +static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
> +					u32 filenames_mode)
>  {
> -	return (mode == FS_ENCRYPTION_MODE_AES_256_XTS);
> -}
> -
> -static inline bool fscrypt_valid_filenames_enc_mode(u32 mode)
> -{
> -	return (mode == FS_ENCRYPTION_MODE_AES_256_CTS);
> +	return ((contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
> +		 filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS) ||
> +		(contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
> +		 filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS));
>  }
>  

IMO, make these 'if' statements, to discourage people from turning this
expression into more of a mess when they add more modes:

static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
                                        u32 filenames_mode)
{
        if (contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
            filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS)
                return true;

        if (contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
            filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS)
                return true;

        return false;
}

Eric

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

[David Gstir]

[-- Attachment #1: Type: text/plain, Size: 5877 bytes --]

Hi Eric!

Thanks for the thorough review! :)

> On 17 May 2017, at 20:08, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> Hi David, thanks for the update!
> 
> On Wed, May 17, 2017 at 01:21:04PM +0200, David Gstir wrote:
>> From: Daniel Walter <dwalter@sigma-star.at>
>> 
>> 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. Especially low-powered embedded
>> devices with crypto accelerators such as CAAM or CESA support only
>> AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
>> performance while still providing a moderate level of security for
>> persistent storage.
> 
> You covered this briefly in an email, but can you include more detail in the
> commit message on the reasoning behind choosing AES-128 instead of AES-256?
> Note that this is independent of the decision of CBC vs. XTS.

Sure, I'll extend the commit message to include that.


> 
>> @@ -129,27 +136,37 @@ static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
>> 				 const char **cipher_str_ret, int *keysize_ret)
>> {
>> 	if (S_ISREG(inode->i_mode)) {
>> -		if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
>> +		switch (ci->ci_data_mode) {
>> +		case FS_ENCRYPTION_MODE_AES_256_XTS:
>> 			*cipher_str_ret = "xts(aes)";
>> 			*keysize_ret = FS_AES_256_XTS_KEY_SIZE;
>> 			return 0;
>> +		case FS_ENCRYPTION_MODE_AES_128_CBC:
>> +			*cipher_str_ret = "cbc(aes)";
>> +			*keysize_ret = FS_AES_128_CBC_KEY_SIZE;
>> +			return 0;
>> +		default:
>> +			pr_warn_once("fscrypto: unsupported contents encryption mode %d for inode %lu\n",
>> +				     ci->ci_data_mode, inode->i_ino);
>> +			return -ENOKEY;
>> 		}
>> -		pr_warn_once("fscrypto: unsupported contents encryption mode "
>> -			     "%d for inode %lu\n",
>> -			     ci->ci_data_mode, inode->i_ino);
>> -		return -ENOKEY;
>> 	}
>> 
>> 	if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
>> -		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
>> +		switch (ci->ci_filename_mode) {
>> +		case FS_ENCRYPTION_MODE_AES_256_CTS:
>> 			*cipher_str_ret = "cts(cbc(aes))";
>> 			*keysize_ret = FS_AES_256_CTS_KEY_SIZE;
>> 			return 0;
>> +		case FS_ENCRYPTION_MODE_AES_128_CTS:
>> +			*cipher_str_ret = "cts(cbc(aes))";
>> +			*keysize_ret = FS_AES_128_CTS_KEY_SIZE;
>> +			return 0;
>> +		default:
>> +			pr_warn_once("fscrypto: unsupported filenames encryption mode %d for inode %lu\n",
>> +				     ci->ci_filename_mode, inode->i_ino);
>> +			return -ENOKEY;
>> 		}
>> -		pr_warn_once("fscrypto: unsupported filenames encryption mode "
>> -			     "%d for inode %lu\n",
>> -			     ci->ci_filename_mode, inode->i_ino);
>> -		return -ENOKEY;
>> 	}
> 
> With the added call to fscrypt_valid_enc_modes() earlier, the warnings about
> unsupported encryption modes are no longer reachable.  IMO, the
> fscrypt_valid_enc_modes() check should be moved into this function, a proper
> warning message added for it, and the redundant warnings removed.  Also now that
> there will be more modes I think it would be appropriate to put the algorithm
> names and key sizes in a table, to avoid the ugly switch statements.  

I agree. I'll clean this up.



>> +	int err;
>> +
>> +	/* init hash transform on demand */
>> +	if (unlikely(essiv_hash_tfm == NULL)) {
>> +		mutex_lock(&essiv_hash_lock);
>> +		if (essiv_hash_tfm == NULL) {
>> +			essiv_hash_tfm = crypto_alloc_shash("sha256", 0, 0);
>> +			if (IS_ERR(essiv_hash_tfm)) {
>> +				pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld\n",
>> +						    PTR_ERR(essiv_hash_tfm));
>> +				err = PTR_ERR(essiv_hash_tfm);
>> +				essiv_hash_tfm = NULL;
>> +				mutex_unlock(&essiv_hash_lock);
>> +				return err;
>> +			}
>> +		}
>> +		mutex_unlock(&essiv_hash_lock);
>> +	}
> 
> There is a bug here: a thread can set essiv_hash_tfm to an ERR_PTR(), and
> another thread can use it before it's set back to NULL.  

Sorry, I missed that... :-(



>> +	err = crypto_cipher_setkey(essiv_tfm, salt, SHA256_DIGEST_SIZE);
>> +	if (err)
>> +		goto out;
> 
> sizeof(salt) instead of hardcoding SHA256_DIGEST_SIZE.
> 
> I think there should also be a brief comment explaining that the ESSIV cipher
> uses AES-256 so that its key size matches the size of the hash, even though the
> "real" encryption may use AES-128.

Good point!


> 
>> +void fscrypt_essiv_cleanup(void)
>> +{
>> +	crypto_free_shash(essiv_hash_tfm);
>> +	essiv_hash_tfm = NULL;
>> +}
> 
> This is called from fscrypt_destroy(), which is a little weird because
> fscrypt_destroy() is meant to clean up only from "fscrypt_initialize()", which
> only allocates certain things.  I think it should be called from
> "fscrypt_exit()" instead.  Then you could also add the __exit annotation, and
> remove setting essiv_hash_tfm to NULL which would clearly be unnecessary.

fscrypt_destroy() is actually also called from fscrypt_exit(). Thats why I chose it,
but changing this fscrypt_exit() seems the cleaner approach. :)

Thanks,
David

--
sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, Austria
UID/VAT Nr: ATU 66964118 | FN: 374287y


[-- Attachment #2: Type: text/html, Size: 20167 bytes --]

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

[David Gstir]

From: Daniel Walter <dwalter@sigma-star.at>

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 <dwalter@sigma-star.at>
[david@sigma-star.at: addressed review comments]
Signed-off-by: David Gstir <david@sigma-star.at>
---
 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 <linux/ratelimit.h>
 #include <linux/dcache.h>
 #include <linux/namei.h>
+#include <crypto/aes.h>
 #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 *)&iv,
+					  (u8 *)&iv);
+	}
+
 	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, &ecr);
 
-	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(&dst, 1);
 	sg_set_page(&dst, dest_page, len, offs);
 	sg_init_table(&src, 1);
 	sg_set_page(&src, src_page, len, offs);
-	skcipher_request_set_crypt(req, &src, &dst, len, &xts_tweak);
+	skcipher_request_set_crypt(req, &src, &dst, len, &iv);
 	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 <linux/fscrypt_supp.h>
+#include <crypto/hash.h>
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE		16
+#define FS_IV_SIZE			16
 #define FS_AES_128_ECB_KEY_SIZE		16
+#define FS_AES_128_CBC_KEY_SIZE		16
+#define FS_AES_128_CTS_KEY_SIZE		16
 #define FS_AES_256_GCM_KEY_SIZE		32
 #define FS_AES_256_CBC_KEY_SIZE		32
 #define FS_AES_256_CTS_KEY_SIZE		32
@@ -54,6 +57,7 @@ struct fscrypt_info {
 	u8 ci_filename_mode;
 	u8 ci_flags;
 	struct crypto_skcipher *ci_ctfm;
+	struct crypto_cipher *ci_essiv_tfm;
 	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
 };
 
@@ -87,4 +91,7 @@ extern int fscrypt_do_page_crypto(const struct inode *inode,
 extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
 					      gfp_t gfp_flags);
 
+/* keyinfo.c */
+extern void __exit fscrypt_essiv_cleanup(void);
+
 #endif /* _FSCRYPT_PRIVATE_H */
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 179e578b875b..b8f5e1d5a3cc 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,8 +10,13 @@
 
 #include <keys/user-type.h>
 #include <linux/scatterlist.h>
+#include <linux/ratelimit.h>
+#include <crypto/aes.h>
+#include <crypto/sha.h>
 #include "fscrypt_private.h"
 
+static struct crypto_shash *essiv_hash_tfm;
+
 static void derive_crypt_complete(struct crypto_async_request *req, int rc)
 {
 	struct fscrypt_completion_result *ecr = req->data;
@@ -27,13 +32,13 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc)
  * derive_key_aes() - Derive a key using AES-128-ECB
  * @deriving_key: Encryption key used for derivation.
  * @source_key:   Source key to which to apply derivation.
- * @derived_key:  Derived key.
+ * @derived_raw_key:  Derived raw key.
  *
  * Return: Zero on success; non-zero otherwise.
  */
 static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
-				u8 source_key[FS_AES_256_XTS_KEY_SIZE],
-				u8 derived_key[FS_AES_256_XTS_KEY_SIZE])
+				const struct fscrypt_key *source_key,
+				u8 derived_raw_key[FS_MAX_KEY_SIZE])
 {
 	int res = 0;
 	struct skcipher_request *req = NULL;
@@ -60,10 +65,10 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 	if (res < 0)
 		goto out;
 
-	sg_init_one(&src_sg, source_key, FS_AES_256_XTS_KEY_SIZE);
-	sg_init_one(&dst_sg, derived_key, FS_AES_256_XTS_KEY_SIZE);
-	skcipher_request_set_crypt(req, &src_sg, &dst_sg,
-					FS_AES_256_XTS_KEY_SIZE, NULL);
+	sg_init_one(&src_sg, source_key->raw, source_key->size);
+	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
+	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
+				   NULL);
 	res = crypto_skcipher_encrypt(req);
 	if (res == -EINPROGRESS || res == -EBUSY) {
 		wait_for_completion(&ecr.completion);
@@ -77,7 +82,7 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 
 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) {
 		printk_once(KERN_WARNING
 				"%s: key size incorrect: %d\n",
 				__func__, master_key->size);
 		res = -ENOKEY;
 		goto out;
 	}
-	res = derive_key_aes(ctx->nonce, master_key->raw, raw_key);
+	res = derive_key_aes(ctx->nonce, master_key, raw_key);
 out:
 	up_read(&keyring_key->sem);
 	key_put(keyring_key);
 	return res;
 }
 
+static const struct {
+	const char *cipher_str;
+	int keysize;
+} available_modes[] = {
+	[FS_ENCRYPTION_MODE_AES_256_XTS] = { "xts(aes)",
+					     FS_AES_256_XTS_KEY_SIZE },
+	[FS_ENCRYPTION_MODE_AES_256_CTS] = { "cts(cbc(aes))",
+					     FS_AES_256_CTS_KEY_SIZE },
+	[FS_ENCRYPTION_MODE_AES_128_CBC] = { "cbc(aes)",
+					     FS_AES_128_CBC_KEY_SIZE },
+	[FS_ENCRYPTION_MODE_AES_128_CTS] = { "cts(cbc(aes))",
+					     FS_AES_128_CTS_KEY_SIZE },
+};
+
 static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
 				 const char **cipher_str_ret, int *keysize_ret)
 {
-	if (S_ISREG(inode->i_mode)) {
-		if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
-			*cipher_str_ret = "xts(aes)";
-			*keysize_ret = FS_AES_256_XTS_KEY_SIZE;
-			return 0;
-		}
-		pr_warn_once("fscrypto: unsupported contents encryption mode "
-			     "%d for inode %lu\n",
-			     ci->ci_data_mode, inode->i_ino);
-		return -ENOKEY;
+	u32 mode;
+
+	if (!fscrypt_valid_enc_modes(ci->ci_data_mode, ci->ci_filename_mode)) {
+		pr_warn_ratelimited("fscrypt: inode %lu uses unsupported encryption modes (contents mode %d, filenames mode %d)\n",
+				    inode->i_ino,
+				    ci->ci_data_mode, ci->ci_filename_mode);
+		return -EINVAL;
 	}
 
-	if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
-		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
-			*cipher_str_ret = "cts(cbc(aes))";
-			*keysize_ret = FS_AES_256_CTS_KEY_SIZE;
-			return 0;
-		}
-		pr_warn_once("fscrypto: unsupported filenames encryption mode "
-			     "%d for inode %lu\n",
-			     ci->ci_filename_mode, inode->i_ino);
-		return -ENOKEY;
+	if (S_ISREG(inode->i_mode)) {
+		mode = ci->ci_data_mode;
+	} else if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
+		mode = ci->ci_filename_mode;
+	} else {
+		WARN_ONCE(1, "fscrypt: filesystem tried to load encryption info for inode %lu, which is not encryptable (file type %d)\n",
+			  inode->i_ino, (inode->i_mode & S_IFMT));
+		return -EINVAL;
 	}
 
-	pr_warn_once("fscrypto: unsupported file type %d for inode %lu\n",
-		     (inode->i_mode & S_IFMT), inode->i_ino);
-	return -ENOKEY;
+	*cipher_str_ret = available_modes[mode].cipher_str;
+	*keysize_ret = available_modes[mode].keysize;
+	return 0;
 }
 
 static void put_crypt_info(struct fscrypt_info *ci)
@@ -163,9 +178,79 @@ static void put_crypt_info(struct fscrypt_info *ci)
 		return;
 
 	crypto_free_skcipher(ci->ci_ctfm);
+	crypto_free_cipher(ci->ci_essiv_tfm);
 	kmem_cache_free(fscrypt_info_cachep, ci);
 }
 
+static int derive_essiv_salt(const u8 *key, int keysize, u8 *salt)
+{
+	struct crypto_shash *tfm = READ_ONCE(essiv_hash_tfm);
+
+	/* init hash transform on demand */
+	if (unlikely(!tfm)) {
+		struct crypto_shash *prev_tfm;
+
+		tfm = crypto_alloc_shash("sha256", 0, 0);
+		if (IS_ERR(tfm)) {
+			pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld\n",
+					    PTR_ERR(tfm));
+			return PTR_ERR(tfm);
+		}
+		prev_tfm = cmpxchg(&essiv_hash_tfm, NULL, tfm);
+		if (prev_tfm) {
+			crypto_free_shash(tfm);
+			tfm = prev_tfm;
+		}
+	}
+
+	{
+		SHASH_DESC_ON_STACK(desc, tfm);
+		desc->tfm = tfm;
+		desc->flags = 0;
+
+		return crypto_shash_digest(desc, key, keysize, salt);
+	}
+}
+
+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;
+
+	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
+	if (IS_ERR(essiv_tfm))
+		return PTR_ERR(essiv_tfm);
+
+	ci->ci_essiv_tfm = essiv_tfm;
+
+	err = derive_essiv_salt(raw_key, keysize, salt);
+	if (err)
+		goto out;
+
+	/*
+	 * Using SHA256 to derive the salt/key will result in AES-256 being
+	 * used for IV generation. File contents encryption will still use the
+	 * configured keysize (AES-128) nevertheless.
+	 */
+	err = crypto_cipher_setkey(essiv_tfm, salt, sizeof(salt));
+	if (err)
+		goto out;
+
+out:
+	memzero_explicit(salt, sizeof(salt));
+	return err;
+}
+
+void __exit fscrypt_essiv_cleanup(void)
+{
+	crypto_free_shash(essiv_hash_tfm);
+}
+
 int fscrypt_get_encryption_info(struct inode *inode)
 {
 	struct fscrypt_info *crypt_info;
@@ -212,6 +297,7 @@ 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_ctfm = NULL;
+	crypt_info->ci_essiv_tfm = NULL;
 	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
 				sizeof(crypt_info->ci_master_key));
 
@@ -228,10 +314,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (!raw_key)
 		goto out;
 
-	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX);
+	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
+				keysize);
 	if (res && inode->i_sb->s_cop->key_prefix) {
 		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
-					     inode->i_sb->s_cop->key_prefix);
+					     inode->i_sb->s_cop->key_prefix,
+					     keysize);
 		if (res2) {
 			if (res2 == -ENOKEY)
 				res = -ENOKEY;
@@ -243,9 +331,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	ctfm = crypto_alloc_skcipher(cipher_str, 0, 0);
 	if (!ctfm || IS_ERR(ctfm)) {
 		res = ctfm ? PTR_ERR(ctfm) : -ENOMEM;
-		printk(KERN_DEBUG
-		       "%s: error %d (inode %u) allocating crypto tfm\n",
-		       __func__, res, (unsigned) inode->i_ino);
+		pr_debug("%s: error %d (inode %lu) allocating crypto tfm\n",
+			 __func__, res, inode->i_ino);
 		goto out;
 	}
 	crypt_info->ci_ctfm = ctfm;
@@ -255,6 +342,14 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (res)
 		goto out;
 
+	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;
+		}
+	}
 	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) == NULL)
 		crypt_info = NULL;
 out:
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 210976e7a269..9914d51dff86 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -38,12 +38,8 @@ static int create_encryption_context_from_policy(struct inode *inode,
 	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
 					FS_KEY_DESCRIPTOR_SIZE);
 
-	if (!fscrypt_valid_contents_enc_mode(
-				policy->contents_encryption_mode))
-		return -EINVAL;
-
-	if (!fscrypt_valid_filenames_enc_mode(
-				policy->filenames_encryption_mode))
+	if (!fscrypt_valid_enc_modes(policy->contents_encryption_mode,
+				     policy->filenames_encryption_mode))
 		return -EINVAL;
 
 	if (policy->flags & ~FS_POLICY_FLAGS_VALID)
diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt_common.h
index 0a30c106c1e5..4022c61f7e9b 100644
--- a/include/linux/fscrypt_common.h
+++ b/include/linux/fscrypt_common.h
@@ -91,14 +91,18 @@ static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
 	return false;
 }
 
-static inline bool fscrypt_valid_contents_enc_mode(u32 mode)
+static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
+					u32 filenames_mode)
 {
-	return (mode == FS_ENCRYPTION_MODE_AES_256_XTS);
-}
+	if (contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
+	    filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS)
+		return true;
 
-static inline bool fscrypt_valid_filenames_enc_mode(u32 mode)
-{
-	return (mode == FS_ENCRYPTION_MODE_AES_256_CTS);
+	if (contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
+	    filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS)
+		return true;
+
+	return false;
 }
 
 static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 24e61a54feaa..a2a3ffb06038 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -272,6 +272,8 @@ struct fsxattr {
 #define FS_ENCRYPTION_MODE_AES_256_GCM		2
 #define FS_ENCRYPTION_MODE_AES_256_CBC		3
 #define FS_ENCRYPTION_MODE_AES_256_CTS		4
+#define FS_ENCRYPTION_MODE_AES_128_CBC		5
+#define FS_ENCRYPTION_MODE_AES_128_CTS		6
 
 struct fscrypt_policy {
 	__u8 version;
-- 
2.12.0

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 <dwalter@sigma-star.at>
> 
> 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 <dwalter@sigma-star.at>
> [david@sigma-star.at: addressed review comments]
> Signed-off-by: David Gstir <david@sigma-star.at>

Overall this looks good now; you can add

Reviewed-by: Eric Biggers <ebiggers@google.com>

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

[David Gstir]

Hi Eric,

> On 23 May 2017, at 21:00, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> Hi David,
> 
> On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
>> From: Daniel Walter <dwalter@sigma-star.at>
>> 
>> 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 <dwalter@sigma-star.at>
>> [david@sigma-star.at: addressed review comments]
>> Signed-off-by: David Gstir <david@sigma-star.at>
> 
> Overall this looks good now; you can add
> 
> Reviewed-by: Eric Biggers <ebiggers@google.com>

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

[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

[Michael Halcrow]

On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
> From: Daniel Walter <dwalter@sigma-star.at>
> 
> 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 <dwalter@sigma-star.at>
> [david@sigma-star.at: addressed review comments]
> Signed-off-by: David Gstir <david@sigma-star.at>
> ---
>  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 <linux/ratelimit.h>
>  #include <linux/dcache.h>
>  #include <linux/namei.h>
> +#include <crypto/aes.h>
>  #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 *)&iv,
> +					  (u8 *)&iv);
> +	}
> +
>  	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, &ecr);
>  
> -	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(&dst, 1);
>  	sg_set_page(&dst, dest_page, len, offs);
>  	sg_init_table(&src, 1);
>  	sg_set_page(&src, src_page, len, offs);
> -	skcipher_request_set_crypt(req, &src, &dst, len, &xts_tweak);
> +	skcipher_request_set_crypt(req, &src, &dst, len, &iv);
>  	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 <linux/fscrypt_supp.h>
> +#include <crypto/hash.h>
>  
>  /* Encryption parameters */
> -#define FS_XTS_TWEAK_SIZE		16
> +#define FS_IV_SIZE			16
>  #define FS_AES_128_ECB_KEY_SIZE		16
> +#define FS_AES_128_CBC_KEY_SIZE		16
> +#define FS_AES_128_CTS_KEY_SIZE		16
>  #define FS_AES_256_GCM_KEY_SIZE		32
>  #define FS_AES_256_CBC_KEY_SIZE		32
>  #define FS_AES_256_CTS_KEY_SIZE		32
> @@ -54,6 +57,7 @@ struct fscrypt_info {
>  	u8 ci_filename_mode;
>  	u8 ci_flags;
>  	struct crypto_skcipher *ci_ctfm;
> +	struct crypto_cipher *ci_essiv_tfm;
>  	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
>  };
>  
> @@ -87,4 +91,7 @@ extern int fscrypt_do_page_crypto(const struct inode *inode,
>  extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
>  					      gfp_t gfp_flags);
>  
> +/* keyinfo.c */
> +extern void __exit fscrypt_essiv_cleanup(void);
> +
>  #endif /* _FSCRYPT_PRIVATE_H */
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 179e578b875b..b8f5e1d5a3cc 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -10,8 +10,13 @@
>  
>  #include <keys/user-type.h>
>  #include <linux/scatterlist.h>
> +#include <linux/ratelimit.h>
> +#include <crypto/aes.h>
> +#include <crypto/sha.h>
>  #include "fscrypt_private.h"
>  
> +static struct crypto_shash *essiv_hash_tfm;
> +
>  static void derive_crypt_complete(struct crypto_async_request *req, int rc)
>  {
>  	struct fscrypt_completion_result *ecr = req->data;
> @@ -27,13 +32,13 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc)
>   * derive_key_aes() - Derive a key using AES-128-ECB
>   * @deriving_key: Encryption key used for derivation.
>   * @source_key:   Source key to which to apply derivation.
> - * @derived_key:  Derived key.
> + * @derived_raw_key:  Derived raw key.
>   *
>   * Return: Zero on success; non-zero otherwise.
>   */
>  static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
> -				u8 source_key[FS_AES_256_XTS_KEY_SIZE],
> -				u8 derived_key[FS_AES_256_XTS_KEY_SIZE])
> +				const struct fscrypt_key *source_key,
> +				u8 derived_raw_key[FS_MAX_KEY_SIZE])
>  {
>  	int res = 0;
>  	struct skcipher_request *req = NULL;
> @@ -60,10 +65,10 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
>  	if (res < 0)
>  		goto out;
>  
> -	sg_init_one(&src_sg, source_key, FS_AES_256_XTS_KEY_SIZE);
> -	sg_init_one(&dst_sg, derived_key, FS_AES_256_XTS_KEY_SIZE);
> -	skcipher_request_set_crypt(req, &src_sg, &dst_sg,
> -					FS_AES_256_XTS_KEY_SIZE, NULL);
> +	sg_init_one(&src_sg, source_key->raw, source_key->size);
> +	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
> +	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
> +				   NULL);
>  	res = crypto_skcipher_encrypt(req);
>  	if (res == -EINPROGRESS || res == -EBUSY) {
>  		wait_for_completion(&ecr.completion);
> @@ -77,7 +82,7 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
>  
>  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.

>  		printk_once(KERN_WARNING
>  				"%s: key size incorrect: %d\n",
>  				__func__, master_key->size);
>  		res = -ENOKEY;
>  		goto out;
>  	}
> -	res = derive_key_aes(ctx->nonce, master_key->raw, raw_key);
> +	res = derive_key_aes(ctx->nonce, master_key, raw_key);
>  out:
>  	up_read(&keyring_key->sem);
>  	key_put(keyring_key);
>  	return res;
>  }
>  
> +static const struct {
> +	const char *cipher_str;
> +	int keysize;
> +} available_modes[] = {
> +	[FS_ENCRYPTION_MODE_AES_256_XTS] = { "xts(aes)",
> +					     FS_AES_256_XTS_KEY_SIZE },
> +	[FS_ENCRYPTION_MODE_AES_256_CTS] = { "cts(cbc(aes))",
> +					     FS_AES_256_CTS_KEY_SIZE },
> +	[FS_ENCRYPTION_MODE_AES_128_CBC] = { "cbc(aes)",
> +					     FS_AES_128_CBC_KEY_SIZE },
> +	[FS_ENCRYPTION_MODE_AES_128_CTS] = { "cts(cbc(aes))",
> +					     FS_AES_128_CTS_KEY_SIZE },
> +};
> +
>  static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
>  				 const char **cipher_str_ret, int *keysize_ret)
>  {
> -	if (S_ISREG(inode->i_mode)) {
> -		if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
> -			*cipher_str_ret = "xts(aes)";
> -			*keysize_ret = FS_AES_256_XTS_KEY_SIZE;
> -			return 0;
> -		}
> -		pr_warn_once("fscrypto: unsupported contents encryption mode "
> -			     "%d for inode %lu\n",
> -			     ci->ci_data_mode, inode->i_ino);
> -		return -ENOKEY;
> +	u32 mode;
> +
> +	if (!fscrypt_valid_enc_modes(ci->ci_data_mode, ci->ci_filename_mode)) {
> +		pr_warn_ratelimited("fscrypt: inode %lu uses unsupported encryption modes (contents mode %d, filenames mode %d)\n",
> +				    inode->i_ino,
> +				    ci->ci_data_mode, ci->ci_filename_mode);
> +		return -EINVAL;
>  	}
>  
> -	if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
> -		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
> -			*cipher_str_ret = "cts(cbc(aes))";
> -			*keysize_ret = FS_AES_256_CTS_KEY_SIZE;
> -			return 0;
> -		}
> -		pr_warn_once("fscrypto: unsupported filenames encryption mode "
> -			     "%d for inode %lu\n",
> -			     ci->ci_filename_mode, inode->i_ino);
> -		return -ENOKEY;
> +	if (S_ISREG(inode->i_mode)) {
> +		mode = ci->ci_data_mode;
> +	} else if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
> +		mode = ci->ci_filename_mode;
> +	} else {
> +		WARN_ONCE(1, "fscrypt: filesystem tried to load encryption info for inode %lu, which is not encryptable (file type %d)\n",
> +			  inode->i_ino, (inode->i_mode & S_IFMT));
> +		return -EINVAL;
>  	}
>  
> -	pr_warn_once("fscrypto: unsupported file type %d for inode %lu\n",
> -		     (inode->i_mode & S_IFMT), inode->i_ino);
> -	return -ENOKEY;
> +	*cipher_str_ret = available_modes[mode].cipher_str;
> +	*keysize_ret = available_modes[mode].keysize;
> +	return 0;
>  }
>  
>  static void put_crypt_info(struct fscrypt_info *ci)
> @@ -163,9 +178,79 @@ static void put_crypt_info(struct fscrypt_info *ci)
>  		return;
>  
>  	crypto_free_skcipher(ci->ci_ctfm);
> +	crypto_free_cipher(ci->ci_essiv_tfm);
>  	kmem_cache_free(fscrypt_info_cachep, ci);
>  }
>  
> +static int derive_essiv_salt(const u8 *key, int keysize, u8 *salt)
> +{
> +	struct crypto_shash *tfm = READ_ONCE(essiv_hash_tfm);
> +
> +	/* init hash transform on demand */
> +	if (unlikely(!tfm)) {
> +		struct crypto_shash *prev_tfm;
> +
> +		tfm = crypto_alloc_shash("sha256", 0, 0);
> +		if (IS_ERR(tfm)) {
> +			pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld\n",
> +					    PTR_ERR(tfm));
> +			return PTR_ERR(tfm);
> +		}
> +		prev_tfm = cmpxchg(&essiv_hash_tfm, NULL, tfm);
> +		if (prev_tfm) {
> +			crypto_free_shash(tfm);
> +			tfm = prev_tfm;
> +		}
> +	}
> +
> +	{
> +		SHASH_DESC_ON_STACK(desc, tfm);
> +		desc->tfm = tfm;
> +		desc->flags = 0;
> +
> +		return crypto_shash_digest(desc, key, keysize, salt);
> +	}
> +}
> +
> +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;
> +
> +	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
> +	if (IS_ERR(essiv_tfm))
> +		return PTR_ERR(essiv_tfm);
> +
> +	ci->ci_essiv_tfm = essiv_tfm;
> +
> +	err = derive_essiv_salt(raw_key, keysize, salt);
> +	if (err)
> +		goto out;
> +
> +	/*
> +	 * Using SHA256 to derive the salt/key will result in AES-256 being
> +	 * used for IV generation. File contents encryption will still use the

It would probably be fine to just truncate the generated salt and
stick with AES-128 throughout.  However that's just a footnote, and
I think you're fine to keep it the way it is now.

> +	 * configured keysize (AES-128) nevertheless.
> +	 */
> +	err = crypto_cipher_setkey(essiv_tfm, salt, sizeof(salt));
> +	if (err)
> +		goto out;
> +
> +out:
> +	memzero_explicit(salt, sizeof(salt));
> +	return err;
> +}
> +
> +void __exit fscrypt_essiv_cleanup(void)
> +{
> +	crypto_free_shash(essiv_hash_tfm);
> +}
> +
>  int fscrypt_get_encryption_info(struct inode *inode)
>  {
>  	struct fscrypt_info *crypt_info;
> @@ -212,6 +297,7 @@ 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_ctfm = NULL;
> +	crypt_info->ci_essiv_tfm = NULL;
>  	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
>  				sizeof(crypt_info->ci_master_key));
>  
> @@ -228,10 +314,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	if (!raw_key)
>  		goto out;
>  
> -	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX);
> +	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> +				keysize);
>  	if (res && inode->i_sb->s_cop->key_prefix) {
>  		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> -					     inode->i_sb->s_cop->key_prefix);
> +					     inode->i_sb->s_cop->key_prefix,
> +					     keysize);
>  		if (res2) {
>  			if (res2 == -ENOKEY)
>  				res = -ENOKEY;
> @@ -243,9 +331,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	ctfm = crypto_alloc_skcipher(cipher_str, 0, 0);
>  	if (!ctfm || IS_ERR(ctfm)) {
>  		res = ctfm ? PTR_ERR(ctfm) : -ENOMEM;
> -		printk(KERN_DEBUG
> -		       "%s: error %d (inode %u) allocating crypto tfm\n",
> -		       __func__, res, (unsigned) inode->i_ino);
> +		pr_debug("%s: error %d (inode %lu) allocating crypto tfm\n",
> +			 __func__, res, inode->i_ino);
>  		goto out;
>  	}
>  	crypt_info->ci_ctfm = ctfm;
> @@ -255,6 +342,14 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	if (res)
>  		goto out;
>  
> +	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;
> +		}
> +	}
>  	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) == NULL)
>  		crypt_info = NULL;
>  out:
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 210976e7a269..9914d51dff86 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -38,12 +38,8 @@ static int create_encryption_context_from_policy(struct inode *inode,
>  	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
>  					FS_KEY_DESCRIPTOR_SIZE);
>  
> -	if (!fscrypt_valid_contents_enc_mode(
> -				policy->contents_encryption_mode))
> -		return -EINVAL;
> -
> -	if (!fscrypt_valid_filenames_enc_mode(
> -				policy->filenames_encryption_mode))
> +	if (!fscrypt_valid_enc_modes(policy->contents_encryption_mode,
> +				     policy->filenames_encryption_mode))
>  		return -EINVAL;
>  
>  	if (policy->flags & ~FS_POLICY_FLAGS_VALID)
> diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt_common.h
> index 0a30c106c1e5..4022c61f7e9b 100644
> --- a/include/linux/fscrypt_common.h
> +++ b/include/linux/fscrypt_common.h
> @@ -91,14 +91,18 @@ static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
>  	return false;
>  }
>  
> -static inline bool fscrypt_valid_contents_enc_mode(u32 mode)
> +static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
> +					u32 filenames_mode)
>  {
> -	return (mode == FS_ENCRYPTION_MODE_AES_256_XTS);
> -}
> +	if (contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
> +	    filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS)
> +		return true;
>  
> -static inline bool fscrypt_valid_filenames_enc_mode(u32 mode)
> -{
> -	return (mode == FS_ENCRYPTION_MODE_AES_256_CTS);
> +	if (contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
> +	    filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS)
> +		return true;
> +
> +	return false;
>  }
>  
>  static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 24e61a54feaa..a2a3ffb06038 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -272,6 +272,8 @@ struct fsxattr {
>  #define FS_ENCRYPTION_MODE_AES_256_GCM		2
>  #define FS_ENCRYPTION_MODE_AES_256_CBC		3
>  #define FS_ENCRYPTION_MODE_AES_256_CTS		4
> +#define FS_ENCRYPTION_MODE_AES_128_CBC		5
> +#define FS_ENCRYPTION_MODE_AES_128_CTS		6
>  
>  struct fscrypt_policy {
>  	__u8 version;
> -- 
> 2.12.0
> 

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

[David Gstir]

> On 15 Jun 2017, at 22:48, Eric Biggers <ebiggers3@gmail.com> 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

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

[David Gstir]

From: Daniel Walter <dwalter@sigma-star.at>

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 <dwalter@sigma-star.at>
[david@sigma-star.at: addressed review comments]
Signed-off-by: David Gstir <david@sigma-star.at>
Reviewed-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/Kconfig              |   1 +
 fs/crypto/crypto.c             |  23 ++++--
 fs/crypto/fscrypt_private.h    |   9 ++-
 fs/crypto/keyinfo.c            | 173 ++++++++++++++++++++++++++++++++---------
 fs/crypto/policy.c             |   8 +-
 include/linux/fscrypt_common.h |  16 ++--
 include/uapi/linux/fs.h        |   2 +
 7 files changed, 174 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 <linux/ratelimit.h>
 #include <linux/dcache.h>
 #include <linux/namei.h>
+#include <crypto/aes.h>
 #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 *)&iv,
+					  (u8 *)&iv);
+	}
+
 	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, &ecr);
 
-	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(&dst, 1);
 	sg_set_page(&dst, dest_page, len, offs);
 	sg_init_table(&src, 1);
 	sg_set_page(&src, src_page, len, offs);
-	skcipher_request_set_crypt(req, &src, &dst, len, &xts_tweak);
+	skcipher_request_set_crypt(req, &src, &dst, len, &iv);
 	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 <linux/fscrypt_supp.h>
+#include <crypto/hash.h>
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE		16
+#define FS_IV_SIZE			16
 #define FS_AES_128_ECB_KEY_SIZE		16
+#define FS_AES_128_CBC_KEY_SIZE		16
+#define FS_AES_128_CTS_KEY_SIZE		16
 #define FS_AES_256_GCM_KEY_SIZE		32
 #define FS_AES_256_CBC_KEY_SIZE		32
 #define FS_AES_256_CTS_KEY_SIZE		32
@@ -54,6 +57,7 @@ struct fscrypt_info {
 	u8 ci_filename_mode;
 	u8 ci_flags;
 	struct crypto_skcipher *ci_ctfm;
+	struct crypto_cipher *ci_essiv_tfm;
 	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
 };
 
@@ -87,4 +91,7 @@ extern int fscrypt_do_page_crypto(const struct inode *inode,
 extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
 					      gfp_t gfp_flags);
 
+/* keyinfo.c */
+extern void __exit fscrypt_essiv_cleanup(void);
+
 #endif /* _FSCRYPT_PRIVATE_H */
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 179e578b875b..018c588c7ac3 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,8 +10,13 @@
 
 #include <keys/user-type.h>
 #include <linux/scatterlist.h>
+#include <linux/ratelimit.h>
+#include <crypto/aes.h>
+#include <crypto/sha.h>
 #include "fscrypt_private.h"
 
+static struct crypto_shash *essiv_hash_tfm;
+
 static void derive_crypt_complete(struct crypto_async_request *req, int rc)
 {
 	struct fscrypt_completion_result *ecr = req->data;
@@ -27,13 +32,13 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc)
  * derive_key_aes() - Derive a key using AES-128-ECB
  * @deriving_key: Encryption key used for derivation.
  * @source_key:   Source key to which to apply derivation.
- * @derived_key:  Derived key.
+ * @derived_raw_key:  Derived raw key.
  *
  * Return: Zero on success; non-zero otherwise.
  */
 static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
-				u8 source_key[FS_AES_256_XTS_KEY_SIZE],
-				u8 derived_key[FS_AES_256_XTS_KEY_SIZE])
+				const struct fscrypt_key *source_key,
+				u8 derived_raw_key[FS_MAX_KEY_SIZE])
 {
 	int res = 0;
 	struct skcipher_request *req = NULL;
@@ -60,10 +65,10 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 	if (res < 0)
 		goto out;
 
-	sg_init_one(&src_sg, source_key, FS_AES_256_XTS_KEY_SIZE);
-	sg_init_one(&dst_sg, derived_key, FS_AES_256_XTS_KEY_SIZE);
-	skcipher_request_set_crypt(req, &src_sg, &dst_sg,
-					FS_AES_256_XTS_KEY_SIZE, NULL);
+	sg_init_one(&src_sg, source_key->raw, source_key->size);
+	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
+	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
+				   NULL);
 	res = crypto_skcipher_encrypt(req);
 	if (res == -EINPROGRESS || res == -EBUSY) {
 		wait_for_completion(&ecr.completion);
@@ -77,7 +82,7 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 
 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) {
 		printk_once(KERN_WARNING
 				"%s: key size incorrect: %d\n",
 				__func__, master_key->size);
 		res = -ENOKEY;
 		goto out;
 	}
-	res = derive_key_aes(ctx->nonce, master_key->raw, raw_key);
+	res = derive_key_aes(ctx->nonce, master_key, raw_key);
 out:
 	up_read(&keyring_key->sem);
 	key_put(keyring_key);
 	return res;
 }
 
+static const struct {
+	const char *cipher_str;
+	int keysize;
+} available_modes[] = {
+	[FS_ENCRYPTION_MODE_AES_256_XTS] = { "xts(aes)",
+					     FS_AES_256_XTS_KEY_SIZE },
+	[FS_ENCRYPTION_MODE_AES_256_CTS] = { "cts(cbc(aes))",
+					     FS_AES_256_CTS_KEY_SIZE },
+	[FS_ENCRYPTION_MODE_AES_128_CBC] = { "cbc(aes)",
+					     FS_AES_128_CBC_KEY_SIZE },
+	[FS_ENCRYPTION_MODE_AES_128_CTS] = { "cts(cbc(aes))",
+					     FS_AES_128_CTS_KEY_SIZE },
+};
+
 static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
 				 const char **cipher_str_ret, int *keysize_ret)
 {
-	if (S_ISREG(inode->i_mode)) {
-		if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
-			*cipher_str_ret = "xts(aes)";
-			*keysize_ret = FS_AES_256_XTS_KEY_SIZE;
-			return 0;
-		}
-		pr_warn_once("fscrypto: unsupported contents encryption mode "
-			     "%d for inode %lu\n",
-			     ci->ci_data_mode, inode->i_ino);
-		return -ENOKEY;
+	u32 mode;
+
+	if (!fscrypt_valid_enc_modes(ci->ci_data_mode, ci->ci_filename_mode)) {
+		pr_warn_ratelimited("fscrypt: inode %lu uses unsupported encryption modes (contents mode %d, filenames mode %d)\n",
+				    inode->i_ino,
+				    ci->ci_data_mode, ci->ci_filename_mode);
+		return -EINVAL;
 	}
 
-	if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
-		if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
-			*cipher_str_ret = "cts(cbc(aes))";
-			*keysize_ret = FS_AES_256_CTS_KEY_SIZE;
-			return 0;
-		}
-		pr_warn_once("fscrypto: unsupported filenames encryption mode "
-			     "%d for inode %lu\n",
-			     ci->ci_filename_mode, inode->i_ino);
-		return -ENOKEY;
+	if (S_ISREG(inode->i_mode)) {
+		mode = ci->ci_data_mode;
+	} else if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
+		mode = ci->ci_filename_mode;
+	} else {
+		WARN_ONCE(1, "fscrypt: filesystem tried to load encryption info for inode %lu, which is not encryptable (file type %d)\n",
+			  inode->i_ino, (inode->i_mode & S_IFMT));
+		return -EINVAL;
 	}
 
-	pr_warn_once("fscrypto: unsupported file type %d for inode %lu\n",
-		     (inode->i_mode & S_IFMT), inode->i_ino);
-	return -ENOKEY;
+	*cipher_str_ret = available_modes[mode].cipher_str;
+	*keysize_ret = available_modes[mode].keysize;
+	return 0;
 }
 
 static void put_crypt_info(struct fscrypt_info *ci)
@@ -163,9 +178,76 @@ static void put_crypt_info(struct fscrypt_info *ci)
 		return;
 
 	crypto_free_skcipher(ci->ci_ctfm);
+	crypto_free_cipher(ci->ci_essiv_tfm);
 	kmem_cache_free(fscrypt_info_cachep, ci);
 }
 
+static int derive_essiv_salt(const u8 *key, int keysize, u8 *salt)
+{
+	struct crypto_shash *tfm = READ_ONCE(essiv_hash_tfm);
+
+	/* init hash transform on demand */
+	if (unlikely(!tfm)) {
+		struct crypto_shash *prev_tfm;
+
+		tfm = crypto_alloc_shash("sha256", 0, 0);
+		if (IS_ERR(tfm)) {
+			pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld\n",
+					    PTR_ERR(tfm));
+			return PTR_ERR(tfm);
+		}
+		prev_tfm = cmpxchg(&essiv_hash_tfm, NULL, tfm);
+		if (prev_tfm) {
+			crypto_free_shash(tfm);
+			tfm = prev_tfm;
+		}
+	}
+
+	{
+		SHASH_DESC_ON_STACK(desc, tfm);
+		desc->tfm = tfm;
+		desc->flags = 0;
+
+		return crypto_shash_digest(desc, key, keysize, salt);
+	}
+}
+
+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];
+
+	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
+	if (IS_ERR(essiv_tfm))
+		return PTR_ERR(essiv_tfm);
+
+	ci->ci_essiv_tfm = essiv_tfm;
+
+	err = derive_essiv_salt(raw_key, keysize, salt);
+	if (err)
+		goto out;
+
+	/*
+	 * Using SHA256 to derive the salt/key will result in AES-256 being
+	 * used for IV generation. File contents encryption will still use the
+	 * configured keysize (AES-128) nevertheless.
+	 */
+	err = crypto_cipher_setkey(essiv_tfm, salt, sizeof(salt));
+	if (err)
+		goto out;
+
+out:
+	memzero_explicit(salt, sizeof(salt));
+	return err;
+}
+
+void __exit fscrypt_essiv_cleanup(void)
+{
+	crypto_free_shash(essiv_hash_tfm);
+}
+
 int fscrypt_get_encryption_info(struct inode *inode)
 {
 	struct fscrypt_info *crypt_info;
@@ -212,6 +294,7 @@ 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_ctfm = NULL;
+	crypt_info->ci_essiv_tfm = NULL;
 	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
 				sizeof(crypt_info->ci_master_key));
 
@@ -228,10 +311,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (!raw_key)
 		goto out;
 
-	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX);
+	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
+				keysize);
 	if (res && inode->i_sb->s_cop->key_prefix) {
 		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
-					     inode->i_sb->s_cop->key_prefix);
+					     inode->i_sb->s_cop->key_prefix,
+					     keysize);
 		if (res2) {
 			if (res2 == -ENOKEY)
 				res = -ENOKEY;
@@ -243,18 +328,30 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	ctfm = crypto_alloc_skcipher(cipher_str, 0, 0);
 	if (!ctfm || IS_ERR(ctfm)) {
 		res = ctfm ? PTR_ERR(ctfm) : -ENOMEM;
-		printk(KERN_DEBUG
-		       "%s: error %d (inode %u) allocating crypto tfm\n",
-		       __func__, res, (unsigned) inode->i_ino);
+		pr_debug("%s: error %d (inode %lu) allocating crypto tfm\n",
+			 __func__, res, inode->i_ino);
 		goto out;
 	}
 	crypt_info->ci_ctfm = ctfm;
 	crypto_skcipher_clear_flags(ctfm, ~0);
 	crypto_skcipher_set_flags(ctfm, CRYPTO_TFM_REQ_WEAK_KEY);
+	/*
+	 * if the provided key is longer than keysize, we use the first
+	 * keysize bytes of the derived key only
+	 */
 	res = crypto_skcipher_setkey(ctfm, raw_key, keysize);
 	if (res)
 		goto out;
 
+	if (S_ISREG(inode->i_mode) &&
+	    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;
+		}
+	}
 	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) == NULL)
 		crypt_info = NULL;
 out:
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 210976e7a269..9914d51dff86 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -38,12 +38,8 @@ static int create_encryption_context_from_policy(struct inode *inode,
 	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
 					FS_KEY_DESCRIPTOR_SIZE);
 
-	if (!fscrypt_valid_contents_enc_mode(
-				policy->contents_encryption_mode))
-		return -EINVAL;
-
-	if (!fscrypt_valid_filenames_enc_mode(
-				policy->filenames_encryption_mode))
+	if (!fscrypt_valid_enc_modes(policy->contents_encryption_mode,
+				     policy->filenames_encryption_mode))
 		return -EINVAL;
 
 	if (policy->flags & ~FS_POLICY_FLAGS_VALID)
diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt_common.h
index 0a30c106c1e5..4022c61f7e9b 100644
--- a/include/linux/fscrypt_common.h
+++ b/include/linux/fscrypt_common.h
@@ -91,14 +91,18 @@ static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
 	return false;
 }
 
-static inline bool fscrypt_valid_contents_enc_mode(u32 mode)
+static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
+					u32 filenames_mode)
 {
-	return (mode == FS_ENCRYPTION_MODE_AES_256_XTS);
-}
+	if (contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
+	    filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS)
+		return true;
 
-static inline bool fscrypt_valid_filenames_enc_mode(u32 mode)
-{
-	return (mode == FS_ENCRYPTION_MODE_AES_256_CTS);
+	if (contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
+	    filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS)
+		return true;
+
+	return false;
 }
 
 static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 24e61a54feaa..a2a3ffb06038 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -272,6 +272,8 @@ struct fsxattr {
 #define FS_ENCRYPTION_MODE_AES_256_GCM		2
 #define FS_ENCRYPTION_MODE_AES_256_CBC		3
 #define FS_ENCRYPTION_MODE_AES_256_CTS		4
+#define FS_ENCRYPTION_MODE_AES_128_CBC		5
+#define FS_ENCRYPTION_MODE_AES_128_CTS		6
 
 struct fscrypt_policy {
 	__u8 version;
-- 
2.12.3

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

[Theodore Ts'o]

On Mon, Jun 19, 2017 at 09:27:58AM +0200, David Gstir wrote:
> From: Daniel Walter <dwalter@sigma-star.at>
> 
> 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 <dwalter@sigma-star.at>
> [david@sigma-star.at: addressed review comments]
> Signed-off-by: David Gstir <david@sigma-star.at>
> Reviewed-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted