[Patch] hfs: fix namelength memory corruption

[Patch] hfs: fix namelength memory corruption

[Eric Sesterhenn]

hi,

this is basically the same as
hfsplus-fix-buffer-overflow-with-a-corrupted-image.patch.
We use the length parameter for a memcopy without checking it and
thereby corruption memory. 

Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>

--- linux/fs/hfs/catalog.c.orig	2008-09-08 15:20:15.000000000 +0200
+++ linux/fs/hfs/catalog.c	2008-09-08 15:21:02.000000000 +0200
@@ -190,6 +190,10 @@ int hfs_cat_find_brec(struct super_block
 
 	fd->search_key->cat.ParID = rec.thread.ParID;
 	len = fd->search_key->cat.CName.len = rec.thread.CName.len;
+	if (len > HFS_NAMELEN) {
+		printk(KERN_ERR "hfs: bad catalog namelength\n");
+		return -EIO;
+	}
 	memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
 	return hfs_brec_find(fd);
 }

Re: [Patch] hfs: fix namelength memory corruption

[Andrew Morton]

On Mon, 8 Sep 2008 15:35:05 +0200
Eric Sesterhenn <snakebyte@gmx.de> wrote:

> hi,
> 
> this is basically the same as
> hfsplus-fix-buffer-overflow-with-a-corrupted-image.patch.

I can't really use the above text in a changelog.  Think how it will
look in git in two years time.

> We use the length parameter for a memcopy without checking it and
> thereby corruption memory. 

"corrupting".

I assume that this bug was found using a deliberately corrupted
filesystem?  If so, that sort of thing should be described in the
changelog.

Please spend a little more time (say, 60 seconds) preparing patch
descriptions.


> Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> 
> --- linux/fs/hfs/catalog.c.orig	2008-09-08 15:20:15.000000000 +0200
> +++ linux/fs/hfs/catalog.c	2008-09-08 15:21:02.000000000 +0200
> @@ -190,6 +190,10 @@ int hfs_cat_find_brec(struct super_block
>  
>  	fd->search_key->cat.ParID = rec.thread.ParID;
>  	len = fd->search_key->cat.CName.len = rec.thread.CName.len;
> +	if (len > HFS_NAMELEN) {
> +		printk(KERN_ERR "hfs: bad catalog namelength\n");
> +		return -EIO;
> +	}
>  	memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
>  	return hfs_brec_find(fd);
>  }

Please send a full changelog for this patch.

I can (and often do) end up writing these things myself, but it's not a
very satisfactory arrangement, particularly when I'm not provided with
sufficient information to do so.

Thanks.

Re: [Patch] hfs: fix namelength memory corruption

[David Wagner]

Eric Sesterhenn  wrote:
>this is basically the same as
>hfsplus-fix-buffer-overflow-with-a-corrupted-image.patch.
>We use the length parameter for a memcopy without checking it and
>thereby corruption memory. 
>
>Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
>
>--- linux/fs/hfs/catalog.c.orig	2008-09-08 15:20:15.000000000 +0200
>+++ linux/fs/hfs/catalog.c	2008-09-08 15:21:02.000000000 +0200
>@@ -190,6 +190,10 @@ int hfs_cat_find_brec(struct super_block
> 
> 	fd->search_key->cat.ParID = rec.thread.ParID;
> 	len = fd->search_key->cat.CName.len = rec.thread.CName.len;
>+	if (len > HFS_NAMELEN) {
>+		printk(KERN_ERR "hfs: bad catalog namelength\n");
>+		return -EIO;
>+	}
> 	memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
> 	return hfs_brec_find(fd);
> }

Ahh.  Took me a second to see that this is saved from
signed/unsigned attacks by the fact that CName.len is u8.
In other words:
len is declared as an "int" (i.e., signed).
memcpy's third parameter is declared as size_t (i.e., unsigned).
Fortunately, len can't be negative, because CName.len is u8.
OK, got it.

I took a glance at some of the rest of fs/hfs/, to see whether
there are any other issues in there, and I couldn't tell whether
it's all OK.  e.g.,

  * hfs_asc2mac() doesn't seem to consistently check for buffer
    overflow.  If nls_io is non-NULL and nls_disk is false, what
    prevents this from writing beyond the end of dst?  Did I overlook
    something?

  * hfs_asc2mac() declares srclen, dstlen as ints, and compares
    them without checking for wraparound ("dstlen > srclen"): what
    if srclen is negative?  Couldn't tell if there's anything to
    worry about here.

  * hfs_cat_build_key doesn't check for integer overflow/wraparound
    ("6 + key->cat.CName.len"); is that safe?  I didn't check.

  * hfs_compare_dentry() declares len as an int.  This is harmless
    on typical compilers, but in principle could invoke undefined
    behavior.  What if s1->len == s2->len and both are > 2^31?  Then
    len is negative, bypassing the length check ("len >= HFS_NAMELEN"),
    and allowing the while() loop to cause integer underflow, which
    technically (according to the C spec) invokes undefined behavior.
    OK, on typical compilers I suspect this is harmless.

I might be talking nonsense.  Even if not, I suspect some or all of
these may actually be safe because of some facts about the callers,
but stylistically, whenever you are handling untrusted data, I wonder
if it would be safer to use code where it is locally obvious that the
code is free of buffer overruns and the like.

Would it be a good idea to review the rest of fs/hfs/ for other
potential signed/unsigned and integer overflow bugs as well?

Would the following practices be a good idea?

  * declare length values as size_t whereever possible.

  * write code where it's locally obvious that it is safe
    (minimize assumptions about how the rest of the code works,
    to reduce maintenance hazards, and make bugs more locally
    apparently by the fact that the code isn't obviously safe).