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

[David Gstir]

[resend without the HTML crap - sorry about that!]

Hi Eric!

Thanks for the thorough review! :)

> On 17 May 2017, at 20:08, Eric Biggers <ebiggers3@xxxxxxxxx> wrote:
> 
> Hi David, thanks for the update!
> 
> On Wed, May 17, 2017 at 01:21:04PM +0200, David Gstir wrote:
>> From: Daniel Walter <dwalter@xxxxxxxxxxxxx>
>> 
>> 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