kern/52993: case sensitive HFS+ can return data from wrong file

["Harold Gutch" <logix%foobar.franken.de@localhost>]

>Number:         52993
>Category:       kern
>Synopsis:       case sensitive HFS+ can return data from wrong file
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Feb 08 11:10:00 +0000 2018
>Originator:     Harold Gutch
>Release:        NetBSD 7.1 (but the bug exists from 5.0 to HEAD)
>Organization:
>Environment:
NetBSD  7.1 NetBSD 7.1 (GENERIC.201703111743Z) amd64
>Description:
On a case sensitive HFS+ file system, when locating a file of a given name, file names are compared against it as memcmp(name1, name2, min(len(name1), len(name2))).  Thus, if name1 starts with name2 (or vice versa) they are understood to be identical and as a result when accessing name1 we might end up reading from name2 instead.

The issue does not occur for case insensitive HFS+ file systems (the default).
>How-To-Repeat:
1) On Mac OS, create a case sensitive HFS+ file system, create two files one's name starts with the other's, and fill them with different data.

osx:tmp hg$ hdiutil create test.dmg -size 1m -fs HFS+X -volname "test"
...............................................................................
created: /Users/hg/tmp/test.dmg
osx:tmp hg$ hdiutil mount test.dmg
/dev/disk7              GUID_partition_scheme
/dev/disk7s1            Apple_HFS                       /Volumes/test
osx:tmp hg$ echo foo > /Volumes/test/test
osx:tmp hg$ echo bar > /Volumes/test/test123
osx:tmp hg$ cat /Volumes/test/test
foo
osx:tmp hg$ cat /Volumes/test/test123
bar


2) After umounting, copy the .dmg over to NetBSD.  There mount and check the inode numbers and the contents of the files.

ichiban# vndconfig vnd0 test.dmg
ichiban# gpt show vnd0 | grep HFS
     40   1968      1  GPT part - Apple HFS
ichiban# dkctl vnd0 addwedge test 40 1968 hfs
dk0 created successfully.
ichiban# mount -t hfs -o ro /dev/dk0 /mnt
ichiban# cat /mnt/test
foo
ichiban# cat /mnt/test123
foo
ichiban# ls -li /mnt/test123 /mnt/test
22 -rw-r--r--  1 503  staff  4 Feb  6 14:47 /mnt/test
22 -rw-r--r--  1 503  staff  4 Feb  6 14:47 /mnt/test123


3) Notice that on NetBSD both, the contents of test and test123, and also their inode number are identical.
>Fix:
[ NOTE: the actual fix is in the few lines after "/* FIXME: ...", the rest is merely a bit of syntax sugar without functional change that just avoids the repeated casts to (const hfs_catalog_key_t*) all over the function and the same then again in hfslib_compare_extent_keys(). ]


--- src/sys/fs/hfs/libhfs.c.orig	2015-06-21 13:40:25.000000000 +0000
+++ src/sys/fs/hfs/libhfs.c	2018-02-06 11:51:04.000000000 +0000
@@ -2384,37 +2384,42 @@
 /* binary compare (i.e., not case folding) */
 int
 hfslib_compare_catalog_keys_bc (
-	const void *a,
-	const void *b)
+	const void *ap,
+	const void *bp)
 {
-	if (((const hfs_catalog_key_t*)a)->parent_cnid
-		== ((const hfs_catalog_key_t*)b)->parent_cnid)
+	int c;
+	const hfs_catalog_key_t *a, *b;
+
+	a = (const hfs_catalog_key_t *) ap;
+	b = (const hfs_catalog_key_t *) bp;
+
+	if (a->parent_cnid == b->parent_cnid)
 	{
-		if (((const hfs_catalog_key_t*)a)->name.length == 0 &&
-			((const hfs_catalog_key_t*)b)->name.length == 0)
+		if (a->name.length == 0 && b->name.length == 0)
 			return 0;
 
-		if (((const hfs_catalog_key_t*)a)->name.length == 0)
+		if (a->name.length == 0)
 			return -1;
-		if (((const hfs_catalog_key_t*)b)->name.length == 0)
+		if (b->name.length == 0)
 			return 1;
 
 		/* FIXME: This does a byte-per-byte comparison, whereas the HFS spec
 		 * mandates a uint16_t chunk comparison. */
-		return memcmp(((const hfs_catalog_key_t*)a)->name.unicode,
-			((const hfs_catalog_key_t*)b)->name.unicode,
-			min(((const hfs_catalog_key_t*)a)->name.length,
-				((const hfs_catalog_key_t*)b)->name.length));
+		c = memcmp(a->name.unicode, b->name.unicode,
+			sizeof(unichar_t)*min(a->name.length, b->name.length));
+		if (c != 0)
+			return c;
+		else
+			return (a->name.length - b->name.length);
 	} else {
-		return (((const hfs_catalog_key_t*)a)->parent_cnid - 
-				((const hfs_catalog_key_t*)b)->parent_cnid);
+		return (a->parent_cnid - b->parent_cnid);
 	}
 }
 
 int
 hfslib_compare_extent_keys (
-	const void *a,
-	const void *b)
+	const void *ap,
+	const void *bp)
 {
 	/*
 	 *	Comparison order, in descending importance:
@@ -2422,27 +2427,25 @@
 	 *		CNID -> fork type -> start block
 	 */
 
-	if (((const hfs_extent_key_t*)a)->file_cnid
-		== ((const hfs_extent_key_t*)b)->file_cnid)
+	const hfs_extent_key_t *a, *b;
+	a = (const hfs_extent_key_t *) ap;
+	b = (const hfs_extent_key_t *) bp;
+
+	if (a->file_cnid == b->file_cnid)
 	{
-		if (((const hfs_extent_key_t*)a)->fork_type
-			== ((const hfs_extent_key_t*)b)->fork_type)
+		if (a->fork_type == b->fork_type)
 		{
-			if (((const hfs_extent_key_t*)a)->start_block
-				== ((const hfs_extent_key_t*)b)->start_block)
+			if (a->start_block == b->start_block)
 			{
 				return 0;
 			} else {
-				return (((const hfs_extent_key_t*)a)->start_block - 
-						((const hfs_extent_key_t*)b)->start_block);
+				return (a->start_block - b->start_block);
 			}
 		} else {
-			return (((const hfs_extent_key_t*)a)->fork_type - 
-					((const hfs_extent_key_t*)b)->fork_type);
+			return (a->fork_type - b->fork_type);
 		}
 	} else {
-		return (((const hfs_extent_key_t*)a)->file_cnid - 
-				((const hfs_extent_key_t*)b)->file_cnid);
+		return (a->file_cnid - b->file_cnid);
 	}
 }