Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932263AbdCaGVz (ORCPT ); Fri, 31 Mar 2017 02:21:55 -0400 Received: from mail-pg0-f67.google.com ([74.125.83.67]:36244 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751058AbdCaGVx (ORCPT ); Fri, 31 Mar 2017 02:21:53 -0400 Date: Thu, 30 Mar 2017 23:21:49 -0700 From: Eric Biggers To: David Gstir Cc: tytso@mit.edu, jaegeuk@kernel.org, dwalter@sigma-star.at, richard@sigma-star.at, herbert@gondor.apana.org.au, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fscrypt@vger.kernel.org Subject: Re: [PATCH] fscrypt: Add support for AES-128-CBC Message-ID: <20170331062149.GA32409@zzz> References: <20170330173840.72909-1-david@sigma-star.at> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170330173840.72909-1-david@sigma-star.at> User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8208 Lines: 221 [+Cc linux-fscrypt] Hi David and Daniel, On Thu, Mar 30, 2017 at 07:38:40PM +0200, David Gstir wrote: > From: Daniel Walter > > 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. And I don't suppose AES-256-CBC is an option for you? Anyway, more comments below: > @@ -147,17 +148,31 @@ int fscrypt_do_page_crypto(const struct inode *inode, fscrypt_direction_t rw, > { > struct { > __le64 index; > - u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)]; > - } xts_tweak; > + u8 padding[FS_IV_SIZE - sizeof(__le64)]; > + } blk_desc; > struct skcipher_request *req = NULL; > DECLARE_FS_COMPLETION_RESULT(ecr); > struct scatterlist dst, src; > struct fscrypt_info *ci = inode->i_crypt_info; > struct crypto_skcipher *tfm = ci->ci_ctfm; > + u8 iv[FS_IV_SIZE]; > int res = 0; > > BUG_ON(len == 0); > > + BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE); > + BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE); > + blk_desc.index = cpu_to_le64(lblk_num); > + memset(blk_desc.padding, 0, sizeof(blk_desc.padding)); > + > + if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) { > + BUG_ON(ci->ci_essiv_tfm == NULL); > + memset(iv, 0, sizeof(iv)); > + crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, (u8 *)&blk_desc); > + } else { > + memcpy(iv, &blk_desc, sizeof(iv)); > + } > + The ESSIV cipher operation should be done in-place, so that only one IV buffer is needed. See what dm-crypt does in crypt_iv_essiv_gen(), for example. Also, it can just use ESSIV 'if (ci->ci_essiv_tfm != NULL)'. That would avoid the awkward BUG_ON() and hardcoding of a specific cipher mode. > @@ -27,13 +28,13 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc) > * derive_key_aes() - Derive a key using AES-128-ECB > * @deriving_key: Encryption key used for derivation. > * @source_key: Source key to which to apply derivation. > - * @derived_key: Derived key. > + * @derived_key_raw: Derived raw key. > * > * Return: Zero on success; non-zero otherwise. > */ > static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE], > - u8 source_key[FS_AES_256_XTS_KEY_SIZE], > - u8 derived_key[FS_AES_256_XTS_KEY_SIZE]) > + struct fscrypt_key *source_key, > + u8 derived_raw_key[FS_MAX_KEY_SIZE]) 'const struct fscrypt_key *'. > { > int res = 0; > struct skcipher_request *req = NULL; > @@ -60,10 +61,10 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE], > if (res < 0) > goto out; > > - sg_init_one(&src_sg, source_key, FS_AES_256_XTS_KEY_SIZE); > - sg_init_one(&dst_sg, derived_key, FS_AES_256_XTS_KEY_SIZE); > - skcipher_request_set_crypt(req, &src_sg, &dst_sg, > - FS_AES_256_XTS_KEY_SIZE, NULL); > + sg_init_one(&src_sg, source_key->raw, source_key->size); > + sg_init_one(&dst_sg, derived_raw_key, source_key->size); > + skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size, > + NULL); > res = crypto_skcipher_encrypt(req); > if (res == -EINPROGRESS || res == -EBUSY) { > wait_for_completion(&ecr.completion); > @@ -75,9 +76,28 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE], > return res; > } > > +static bool valid_key_size(struct fscrypt_info *ci, struct fscrypt_key *key, > + int reg_file) > +{ > + if (reg_file) { > + switch(ci->ci_data_mode) { > + case FS_ENCRYPTION_MODE_AES_256_XTS: > + return key->size >= FS_AES_256_XTS_KEY_SIZE; > + case FS_ENCRYPTION_MODE_AES_128_CBC: > + return key->size >= FS_AES_128_CBC_KEY_SIZE; > + } > + } else { > + if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) > + return key->size >= FS_AES_256_CTS_KEY_SIZE; > + if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_128_CTS) > + return key->size >= FS_AES_128_CTS_KEY_SIZE; > + } > + return false; > +} > + This is redundant with how the key size is determined in determine_cipher_type(). How about passing the expected key size to validate_user_key() (instead of 'reg_file') and verifying that key->size >= keysize? Also, it should be verified that key->size <= FS_MAX_KEY_SIZE (since that's how large the buffer in fscrypt_key is) and key->size % AES_BLOCK_SIZE == 0 (since derive_key_aes() assumes the key is evenly divisible into AES blocks). > @@ -134,6 +154,11 @@ static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode, > *keysize_ret = FS_AES_256_XTS_KEY_SIZE; > return 0; > } > + if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) { > + *cipher_str_ret = "cbc(aes)"; > + *keysize_ret = FS_AES_128_CBC_KEY_SIZE; > + return 0; > + } switch (ci->ci_data_mode) { ... } > + if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_128_CTS) { > + *cipher_str_ret = "cts(cbc(aes))"; > + *keysize_ret = FS_AES_128_CTS_KEY_SIZE; > + return 0; > + } switch (ci->ci_filename_mode) { ... } > + if (ci->ci_essiv_tfm) > + crypto_free_cipher(ci->ci_essiv_tfm); No need to check for NULL before calling crypto_free_cipher(). > + 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. 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. Also, the hash should be called through the crypto API. Also, please factor creating the essiv_tfm into its own function. fscrypt_get_encryption_info() is long enough already. Something else to consider (probably for the future; this doesn't necessarily have to be done yet) is that you really only need one essiv_tfm per *key*, not one per inode. To deduplicate them you'd need a hash table or LRU queue or something to keep track of the keys in use. - Eric