eCryptfs: Check array bounds for filename characters

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

fs/ecryptfs/crypto.c +30 -9

index 58609bd..d2d2b9e 100644
@@ -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,

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.

Quoted 5 lines · 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.

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'.