[PATCH v2 0/4] Prevent out-of-bound reads

[PATCH v2 0/4] Prevent out-of-bound reads

[Lidong Chen]

v2:
- patch 4/4: Only mark the informative errors for translation based on the comments.
- patch 2/4: Include a reference to the fix in the patch 2/4 commit message.
- No changes to the rest of 2 patches.

Lidong Chen (4):
  fs/hfsplus: Validate btree node size
  fs/hfsplus: Prevent out of bound access in catalog file
  fs/hfsplus: Set grub errno to prevent NULL pointer access
  fs/hfsplus: Mark error strings for translation

 grub-core/fs/hfsplus.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

-- 
2.39.1

[PATCH v2 1/4] fs/hfsplus: Validate btree node size

[Lidong Chen]

The invalid btree node size can cause crashes when parsing
the btree. The fix is to ensure the btree node size is within
the valid range defined in the HFS Plus techical note, TN1150.

https://developer.apple.com/library/archive/technotes/tn/tn1150.html

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
 grub-core/fs/hfsplus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 6337cbfcb..1ffebc8be 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -84,6 +84,9 @@ struct grub_hfsplus_catfile
 #define GRUB_HFSPLUS_FILEMODE_DIRECTORY	0040000
 #define GRUB_HFSPLUS_FILEMODE_SYMLINK	0120000
 
+#define HFSPLUS_BTNODE_MINSZ	(1 << 9)
+#define HFSPLUS_BTNODE_MAXSZ	(1 << 15)
+
 /* Some pre-defined file IDs.  */
 enum
   {
@@ -584,6 +587,10 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
       return 0;
     }
 
+  if (btree->nodesize < HFSPLUS_BTNODE_MINSZ ||
+      btree->nodesize > HFSPLUS_BTNODE_MAXSZ)
+    return grub_error (GRUB_ERR_BAD_FS, "invalid HFS+ btree node size");
+
   node = grub_malloc (btree->nodesize);
   if (! node)
     return grub_errno;
-- 
2.39.1

[PATCH v2 2/4] fs/hfsplus: Prevent out of bound access in catalog file

[Lidong Chen]

A corrupted hfsplus can have a catalog key that is out of range.
This can lead to out of bound access when advancing the pointer to
access catalog file info. The valid range of a catalog key is specified
in HFS Plus Technical Note TN1150.

https://developer.apple.com/library/archive/technotes/tn/tn1150.html

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
 grub-core/fs/hfsplus.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 1ffebc8be..9c1f12574 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -87,6 +87,9 @@ struct grub_hfsplus_catfile
 #define HFSPLUS_BTNODE_MINSZ	(1 << 9)
 #define HFSPLUS_BTNODE_MAXSZ	(1 << 15)
 
+#define HFSPLUS_CATKEY_MIN_LEN	6
+#define HFSPLUS_CATKEY_MAX_LEN	516
+
 /* Some pre-defined file IDs.  */
 enum
   {
@@ -699,6 +702,13 @@ list_nodes (void *record, void *hook_arg)
 
   catkey = (struct grub_hfsplus_catkey *) record;
 
+  if (grub_be_to_cpu16 (catkey->keylen) < HFSPLUS_CATKEY_MIN_LEN ||
+      grub_be_to_cpu16 (catkey->keylen) > HFSPLUS_CATKEY_MAX_LEN)
+    {
+      grub_error (GRUB_ERR_BAD_FS, "catalog key length is out of range");
+      return 1;
+    }
+
   fileinfo =
     (struct grub_hfsplus_catfile *) ((char *) record
 				     + grub_be_to_cpu16 (catkey->keylen)
-- 
2.39.1

[PATCH v2 3/4] fs/hfsplus: Set grub errno to prevent NULL pointer access

[Lidong Chen]

When an invalid node size is detected in grub_hfsplus_mount(), data pinter
is freed. Thus, file->data is not set. The code should also set the
grub error when that happens to indicate an error and to avoid accessing
the unintialized file->data in grub_file_close().

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
 grub-core/fs/hfsplus.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 9c1f12574..cf13e8a63 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -357,7 +357,10 @@ grub_hfsplus_mount (grub_disk_t disk)
 			  (header.key_compare == GRUB_HFSPLUSX_BINARYCOMPARE));
 
   if (data->catalog_tree.nodesize < 2)
-    goto fail;
+    {
+      grub_error (GRUB_ERR_BAD_FS, "invalid catalog node size");
+      goto fail;
+    }
 
   if (grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, 0,
 			      sizeof (struct grub_hfsplus_btnode),
@@ -374,7 +377,10 @@ grub_hfsplus_mount (grub_disk_t disk)
   data->extoverflow_tree.nodesize = grub_be_to_cpu16 (header.nodesize);
 
   if (data->extoverflow_tree.nodesize < 2)
-    goto fail;
+    {
+      grub_error (GRUB_ERR_BAD_FS, "invalid extents overflow node size");
+      goto fail;
+    }
 
   data->extoverflow_tree_ready = 1;
 
-- 
2.39.1

[PATCH v2 4/4] fs/hfsplus: Mark error strings for translation

[Lidong Chen]

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
 grub-core/fs/hfsplus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index cf13e8a63..793cbdba2 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -277,7 +277,7 @@ grub_hfsplus_mount (grub_disk_t disk)
       /* See if there's an embedded HFS+ filesystem.  */
       if (grub_be_to_cpu16 (volheader.hfs.embed_sig) != GRUB_HFSPLUS_MAGIC)
 	{
-	  grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
+	  grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
 	  goto fail;
 	}
 
@@ -303,7 +303,7 @@ grub_hfsplus_mount (grub_disk_t disk)
       || ((volheader.hfsplus.blksize & (volheader.hfsplus.blksize - 1)) != 0)
       || grub_be_to_cpu32 (volheader.hfsplus.blksize) < GRUB_DISK_SECTOR_SIZE)
     {
-      grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
+      grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
       goto fail;
     }
 
@@ -406,7 +406,7 @@ grub_hfsplus_mount (grub_disk_t disk)
  fail:
 
   if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
-    grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
+    grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
 
   grub_free (data);
   return 0;
-- 
2.39.1

Re: [PATCH v2 0/4] Prevent out-of-bound reads

[Daniel Kiper]

On Wed, May 03, 2023 at 05:32:16PM +0000, Lidong Chen wrote:
> v2:
> - patch 4/4: Only mark the informative errors for translation based on the comments.
> - patch 2/4: Include a reference to the fix in the patch 2/4 commit message.
> - No changes to the rest of 2 patches.
>
> Lidong Chen (4):
>   fs/hfsplus: Validate btree node size
>   fs/hfsplus: Prevent out of bound access in catalog file
>   fs/hfsplus: Set grub errno to prevent NULL pointer access
>   fs/hfsplus: Mark error strings for translation
>
>  grub-core/fs/hfsplus.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)

For all patches except #4 Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Vladimir convinced me that the last patch does not make a lot of sense...

Lidong, thank you for fixing these issues!

Daniel