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

From: David Gstir <david@sigma-star.at>
To: Eric Biggers <ebiggers3@gmail.com>
Cc: Theodore Ts'o <tytso@mit.edu>, Jaegeuk Kim <jaegeuk@kernel.org>,
	Richard Weinberger <richard@sigma-star.at>,
	herbert@gondor.apana.org.au, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	Daniel Walter <dwalter@sigma-star.at>
Subject: Re: [PATCH v4] fscrypt: Add support for AES-128-CBC
Date: Wed, 31 May 2017 17:57:22 +0200	[thread overview]
Message-ID: <54DA453A-BC92-466E-B3D2-6C67D7D32A90@sigma-star.at> (raw)
In-Reply-To: <20170523190041.GA132828@gmail.com>

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