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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111121234649.GC9205@boyd>
Date:	Mon, 21 Nov 2011 17:46:49 -0600
From:	Tyler Hicks <tyhicks@...onical.com>
To:	torvalds@...ux-foundation.org,
	Michael Halcrow <mhalcrow@...gle.com>
Cc:	ecryptfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	thieule@...gle.com
Subject: Re: [PATCH] eCryptfs: Check array bounds for filename characters

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@...gle.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@...nel.org>
Acked-by: Tyler Hicks <tyhicks@...onical.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,

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)