Re: [PATCH] eCryptfs: Check array bounds for filename characters

[Tyler Hicks <tyhicks@canonical.com>]

[-- Attachment #1: Type: text/plain, Size: 4318 bytes --]

On 2011-11-21 15:32:58, Michael Halcrow wrote:
> Characters with ASCII values greater than the size of
> filename_rev_map[] are valid filename
> characters. ecryptfs_decode_from_filename() will access kernel memory
> beyond that array, and ecryptfs_parse_tag_70_packet() will then
> decrypt those characters. The attacker, using the FNEK of the crafted
> file, can then re-encrypt the characters to reveal the kernel memory
> past the end of the filename_rev_map[] array. I expect low security
> impact since this array is statically allocated in the text area, and
> the amount of memory past the array that is accessible is limited by
> the largest possible ASCII filename character.
> 
> This change verifies that the offset into filename_rev_map[] is within
> bounds. If any one character is not, then eCryptfs will consider the
> filename to not be a valid encrypted filename and will copy it
> verbatim rather than try to decode/decrypt it.
> 
> Signed-off-by: Mike Halcrow <mhalcrow@google.com>

Linus - This is a pretty low security risk, but I figure that it is
probably best if you apply it directly to your tree. I don't think there
is much need for it to go through my tree. If you do take it in, feel
free to add the following tags:

Cc: <stable@kernel.org>
Acked-by: Tyler Hicks <tyhicks@canonical.com>

BTW, I reviewed and tested this patch before Mike sent it out to the
list.

Tyler

> 
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index 58609bd..d2d2b9e 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -2032,11 +2032,14 @@ out:
>   * @dst_size: Set to the size of the decoded string.
>   * @src: The encoded set of octets to decode.
>   * @src_size: The size of the encoded set of octets to decode.
> + *
> + * Returns zero on success; non-zero otherwise
>   */
> -static void
> +static int
>  ecryptfs_decode_from_filename(unsigned char *dst, size_t *dst_size,
>  			      const unsigned char *src, size_t src_size)
>  {
> +	int rc = 0;
>  	u8 current_bit_offset = 0;
>  	size_t src_byte_offset = 0;
>  	size_t dst_byte_offset = 0;
> @@ -2052,9 +2055,13 @@ ecryptfs_decode_from_filename(unsigned char *dst, size_t *dst_size,
>  		goto out;
>  	}
>  	while (src_byte_offset < src_size) {
> -		unsigned char src_byte =
> -				filename_rev_map[(int)src[src_byte_offset]];
> -
> +		int rev_map_offset = (int)src[src_byte_offset];
> +		unsigned char src_byte;
> +		if (rev_map_offset > (ARRAY_SIZE(filename_rev_map) - 1)) {
> +			rc = -EINVAL;
> +			goto out;
> +		}
> +		src_byte = filename_rev_map[rev_map_offset];
>  		switch (current_bit_offset) {
>  		case 0:
>  			dst[dst_byte_offset] = (src_byte << 2);
> @@ -2081,7 +2088,7 @@ ecryptfs_decode_from_filename(unsigned char *dst, size_t *dst_size,
>  	}
>  	(*dst_size) = dst_byte_offset;
>  out:
> -	return;
> +	return rc;
>  }
>  
>  /**
> @@ -2235,8 +2242,14 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name,
>  
>  		name += ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE;
>  		name_size -= ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE;
> -		ecryptfs_decode_from_filename(NULL, &decoded_name_size,
> -					      name, name_size);
> +		rc = ecryptfs_decode_from_filename(NULL, &decoded_name_size,
> +						   name, name_size);
> +		if (rc) {
> +			rc = ecryptfs_copy_filename(plaintext_name,
> +						    plaintext_name_size,
> +						    orig_name, orig_name_size);
> +			goto out;
> +		}
>  		decoded_name = kmalloc(decoded_name_size, GFP_KERNEL);
>  		if (!decoded_name) {
>  			printk(KERN_ERR "%s: Out of memory whilst attempting "
> @@ -2245,8 +2258,16 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name,
>  			rc = -ENOMEM;
>  			goto out;
>  		}
> -		ecryptfs_decode_from_filename(decoded_name, &decoded_name_size,
> -					      name, name_size);
> +		rc = ecryptfs_decode_from_filename(decoded_name,
> +						   &decoded_name_size,
> +						   name,
> +						   name_size);
> +		if (rc) {
> +			rc = ecryptfs_copy_filename(plaintext_name,
> +						    plaintext_name_size,
> +						    orig_name, orig_name_size);
> +			goto out_free;
> +		}
>  		rc = ecryptfs_parse_tag_70_packet(plaintext_name,
>  						  plaintext_name_size,
>  						  &packet_size,

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]