On 2015-02-23 11:34:10, Colin King wrote:
> From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
>
> The patch 237fead61998: "[PATCH] ecryptfs: fs/Makefile and
> fs/Kconfig" from Oct 4, 2006, leads to the following static checker
> warning:
>
> fs/ecryptfs/crypto.c:846 ecryptfs_new_file_context()
> error: off-by-one overflow 'crypt_stat->cipher' size 32. rl = '0-32'
>
> There is a mismatch between the size of ecryptfs_crypt_stat.cipher
> and ecryptfs_mount_crypt_stat.global_default_cipher_name causing the
> copy of the cipher name to cause a off-by-one string copy error. This
> fix ensures the space reserved for this string is the same size including
> the trailing zero at the end throughout ecryptfs.
>
> This fix avoids increasing the size of ecryptfs_crypt_stat.cipher
> and also ecryptfs_parse_tag_70_packet_silly_stack.cipher_string and instead
> reduces the of ECRYPTFS_MAX_CIPHER_NAME_SIZE to 31 and includes the + 1 for
> the end of string terminator.
>
> Signed-off-by: Colin Ian King <colin.king@xxxxxxxxxxxxx>
Thanks for the patch, Colin! It looks to me like it is the correct
change.
I've added Dan to cc and will add a Reported-by patch tag to give him
proper credit.
However, I don't think that this overflow can be triggered. The
ecryptfs_mount_crypt_stat.global_default_cipher_name buffer is filled
from the value of the 'ecryptfs_cipher' mount argument. That mount
argument value is validated by the ecryptfs_code_for_cipher_string()
function. It requires that the string matches one of the strings in this
array:
static struct ecryptfs_cipher_code_str_map_elem
ecryptfs_cipher_code_str_map[] = {
{"aes",RFC2440_CIPHER_AES_128 },
{"blowfish", RFC2440_CIPHER_BLOWFISH},
{"des3_ede", RFC2440_CIPHER_DES3_EDE},
{"cast5", RFC2440_CIPHER_CAST_5},
{"twofish", RFC2440_CIPHER_TWOFISH},
{"cast6", RFC2440_CIPHER_CAST_6},
{"aes", RFC2440_CIPHER_AES_192},
{"aes", RFC2440_CIPHER_AES_256}
};
None of those cipher strings are long enough to overflow the 32 byte
ecryptfs_crypt_stat.cipher or
ecryptfs_parse_tag_70_packet_silly_stack.cipher_string buffers.
This is a valid cleanup that will protect us from accidentally
overflowing the buffer in the future so I'm going to go ahead and apply
it to my next branch. I'll add a comment making it clear that this is
hardening/cleanup and there is no actual overflow.
Tyler
> ---
> fs/ecryptfs/ecryptfs_kernel.h | 4 ++--
> fs/ecryptfs/keystore.c | 2 +-
> fs/ecryptfs/main.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index 90d1882..5ba029e 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -124,7 +124,7 @@ ecryptfs_get_key_payload_data(struct key *key)
> }
>
> #define ECRYPTFS_MAX_KEYSET_SIZE 1024
> -#define ECRYPTFS_MAX_CIPHER_NAME_SIZE 32
> +#define ECRYPTFS_MAX_CIPHER_NAME_SIZE 31
> #define ECRYPTFS_MAX_NUM_ENC_KEYS 64
> #define ECRYPTFS_MAX_IV_BYTES 16 /* 128 bits */
> #define ECRYPTFS_SALT_BYTES 2
> @@ -237,7 +237,7 @@ struct ecryptfs_crypt_stat {
> struct crypto_ablkcipher *tfm;
> struct crypto_hash *hash_tfm; /* Crypto context for generating
> * the initialization vectors */
> - unsigned char cipher[ECRYPTFS_MAX_CIPHER_NAME_SIZE];
> + unsigned char cipher[ECRYPTFS_MAX_CIPHER_NAME_SIZE + 1];
> unsigned char key[ECRYPTFS_MAX_KEY_BYTES];
> unsigned char root_iv[ECRYPTFS_MAX_IV_BYTES];
> struct list_head keysig_list;
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index 917bd5c..6bd67e2 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -891,7 +891,7 @@ struct ecryptfs_parse_tag_70_packet_silly_stack {
> struct blkcipher_desc desc;
> char fnek_sig_hex[ECRYPTFS_SIG_SIZE_HEX + 1];
> char iv[ECRYPTFS_MAX_IV_BYTES];
> - char cipher_string[ECRYPTFS_MAX_CIPHER_NAME_SIZE];
> + char cipher_string[ECRYPTFS_MAX_CIPHER_NAME_SIZE + 1];
> };
>
> /**
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index d9eb84b..76dfb01 100644
> --- a/fs/ecryptfs/main.c
> +++ b/fs/ecryptfs/main.c
> @@ -407,7 +407,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
> if (!cipher_name_set) {
> int cipher_name_len = strlen(ECRYPTFS_DEFAULT_CIPHER);
>
> - BUG_ON(cipher_name_len >= ECRYPTFS_MAX_CIPHER_NAME_SIZE);
> + BUG_ON(cipher_name_len > ECRYPTFS_MAX_CIPHER_NAME_SIZE);
> strcpy(mount_crypt_stat->global_default_cipher_name,
> ECRYPTFS_DEFAULT_CIPHER);
> }
> --
> 2.1.4
>
Attachment:
signature.asc
Description: Digital signature
