From: Colin Ian King <[email protected]>
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 <[email protected]>
---
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
On 2015-02-23 11:34:10, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
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
>