[PATCH] eCryptfs: Check array bounds for filename characters

[PATCH] eCryptfs: Check array bounds for filename characters

[Michael Halcrow]

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>


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,

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

[Tyler Hicks]

[-- 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 --]

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

[Linus Torvalds]

On Mon, Nov 21, 2011 at 3:32 PM, Michael Halcrow <mhalcrow@google.com> 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.

Ugh. I really don't like the patch.

Why isn't the patch just this one-liner:

  diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
  index 58609bde3b9f..7c50715c05d6 100644
  --- a/fs/ecryptfs/crypto.c
  +++ b/fs/ecryptfs/crypto.c
  @@ -1943,7 +1943,7 @@ static unsigned char *portable_filename_chars
= ("-.0123456789ABCD"

   /* We could either offset on every reverse map or just pad some 0x00's
    * at the front here */
  -static const unsigned char filename_rev_map[] = {
  +static const unsigned char filename_rev_map[256] = {
          0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 7 */
          0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 15 */
          0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 23 */

instead?

Making invalid characters over \x50 be somehow magically different
from invalid characters elsewhere seems just totally bogus. There are
lots of characters that aren't valid, and they have the
filename_rev_map[] value of 0 elsewhere.

So the simpler one-liner is not only simpler, but gives much saner
semantics, I think - now invalid character '\x05' gets exactly the
same result as invalid character '\xf5'.

Hmm?

                         Linus

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

[Tyler Hicks]

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

On 2011-11-21 16:49:07, Linus Torvalds wrote:
> On Mon, Nov 21, 2011 at 3:32 PM, Michael Halcrow <mhalcrow@google.com> 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.
> 
> Ugh. I really don't like the patch.
> 
> Why isn't the patch just this one-liner:
> 
>   diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
>   index 58609bde3b9f..7c50715c05d6 100644
>   --- a/fs/ecryptfs/crypto.c
>   +++ b/fs/ecryptfs/crypto.c
>   @@ -1943,7 +1943,7 @@ static unsigned char *portable_filename_chars
> = ("-.0123456789ABCD"
> 
>    /* We could either offset on every reverse map or just pad some 0x00's
>     * at the front here */
>   -static const unsigned char filename_rev_map[] = {
>   +static const unsigned char filename_rev_map[256] = {
>           0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 7 */
>           0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 15 */
>           0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 23 */
> 
> instead?
> 
> Making invalid characters over \x50 be somehow magically different
> from invalid characters elsewhere seems just totally bogus. There are
> lots of characters that aren't valid, and they have the
> filename_rev_map[] value of 0 elsewhere.
> 
> So the simpler one-liner is not only simpler, but gives much saner
> semantics, I think - now invalid character '\x05' gets exactly the
> same result as invalid character '\xf5'.

Good point - I'll get this in proper patch form and send a pull request
your way, along with a couple of other fixes.

Tyler

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