lkml.org 
[lkml]   [2017]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] fscrypt: Add support for AES-128-CBC
From
Date
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

\
 
 \ /
  Last update: 2017-03-31 13:13    [W:0.132 / U:0.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site