Re: [PATCH] eCryptfs: ensure copy to crypt_stat->cipher does not overrun

[Tyler Hicks]

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