[resend without the HTML crap - sorry about that!]
Hi Eric!
Thanks for the thorough review! :)
> On 17 May 2017, at 20:08, Eric Biggers <ebigge...@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 <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. 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
