Hi Eric,
thanks for the feedback!
> On 31.03.2017, at 08:21, Eric Biggers <ebigge...@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 <dwal...@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
