[U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries

[U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries

[Stefan Brüns]

The following command triggers a segfault in search_dir:
./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
    ext4write host 0 0 /./foo 0x10'

The following command triggers a segfault in check_filename:
./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
    ext4write host 0 0 /. 0x10'

"." is the first entry in the directory, thus previous_dir is NULL. The
whole previous_dir block in search_dir seems to be a bad copy from
check_filename(...). As the changed data is not written to disk, the
statement is mostly harmless, save the possible NULL-ptr reference.

Typically a file is unlinked by extending the direntlen of the previous
entry. If the entry is the first entry in the directory block, it is
invalidated by setting inode=0.

The inode==0 case is hard to trigger without crafted filesystems. It only
hits if the first entry in a directory block is deleted and later a lookup
for the entry (by name) is done.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 57 ++++++++++++++++++---------------------------------
 fs/ext4/ext4_write.c  |  2 +-
 include/ext4fs.h      |  2 +-
 3 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index b00b84f..16c5f53 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -511,16 +511,14 @@ fail:
 static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 {
 	int status;
-	int inodeno;
+	int inodeno = 0;
 	int totalbytes;
 	int templength;
 	int direct_blk_idx;
 	long int blknr;
-	int found = 0;
 	char *ptr = NULL;
 	unsigned char *block_buffer = NULL;
 	struct ext2_dirent *dir = NULL;
-	struct ext2_dirent *previous_dir = NULL;
 	struct ext_filesystem *fs = get_fs();
 
 	/* read the block no allocated to a file */
@@ -530,7 +528,7 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 		if (blknr == 0)
 			goto fail;
 
-		/* read the blocks of parenet inode */
+		/* read the blocks of parent inode */
 		block_buffer = zalloc(fs->blksz);
 		if (!block_buffer)
 			goto fail;
@@ -550,15 +548,9 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 			 * space in the block that means
 			 * it is a last entry of directory entry
 			 */
-			if (strlen(dirname) == dir->namelen) {
+			if (dir->inode && (strlen(dirname) == dir->namelen)) {
 				if (strncmp(dirname, ptr + sizeof(struct ext2_dirent), dir->namelen) == 0) {
-					uint16_t new_len;
-					new_len = le16_to_cpu(previous_dir->direntlen);
-					new_len += le16_to_cpu(dir->direntlen);
-					previous_dir->direntlen = cpu_to_le16(new_len);
 					inodeno = le32_to_cpu(dir->inode);
-					dir->inode = 0;
-					found = 1;
 					break;
 				}
 			}
@@ -569,19 +561,15 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 			/* traversing the each directory entry */
 			templength = le16_to_cpu(dir->direntlen);
 			totalbytes = totalbytes + templength;
-			previous_dir = dir;
 			dir = (struct ext2_dirent *)((char *)dir + templength);
 			ptr = (char *)dir;
 		}
 
-		if (found == 1) {
-			free(block_buffer);
-			block_buffer = NULL;
-			return inodeno;
-		}
-
 		free(block_buffer);
 		block_buffer = NULL;
+
+		if (inodeno > 0)
+			return inodeno;
 	}
 
 fail:
@@ -757,15 +745,13 @@ fail:
 	return result_inode_no;
 }
 
-static int check_filename(char *filename, unsigned int blknr)
+static int unlink_filename(char *filename, unsigned int blknr)
 {
-	unsigned int first_block_no_of_root;
 	int totalbytes = 0;
 	int templength = 0;
 	int status, inodeno;
 	int found = 0;
 	char *root_first_block_buffer = NULL;
-	char *root_first_block_addr = NULL;
 	struct ext2_dirent *dir = NULL;
 	struct ext2_dirent *previous_dir = NULL;
 	char *ptr = NULL;
@@ -773,18 +759,15 @@ static int check_filename(char *filename, unsigned int blknr)
 	int ret = -1;
 
 	/* get the first block of root */
-	first_block_no_of_root = blknr;
 	root_first_block_buffer = zalloc(fs->blksz);
 	if (!root_first_block_buffer)
 		return -ENOMEM;
-	root_first_block_addr = root_first_block_buffer;
-	status = ext4fs_devread((lbaint_t)first_block_no_of_root *
-				fs->sect_perblk, 0,
+	status = ext4fs_devread((lbaint_t)blknr * fs->sect_perblk, 0,
 				fs->blksz, root_first_block_buffer);
 	if (status == 0)
 		goto fail;
 
-	if (ext4fs_log_journal(root_first_block_buffer, first_block_no_of_root))
+	if (ext4fs_log_journal(root_first_block_buffer, blknr))
 		goto fail;
 	dir = (struct ext2_dirent *)root_first_block_buffer;
 	ptr = (char *)dir;
@@ -796,19 +779,20 @@ static int check_filename(char *filename, unsigned int blknr)
 		 * is free availble space in the block that
 		 * means it is a last entry of directory entry
 		 */
-		if (strlen(filename) == dir->namelen) {
-			if (strncmp(filename, ptr + sizeof(struct ext2_dirent),
-				dir->namelen) == 0) {
+		if (dir->inode && (strncmp(ptr + sizeof(struct ext2_dirent),
+					   filename, dir->namelen) == 0)) {
+			printf("file found, deleting\n");
+			inodeno = le32_to_cpu(dir->inode);
+			if (previous_dir) {
 				uint16_t new_len;
-				printf("file found deleting\n");
 				new_len = le16_to_cpu(previous_dir->direntlen);
 				new_len += le16_to_cpu(dir->direntlen);
 				previous_dir->direntlen = cpu_to_le16(new_len);
-				inodeno = le32_to_cpu(dir->inode);
+			} else {
 				dir->inode = 0;
-				found = 1;
-				break;
 			}
+			found = 1;
+			break;
 		}
 
 		if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen))
@@ -824,8 +808,7 @@ static int check_filename(char *filename, unsigned int blknr)
 
 
 	if (found == 1) {
-		if (ext4fs_put_metadata(root_first_block_addr,
-					first_block_no_of_root))
+		if (ext4fs_put_metadata(root_first_block_buffer, blknr))
 			goto fail;
 		ret = inodeno;
 	}
@@ -835,7 +818,7 @@ fail:
 	return ret;
 }
 
-int ext4fs_filename_check(char *filename)
+int ext4fs_filename_unlink(char *filename)
 {
 	short direct_blk_idx = 0;
 	long int blknr = -1;
@@ -847,7 +830,7 @@ int ext4fs_filename_check(char *filename)
 		blknr = read_allocated_block(g_parent_inode, direct_blk_idx);
 		if (blknr == 0)
 			break;
-		inodeno = check_filename(filename, blknr);
+		inodeno = unlink_filename(filename, blknr);
 		if (inodeno != -1)
 			return inodeno;
 	}
diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index d03d692..f5811aa 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -865,7 +865,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 	if (ext4fs_iget(parent_inodeno, g_parent_inode))
 		goto fail;
 	/* check if the filename is already present in root */
-	existing_file_inodeno = ext4fs_filename_check(filename);
+	existing_file_inodeno = ext4fs_filename_unlink(filename);
 	if (existing_file_inodeno != -1) {
 		ret = ext4fs_delete_file(existing_file_inodeno);
 		fs->first_pass_bbmap = 0;
diff --git a/include/ext4fs.h b/include/ext4fs.h
index 13d2c56..e3f6216 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -124,7 +124,7 @@ extern int gindex;
 
 int ext4fs_init(void);
 void ext4fs_deinit(void);
-int ext4fs_filename_check(char *filename);
+int ext4fs_filename_unlink(char *filename);
 int ext4fs_write(const char *fname, unsigned char *buffer,
 		 unsigned long sizebytes);
 int ext4_write_file(const char *filename, void *buf, loff_t offset, loff_t len,
-- 
2.9.2

[U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries

[Tom Rini]

On Sun, Aug 14, 2016 at 05:11:04AM +0200, Stefan Br?ns wrote:

> The following command triggers a segfault in search_dir:
> ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
>     ext4write host 0 0 /./foo 0x10'
> 
> The following command triggers a segfault in check_filename:
> ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
>     ext4write host 0 0 /. 0x10'
> 
> "." is the first entry in the directory, thus previous_dir is NULL. The
> whole previous_dir block in search_dir seems to be a bad copy from
> check_filename(...). As the changed data is not written to disk, the
> statement is mostly harmless, save the possible NULL-ptr reference.
> 
> Typically a file is unlinked by extending the direntlen of the previous
> entry. If the entry is the first entry in the directory block, it is
> invalidated by setting inode=0.
> 
> The inode==0 case is hard to trigger without crafted filesystems. It only
> hits if the first entry in a directory block is deleted and later a lookup
> for the entry (by name) is done.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 57 ++++++++++++++++++---------------------------------
>  fs/ext4/ext4_write.c  |  2 +-
>  include/ext4fs.h      |  2 +-
>  3 files changed, 22 insertions(+), 39 deletions(-)

Can you please add the test case to the existing scripts?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160819/ef0bdc60/attachment.sig>

[U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries

[Stefan Bruens]

On Freitag, 19. August 2016 15:54:51 CEST you wrote:
> On Sun, Aug 14, 2016 at 05:11:04AM +0200, Stefan Br?ns wrote:
> > The following command triggers a segfault in search_dir:
> > ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
> > 
> >     ext4write host 0 0 /./foo 0x10'
> > 
> > The following command triggers a segfault in check_filename:
> > ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
> > 
> >     ext4write host 0 0 /. 0x10'
> > 
> > "." is the first entry in the directory, thus previous_dir is NULL. The
> > whole previous_dir block in search_dir seems to be a bad copy from
> > check_filename(...). As the changed data is not written to disk, the
> > statement is mostly harmless, save the possible NULL-ptr reference.
> > 
> > Typically a file is unlinked by extending the direntlen of the previous
> > entry. If the entry is the first entry in the directory block, it is
> > invalidated by setting inode=0.
> > 
> > The inode==0 case is hard to trigger without crafted filesystems. It only
> > hits if the first entry in a directory block is deleted and later a lookup
> > for the entry (by name) is done.
> > 
> > Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> > ---
> > 
> >  fs/ext4/ext4_common.c | 57
> >  ++++++++++++++++++--------------------------------- fs/ext4/ext4_write.c
> >   |  2 +-
> >  include/ext4fs.h      |  2 +-
> >  3 files changed, 22 insertions(+), 39 deletions(-)
> 
> Can you please add the test case to the existing scripts?  Thanks!

Adding this to the current test script is somewhat problematic. The test runs 
all tests for fat and ext4, so each testcase should be file system agnostic. 
Unfortunately fat and ext4 (at least as implemented in U-Boot) have different 
semantics, as ext4 in U-Boot requires all path to absolute paths, whereas fat 
seems to require something else (relative path? absolute path, but without 
leading '/'?).

Calling 'fatwrite host 0 0 /. 0x10' happily creates a directory! called '/.', 
'fatwrite host 0 0 /./foo 0x10' creates a file and copletely messes up the 
filesystem (according to fsck.vfat and mounting the fs in linux).

Any advise?

Kind regards,

Stefan

-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries

[Tom Rini]

On Sun, Aug 28, 2016 at 09:44:41PM +0200, Stefan Bruens wrote:
> On Freitag, 19. August 2016 15:54:51 CEST you wrote:
> > On Sun, Aug 14, 2016 at 05:11:04AM +0200, Stefan Br?ns wrote:
> > > The following command triggers a segfault in search_dir:
> > > ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
> > > 
> > >     ext4write host 0 0 /./foo 0x10'
> > > 
> > > The following command triggers a segfault in check_filename:
> > > ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
> > > 
> > >     ext4write host 0 0 /. 0x10'
> > > 
> > > "." is the first entry in the directory, thus previous_dir is NULL. The
> > > whole previous_dir block in search_dir seems to be a bad copy from
> > > check_filename(...). As the changed data is not written to disk, the
> > > statement is mostly harmless, save the possible NULL-ptr reference.
> > > 
> > > Typically a file is unlinked by extending the direntlen of the previous
> > > entry. If the entry is the first entry in the directory block, it is
> > > invalidated by setting inode=0.
> > > 
> > > The inode==0 case is hard to trigger without crafted filesystems. It only
> > > hits if the first entry in a directory block is deleted and later a lookup
> > > for the entry (by name) is done.
> > > 
> > > Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> > > ---
> > > 
> > >  fs/ext4/ext4_common.c | 57
> > >  ++++++++++++++++++--------------------------------- fs/ext4/ext4_write.c
> > >   |  2 +-
> > >  include/ext4fs.h      |  2 +-
> > >  3 files changed, 22 insertions(+), 39 deletions(-)
> > 
> > Can you please add the test case to the existing scripts?  Thanks!
> 
> Adding this to the current test script is somewhat problematic. The test runs 
> all tests for fat and ext4, so each testcase should be file system agnostic. 
> Unfortunately fat and ext4 (at least as implemented in U-Boot) have different 
> semantics, as ext4 in U-Boot requires all path to absolute paths, whereas fat 
> seems to require something else (relative path? absolute path, but without 
> leading '/'?).
> 
> Calling 'fatwrite host 0 0 /. 0x10' happily creates a directory! called '/.', 
> 'fatwrite host 0 0 /./foo 0x10' creates a file and copletely messes up the 
> filesystem (according to fsck.vfat and mounting the fs in linux).
> 
> Any advise?

Can we fix this up in the argument parsing?  This sounds like it's
showing some bugs in the fatwrite parsing code itself.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160902/a460e4ad/attachment.sig>

[U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries

[Brüns, Stefan]

On Freitag, 2. September 2016 10:53:08 CEST you wrote:
> > 
> > Adding this to the current test script is somewhat problematic. The test
> > runs all tests for fat and ext4, so each testcase should be file system
> > agnostic. Unfortunately fat and ext4 (at least as implemented in U-Boot)
> > have different semantics, as ext4 in U-Boot requires all path to absolute
> > paths, whereas fat seems to require something else (relative path?
> > absolute path, but without leading '/'?).
> > 
> > Calling 'fatwrite host 0 0 /. 0x10' happily creates a directory! called
> > '/.', 'fatwrite host 0 0 /./foo 0x10' creates a file and copletely messes
> > up the filesystem (according to fsck.vfat and mounting the fs in linux).
> > 
> > Any advise?
> 
> Can we fix this up in the argument parsing?  This sounds like it's
> showing some bugs in the fatwrite parsing code itself.

The fatwrite code is hardly doing any parsing at all. It does not strip any 
"/" or "\" characters, does not interpret these as dir delimiters, and just 
pushes whatever it gets into the directory.

For the lookup, it uses a function which is quite similar to the fatload/fatls 
function, but still different. It only traverses the root directory.

The whole fatwrite seems to be a 50% almost verbatim copy of the read 
implementation and shares hardly any code. The problem is the "almost" copy, 
most functions have minor differences.

I think lots of code could be removed from fatwrite if the read implementation 
where better structured, but e.g. the main entry point is a huge function 
which, depending on some flags either prints the directory listing while 
walking/traversing the tree, returns the file size, loads a specified file 
into a buffer, or errors out in case some path element was not reachable.

So, currently there are two options for the bad fatwrite behaviour:
a) add even more duplicate code to fatwrite
b) restructure fatread to be reusable

Opinions, please!

Kind regards,

Stefan