[PATCH 0/4] fs/iso9660: Fix out-of-bounds read

[PATCH 0/4] fs/iso9660: Fix out-of-bounds read

[Lidong Chen]

This patches set fix a few out-of-bound reads and an infinite loop
in fs/iso9660. The main issues are that there is no validation for
the SUSP/RRIP entry size and no check for the boundary before read. 

Lidong Chen (4):
  fs/iso9660: Add check to prevent infinite loop
  fs/iso9660: Prevent read past the end of system use area
  fs/iso9660: Avoid reading past the entry boundary
  fs/iso9660: Incorrect check for entry boudary

 grub-core/fs/iso9660.c | 91 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 81 insertions(+), 10 deletions(-)

-- 
2.35.1

[PATCH 1/4] fs/iso9660: Add check to prevent infinite loop

[Lidong Chen]

There is no check for the end of block When reading
directory extents. It resulted in read_node() always
read from the same offset in the while loop, thus
caused infinite loop. The fix added a check for the
end of the block and ensure the read is within directory
boundary.

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

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 91817ec1f..4f4cd6165 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -795,6 +795,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
 	while (dirent.flags & FLAG_MORE_EXTENTS)
 	  {
 	    offset += dirent.len;
+
+	    /* offset should within the dir's len. */
+	    if (offset > len)
+	      {
+		if (ctx.filename_alloc)
+		  grub_free (ctx.filename);
+		return 0;
+	      }
+
 	    if (read_node (dir, offset, sizeof (dirent), (char *) &dirent))
 	      {
 		if (ctx.filename_alloc)
@@ -802,6 +811,18 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
 		grub_free (node);
 		return 0;
 	      }
+
+	    /*
+	     * It is either the end of block or zero-padded sector,
+	     * skip to the next block.
+	     */
+	    if (!dirent.len)
+	      {
+		offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
+		dirent.flags |= FLAG_MORE_EXTENTS;
+		continue;
+	      }
+
 	    if (node->have_dirents >= node->alloc_dirents)
 	      {
 		struct grub_fshelp_node *new_node;
-- 
2.35.1

Re: [PATCH 1/4] fs/iso9660: Add check to prevent infinite loop

[Thomas Schmitt]

Hi,

On Wed, 14 Dec 2022 18:55:02 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> There is no check for the end of block When reading

s/When/when/

> directory extents. It resulted in read_node() always
> read from the same offset in the while loop, thus
> caused infinite loop. The fix added a check for the
> end of the block and ensure the read is within directory
> boundary.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/iso9660.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 91817ec1f..4f4cd6165 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -795,6 +795,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>  	while (dirent.flags & FLAG_MORE_EXTENTS)
>  	  {
>  	    offset += dirent.len;
> +
> +	    /* offset should within the dir's len. */
> +	    if (offset > len)
> +	      {
> +		if (ctx.filename_alloc)
> +		  grub_free (ctx.filename);
> +		return 0;
> +	      }
> +
>  	    if (read_node (dir, offset, sizeof (dirent), (char *) &dirent))
>  	      {
>  		if (ctx.filename_alloc)
> @@ -802,6 +811,18 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>  		grub_free (node);
>  		return 0;
>  	      }
> +
> +	    /*
> +	     * It is either the end of block or zero-padded sector,
> +	     * skip to the next block.
> +	     */
> +	    if (!dirent.len)
> +	      {
> +		offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
> +		dirent.flags |= FLAG_MORE_EXTENTS;
> +		continue;
> +	      }
> +
>  	    if (node->have_dirents >= node->alloc_dirents)
>  	      {
>  		struct grub_fshelp_node *new_node;
> --
> 2.35.1
>

Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>

The second hunk will become very necessary when more initrds >= 4 GiB will
be around. Then GRUB might more probably encounter directory records of a
large file which are not stored in the same block.

(Are we aware of the file size limit of 32 GiB - 14 KiB - 1 imposed by
   struct grub_fshelp_node { ... struct grub_iso9660_dir dirents[8]; ... }
 ? )


Have a nice day :)

Thomas

Re: [PATCH 1/4] fs/iso9660: Add check to prevent infinite loop

[Lidong Chen]

> On Dec 15, 2022, at 9:52 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> On Wed, 14 Dec 2022 18:55:02 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
>> There is no check for the end of block When reading
> 
> s/When/when/
> 
>> directory extents. It resulted in read_node() always
>> read from the same offset in the while loop, thus
>> caused infinite loop. The fix added a check for the
>> end of the block and ensure the read is within directory
>> boundary.
>> 
>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>> ---
>> grub-core/fs/iso9660.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>> 
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index 91817ec1f..4f4cd6165 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -795,6 +795,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>> 	while (dirent.flags & FLAG_MORE_EXTENTS)
>> 	  {
>> 	    offset += dirent.len;
>> +
>> +	    /* offset should within the dir's len. */
>> +	    if (offset > len)
>> +	      {
>> +		if (ctx.filename_alloc)
>> +		  grub_free (ctx.filename);
>> +		return 0;
>> +	      }
>> +
>> 	    if (read_node (dir, offset, sizeof (dirent), (char *) &dirent))
>> 	      {
>> 		if (ctx.filename_alloc)
>> @@ -802,6 +811,18 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>> 		grub_free (node);
>> 		return 0;
>> 	      }
>> +
>> +	    /*
>> +	     * It is either the end of block or zero-padded sector,
>> +	     * skip to the next block.
>> +	     */
>> +	    if (!dirent.len)
>> +	      {
>> +		offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
>> +		dirent.flags |= FLAG_MORE_EXTENTS;
>> +		continue;
>> +	      }
>> +
>> 	    if (node->have_dirents >= node->alloc_dirents)
>> 	      {
>> 		struct grub_fshelp_node *new_node;
>> --
>> 2.35.1
>> 
> 
> Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
> 
> The second hunk will become very necessary when more initrds >= 4 GiB will
> be around. Then GRUB might more probably encounter directory records of a
> large file which are not stored in the same block.
> 
> (Are we aware of the file size limit of 32 GiB - 14 KiB - 1 imposed by
>   struct grub_fshelp_node { ... struct grub_iso9660_dir dirents[8]; ... }
> ? )
> 
I am not familiar with this file size limit. Do we need to add a check somewhere?

Thanks,
Lidong

> 
> Have a nice day :)
> 
> Thomas
> 

Re: [PATCH 1/4] fs/iso9660: Add check to prevent infinite loop

[Thomas Schmitt]

Hi,

i wrote:
> > (Are we aware of the file size limit of 32 GiB - 14 KiB - 1 imposed by
> >   struct grub_fshelp_node { ... struct grub_iso9660_dir dirents[8]; ... }
> > ? )

Lidong Chen wrote:
> I am not familiar with this file size limit. Do we need to add a check
> somewhere?

Good question. The answer probably disproves my statement because the
struct definition seems not to match exactly its usage:

Assessment happens in grub_iso9660_iterate_dir():

        while (dirent.flags & FLAG_MORE_EXTENTS)
          {
            ...
            if (node->have_dirents >= node->alloc_dirents)
              {

At this point an overflow of currently allocated .dirents[] was detected.

                struct grub_fshelp_node *new_node;
                grub_size_t sz;

                if (grub_mul (node->alloc_dirents, 2, &node->alloc_dirents) ||
                    grub_sub (node->alloc_dirents, ARRAY_SIZE (node->dirents), &sz) ||
                    grub_mul (sz, sizeof (node->dirents[0]), &sz) ||
                    grub_add (sz, sizeof (struct grub_fshelp_node), &sz))
                  goto fail_0;

                new_node = grub_realloc (node, sz);

I understand the computations in the if-clause as:
- The number of allocated dirents is doubled.
- The new_node size is the size of the new number of .dirents minus 8
  .dirent sizes for the eight .dirents which are part of the
  grub_fshelp_node definition,
- plus the defined size of the grub_fshelp_node.

The new_node gets allocated with that size, which provides enough space
for the new dirent and many of its potential successors.


So i retract my statement. Data file size seems quite unlimited.
At some point grub_mul() or grub_realloc() will throw an error if the number
of .dirents is too high for grub_size_t or the machine's memory.


Have a nice day :)

Thomas

[PATCH 2/4] fs/iso9660: Prevent read past the end of system use area

[Lidong Chen]

In the code, the for loop advanced the entry pointer to the
next entry before checking if the next entry is within the
system use area boundary. Another issue in the code was that
there is no check for the size of system use area. For a
corrupted system, the size of system use area can be less than
the size of SUSP entry size (5 bytes). These can cause buffer
overrun. The fixes added the checks to ensure the read is
valid and within the boundary.

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

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 4f4cd6165..9170fa820 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
 #define GRUB_ISO9660_VOLDESC_PART	3
 #define GRUB_ISO9660_VOLDESC_END	255
 
+#define GRUB_ISO9660_SUSP_HEADER_SZ	5
+
 /* The head of a volume descriptor.  */
 struct grub_iso9660_voldesc
 {
@@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
   if (sua_size <= 0)
     return GRUB_ERR_NONE;
 
+  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
+    return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
+
   sua = grub_malloc (sua_size);
   if (!sua)
     return grub_errno;
@@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
   if (err)
     return err;
 
-  for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry < (char *) sua + sua_size - 1 && entry->len > 0;
-       entry = (struct grub_iso9660_susp_entry *)
-	 ((char *) entry + entry->len))
+  entry = (struct grub_iso9660_susp_entry *) sua;
+
+  while (entry->len > 0)
     {
+      /* Ensure the entry is within System Use Area */
+      if ((char *) entry + entry->len > (sua + sua_size))
+        break;
+
       /* The last entry.  */
       if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
 	break;
@@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
 	  off = grub_le_to_cpu32 (ce->off);
 	  ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
 
+	  if (sua_size <= 0)
+	    break;
+
+	  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
+	    {
+	      grub_free (sua);
+	      return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");
+	    }
+
 	  grub_free (sua);
 	  sua = grub_malloc (sua_size);
 	  if (!sua)
@@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
 	  grub_free (sua);
 	  return 0;
 	}
+
+      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
+
+      if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
+        break;
     }
 
   grub_free (sua);
@@ -574,7 +597,7 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
 	  char *old;
 	  grub_size_t sz;
 
-	  csize = entry->len - 5;
+	  csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;
 	  old = ctx->filename;
 	  if (ctx->filename_alloc)
 	    {
-- 
2.35.1

Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area

[Thomas Schmitt]

Hi,

On Wed, 14 Dec 2022 18:55:03 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> In the code, the for loop advanced the entry pointer to the
> next entry before checking if the next entry is within the
> system use area boundary. Another issue in the code was that
> there is no check for the size of system use area. For a
> corrupted system, the size of system use area can be less than
> the size of SUSP entry size (5 bytes).

The minimum size of a SUSP entry is 4, not 5. Examples of 4-byte entries
are ST in SUSP and RE in RRIP. (Ending before ST would be harmless, because
ST marks the end of the SUSP entry chain. But RE may appear before the end.)


> These can cause buffer
> overrun. The fixes added the checks to ensure the read is
> valid and within the boundary.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/iso9660.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 4f4cd6165..9170fa820 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define GRUB_ISO9660_VOLDESC_PART	3
>  #define GRUB_ISO9660_VOLDESC_END	255
>
> +#define GRUB_ISO9660_SUSP_HEADER_SZ	5

So i think this should be 4, not 5.
If we find reason why it should be 5, then the name SUSP_HEADER_SZ is not
appropriate. The SUSP header size is surely 4.


> +
>  /* The head of a volume descriptor.  */
>  struct grub_iso9660_voldesc
>  {
> @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>    if (sua_size <= 0)
>      return GRUB_ERR_NONE;
>
> +  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> +    return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
> +

Here we need 4.


>    sua = grub_malloc (sua_size);
>    if (!sua)
>      return grub_errno;
> @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>    if (err)
>      return err;
>
> -  for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry < (char *) sua + sua_size - 1 && entry->len > 0;
> -       entry = (struct grub_iso9660_susp_entry *)
> -	 ((char *) entry + entry->len))
> +  entry = (struct grub_iso9660_susp_entry *) sua;
> +
> +  while (entry->len > 0)
>      {
> +      /* Ensure the entry is within System Use Area */
> +      if ((char *) entry + entry->len > (sua + sua_size))
> +        break;
> +
>        /* The last entry.  */
>        if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
>  	break;
> @@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>  	  off = grub_le_to_cpu32 (ce->off);
>  	  ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
>
> +	  if (sua_size <= 0)
> +	    break;
> +
> +	  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)

4 would be appropriate here.


> +	    {
> +	      grub_free (sua);
> +	      return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");
> +	    }
> +
>  	  grub_free (sua);
>  	  sua = grub_malloc (sua_size);
>  	  if (!sua)
> @@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>  	  grub_free (sua);
>  	  return 0;
>  	}
> +
> +      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);

This is equivalent to the third statement in the "for" which was
replaced by a while-loop. So far so good.

But i believe to see an old bug. This advancing of entry breaks the
chain of CE if the first entry of the Continuation Area is again a CE.

          err = grub_disk_read (node->data->disk, ce_block, off,
                                sua_size, sua);
          ...
          entry = (struct grub_iso9660_susp_entry *) sua;
        }

      if (hook (entry, hook_arg))
        {
          grub_free (sua);
          return 0;
        }

      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);

hook() will not interpret CE (and has no means to advance "entry").
entry = entry + entry->len; will then skip over the entry so that the loop
will not interpret it either.
The SUSP area will be perceived as having ended, although more SUSP entries
are present for the file in question.

I hereby ask for more opinions about this. Maybe i misinterpret the old loop.

Background:
CE points to the block and offset where the chain of SUSP entries goes on.
It is needed if the SUSP entry set exceeds the size limit of 255 bytes for
a directory record.
The byte at (ce->blk * block_size + ce->off) is the first byte of the next
SUSP entry in the chain.
Linux hates SUSP crossing block boundaries. So we ISO producers use further
CE entries to hop over block boundaries.

libisofs can produce a Continuation Area with first and only entry CE,
which then points to a new block with more SUSP payload. This happens if a
continuation block offers not much more than 28 free bytes, so that only
the 28 bytes of CE have room.


(GRUB is not really correct by performing the CE jump immediately when CE is
seen. The specs allow more SUSP entries to follow up to the end of the
directory record's SUSP range. Only after interpreting them, an encountered
CE should cause the jump to its Continuation Area. SUSP-1.12, 5.1:
  "The "CE" System Use Entry indicates a Continuation Area that shall be
   processed after the current System Use field or Continuation Area is
   processed."
mkisofs and libisofs both put their CE at the end of the directory record
of Continuation Area. So an exotic ISO would be needed to demonstrate a
problem with GRUB's immediate CE interpretation.)


> +
> +      if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
> +        break;

4 would be appropriate.


>      }
>
>    grub_free (sua);
> @@ -574,7 +597,7 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>  	  char *old;
>  	  grub_size_t sz;
>
> -	  csize = entry->len - 5;
> +	  csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;

Here it is not appropriate to use GRUB_ISO9660_SUSP_HEADER_SZ.
This code subtracts the count of the 4 header bytes of an NM entry and of
the 1 FLAGS byte of NM. Goal is to compute the size of the name string.
So if GRUB_ISO9660_SUSP_HEADER_SZ is defined as 4, then
(GRUB_ISO9660_SUSP_HEADER_SZ + 1) would be ok. But a plain 5 is ok, too,
because this is a specific property of the NM definition of version 1.

(Actually one should verify (entry->version == 1) before interpreting
an NM entry that way. Well, i see that libisofs doesn't check either.
Excuse is that the RRIP specs did not change since the mid 1990s.)

Whatever gets decided, the "5", which is six lines higher, should be changed
to the same expression:

      else if (entry->len >= 5)


>  	  old = ctx->filename;
>  	  if (ctx->filename_alloc)
>  	    {
> --
> 2.35.1
>

So no "Reviewed-by:" yet.

Open issues:

The definition of GRUB_ISO9660_SUSP_HEADER_SZ should be changed to 4.

Within the interpretation of NM this change should not happen:
> -       csize = entry->len - 5;
> +       csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;

We need to decide what to do with the potential old bug that a CE entry
as the first entry of a Continuation Area gets skipped without
interpretation.


Have a nice day :)

Thomas

Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area

[Lidong Chen]

> On Dec 15, 2022, at 10:00 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> On Wed, 14 Dec 2022 18:55:03 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
>> In the code, the for loop advanced the entry pointer to the
>> next entry before checking if the next entry is within the
>> system use area boundary. Another issue in the code was that
>> there is no check for the size of system use area. For a
>> corrupted system, the size of system use area can be less than
>> the size of SUSP entry size (5 bytes).
> 
> The minimum size of a SUSP entry is 4, not 5. Examples of 4-byte entries
> are ST in SUSP and RE in RRIP. (Ending before ST would be harmless, because
> ST marks the end of the SUSP entry chain. But RE may appear before the end.)
> 

I will fix it.

> 
>> These can cause buffer
>> overrun. The fixes added the checks to ensure the read is
>> valid and within the boundary.
>> 
>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>> ---
>> grub-core/fs/iso9660.c | 31 +++++++++++++++++++++++++++----
>> 1 file changed, 27 insertions(+), 4 deletions(-)
>> 
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index 4f4cd6165..9170fa820 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>> #define GRUB_ISO9660_VOLDESC_PART	3
>> #define GRUB_ISO9660_VOLDESC_END	255
>> 
>> +#define GRUB_ISO9660_SUSP_HEADER_SZ	5
> 
> So i think this should be 4, not 5.
> If we find reason why it should be 5, then the name SUSP_HEADER_SZ is not
> appropriate. The SUSP header size is surely 4.

You are right. SUSP-1.12 stated: 
“If the remaining allocated space following the last recorded System Use
 Entry in a System Use field or Continuation Area is less than 4 bytes long,
 It cannot contain a System Use Entry and should be ignored”

It implied the minimum SUSP Entry size is 4.  I will fix it.

> 
> 
>> +
>> /* The head of a volume descriptor.  */
>> struct grub_iso9660_voldesc
>> {
>> @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>>   if (sua_size <= 0)
>>     return GRUB_ERR_NONE;
>> 
>> +  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
>> +    return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
>> +
> 
> Here we need 4.
> 
> 
>>   sua = grub_malloc (sua_size);
>>   if (!sua)
>>     return grub_errno;
>> @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>>   if (err)
>>     return err;
>> 
>> -  for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry < (char *) sua + sua_size - 1 && entry->len > 0;
>> -       entry = (struct grub_iso9660_susp_entry *)
>> -	 ((char *) entry + entry->len))
>> +  entry = (struct grub_iso9660_susp_entry *) sua;
>> +
>> +  while (entry->len > 0)
>>     {
>> +      /* Ensure the entry is within System Use Area */
>> +      if ((char *) entry + entry->len > (sua + sua_size))
>> +        break;
>> +
>>       /* The last entry.  */
>>       if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
>> 	break;
>> @@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>> 	  off = grub_le_to_cpu32 (ce->off);
>> 	  ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
>> 
>> +	  if (sua_size <= 0)
>> +	    break;
>> +
>> +	  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> 
> 4 would be appropriate here.
> 
> 
>> +	    {
>> +	      grub_free (sua);
>> +	      return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");
>> +	    }
>> +
>> 	  grub_free (sua);
>> 	  sua = grub_malloc (sua_size);
>> 	  if (!sua)
>> @@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>> 	  grub_free (sua);
>> 	  return 0;
>> 	}
>> +
>> +      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
> 
> This is equivalent to the third statement in the "for" which was
> replaced by a while-loop. So far so good.
> 
> But i believe to see an old bug. This advancing of entry breaks the
> chain of CE if the first entry of the Continuation Area is again a CE.
> 
>          err = grub_disk_read (node->data->disk, ce_block, off,
>                                sua_size, sua);
>          ...
>          entry = (struct grub_iso9660_susp_entry *) sua;
>        }
> 
>      if (hook (entry, hook_arg))
>        {
>          grub_free (sua);
>          return 0;
>        }
> 
>      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
> 
> hook() will not interpret CE (and has no means to advance "entry").
> entry = entry + entry->len; will then skip over the entry so that the loop
> will not interpret it either.
> The SUSP area will be perceived as having ended, although more SUSP entries
> are present for the file in question.
> 
> I hereby ask for more opinions about this. Maybe i misinterpret the old loop.
> 
> Background:
> CE points to the block and offset where the chain of SUSP entries goes on.
> It is needed if the SUSP entry set exceeds the size limit of 255 bytes for
> a directory record.
> The byte at (ce->blk * block_size + ce->off) is the first byte of the next
> SUSP entry in the chain.
> Linux hates SUSP crossing block boundaries. So we ISO producers use further
> CE entries to hop over block boundaries.
> 
> libisofs can produce a Continuation Area with first and only entry CE,
> which then points to a new block with more SUSP payload. This happens if a
> continuation block offers not much more than 28 free bytes, so that only
> the 28 bytes of CE have room.
> 
> 
> (GRUB is not really correct by performing the CE jump immediately when CE is
> seen. The specs allow more SUSP entries to follow up to the end of the
> directory record's SUSP range. Only after interpreting them, an encountered
> CE should cause the jump to its Continuation Area. SUSP-1.12, 5.1:
>  "The "CE" System Use Entry indicates a Continuation Area that shall be
>   processed after the current System Use field or Continuation Area is
>   processed."
> mkisofs and libisofs both put their CE at the end of the directory record
> of Continuation Area. So an exotic ISO would be needed to demonstrate a
> problem with GRUB's immediate CE interpretation.)
> 
> 
>> +
>> +      if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
>> +        break;
> 
> 4 would be appropriate.
> 
> 
>>     }
>> 
>>   grub_free (sua);
>> @@ -574,7 +597,7 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>> 	  char *old;
>> 	  grub_size_t sz;
>> 
>> -	  csize = entry->len - 5;
>> +	  csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;
> 
> Here it is not appropriate to use GRUB_ISO9660_SUSP_HEADER_SZ.
> This code subtracts the count of the 4 header bytes of an NM entry and of
> the 1 FLAGS byte of NM. Goal is to compute the size of the name string.
> So if GRUB_ISO9660_SUSP_HEADER_SZ is defined as 4, then
> (GRUB_ISO9660_SUSP_HEADER_SZ + 1) would be ok. But a plain 5 is ok, too,
> because this is a specific property of the NM definition of version 1.

Ok, I will revert the change and keep the plain ‘5’ as it is. 

> 
> (Actually one should verify (entry->version == 1) before interpreting
> an NM entry that way. Well, i see that libisofs doesn't check either.
> Excuse is that the RRIP specs did not change since the mid 1990s.)

Ok, for that excuse, we don’t add the check for the version till it is needed then.

> 
> Whatever gets decided, the "5", which is six lines higher, should be changed
> to the same expression:
> 
>      else if (entry->len >= 5)
> 
> 
>> 	  old = ctx->filename;
>> 	  if (ctx->filename_alloc)
>> 	    {
>> --
>> 2.35.1
>> 
> 
> So no "Reviewed-by:" yet.
> 
> Open issues:
> 
> The definition of GRUB_ISO9660_SUSP_HEADER_SZ should be changed to 4.
> 
> Within the interpretation of NM this change should not happen:
>> -       csize = entry->len - 5;
>> +       csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;

I will fix it.

Thanks,
Lidong

> 
> We need to decide what to do with the potential old bug that a CE entry
> as the first entry of a Continuation Area gets skipped without
> interpretation.
> 
> 
> Have a nice day :)
> 
> Thomas
> 

Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area

[Thomas Schmitt]

Hi,

while preparing a proposal how to avoid skipping of CE (and ST) if they
are found at the start of a continuation area, i came to a problem of
patch [2/4] which i did not see when reviewing it yesterday:

> +           return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");

It is not about the size of the CE entry but about the size of the
continuation area which the CE entry announces.

So i propose as error message

  "invalid continuation area size in CE entry"


Have a nice day :)

Thomas

Proposal: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Thomas Schmitt]

Hi,

i believe that the problem about CE entries sitting at the start of a SUSP
continuation area could be quite easily solved after patch [2/4] was applied,
which changes the for-loop to a while loop with the entry-advance step at its
end.

So i propose to Lidong Chen to handle this problem by another patch in
the series, which comes after [2/4] (then [2/5]).
Please verify that my thoughts about CE (and additionally about ST) are
correct and consider the following diff, which compiles but is not tested
otherwise.

I agree in advance to changing my "Signed-off-by:" to "Suggested-by:" if
deemed appropriate.

------------------------------------------------------------------------

fs/iso9660: Prevent skipping CE or ST at start of continuation area

If processing of a SUSP CE entry leads to a continuation area which begins
by entry CE or ST, then these entries were skipped without interpretation.
In case of CE this would lead to premature end of processing the SUSP entries
of the file.
In case of ST this could cause following non-SUSP bytes to be interpreted
as SUSP entries.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>

--- grub-core/fs/iso9660.c.lidong_chen_patch_2	2022-12-15 11:46:50.654295924 +0100
+++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix	2022-12-16 10:05:52.990599283 +0100
@@ -336,6 +336,12 @@ grub_iso9660_susp_iterate (grub_fshelp_n
 	    }

 	  entry = (struct grub_iso9660_susp_entry *) sua;
+
+	  if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
+	      || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
+	      /* The hook function will not process CE or ST.
+		 Advancing to the next entry would skip them. */
+	      continue;
 	}

       if (hook (entry, hook_arg))

------------------------------------------------------------------------


Have a nice day :)

Thomas

Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Thomas Schmitt]

Hi,

i realize that my previous proposal opens a possibility for regression with
a very bad ISO image.
The danger is in an endless loop by a CE entry which points to itself.
The bug which i want to see fixed currently prevents this special pitfall.

(Other endless loops by CE are possible and not prevented. It's much like
 with symbolic link loops. In the end only a hard limit on the number of
 CE hops would help.)

So i now propose with the same sketch of a commit message, this change
(compiles but was but not tested):
------------------------------------------------------------------------

fs/iso9660: Prevent skipping CE or ST at start of continuation area

If processing of a SUSP CE entry leads to a continuation area which begins
by entry CE or ST, then these entries were skipped without interpretation.
In case of CE this would lead to premature end of processing the SUSP entries
of the file.
In case of ST this could cause following non-SUSP bytes to be interpreted
as SUSP entries.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>

--- grub-core/fs/iso9660.c.lidong_chen_patch_2	2022-12-15 11:46:50.654295924 +0100
+++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix_v2	2022-12-16 13:54:55.654651173 +0100
@@ -336,6 +336,21 @@ grub_iso9660_susp_iterate (grub_fshelp_n
 	    }

 	  entry = (struct grub_iso9660_susp_entry *) sua;
+
+	  /* The hook function will not process CE or ST.
+	     Advancing to the next entry would skip them. */
+	  if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
+	    {
+	      ce = (struct grub_iso9660_susp_ce *) entry;
+	      if (ce_block
+		  != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
+		  || off != grub_le_to_cpu32 (ce->off))
+		continue;
+	      /* Ending up here indicates an endless loop by self reference.
+	         So skip this bad CE as was done before december 2022. */
+	    }
+	  if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
+	    break;
 	}

       if (hook (entry, hook_arg))
------------------------------------------------------------------------


Have a nice day :)

Thomas

Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Lidong Chen]

> On Dec 16, 2022, at 4:57 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> i realize that my previous proposal opens a possibility for regression with
> a very bad ISO image.
> The danger is in an endless loop by a CE entry which points to itself.
> The bug which i want to see fixed currently prevents this special pitfall.
> 
> (Other endless loops by CE are possible and not prevented. It's much like
> with symbolic link loops. In the end only a hard limit on the number of
> CE hops would help.)
> 
> So i now propose with the same sketch of a commit message, this change
> (compiles but was but not tested):
> ------------------------------------------------------------------------
> 
> fs/iso9660: Prevent skipping CE or ST at start of continuation area
> 
> If processing of a SUSP CE entry leads to a continuation area which begins
> by entry CE or ST, then these entries were skipped without interpretation.
> In case of CE this would lead to premature end of processing the SUSP entries
> of the file.
> In case of ST this could cause following non-SUSP bytes to be interpreted
> as SUSP entries.
> 
> Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
> 
> --- grub-core/fs/iso9660.c.lidong_chen_patch_2	2022-12-15 11:46:50.654295924 +0100
> +++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix_v2	2022-12-16 13:54:55.654651173 +0100
> @@ -336,6 +336,21 @@ grub_iso9660_susp_iterate (grub_fshelp_n
> 	    }
> 
> 	  entry = (struct grub_iso9660_susp_entry *) sua;
> +
> +	  /* The hook function will not process CE or ST.
> +	     Advancing to the next entry would skip them. */
> +	  if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
> +	    {
> +	      ce = (struct grub_iso9660_susp_ce *) entry;
> +	      if (ce_block
> +		  != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
> +		  || off != grub_le_to_cpu32 (ce->off))
> +		continue;
> +	      /* Ending up here indicates an endless loop by self reference.
> +	         So skip this bad CE as was done before december 2022. */
> +	    }
> +	  if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> +	    break;
> 	}
> 
>       if (hook (entry, hook_arg))
> ————————————————————————————————————
Thank you for providing the diff! I will create a new patch for the changes.

Thanks,
Lidong 
> 
> 
> Have a nice day :)
> 
> Thomas
> 

Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Lidong Chen]

[-- Attachment #1: Type: text/plain, Size: 3283 bytes --]



On Dec 16, 2022, at 4:57 AM, Thomas Schmitt <scdbackup@gmx.net<mailto:scdbackup@gmx.net>> wrote:

Hi,

i realize that my previous proposal opens a possibility for regression with
a very bad ISO image.
The danger is in an endless loop by a CE entry which points to itself.
The bug which i want to see fixed currently prevents this special pitfall.

(Other endless loops by CE are possible and not prevented. It's much like
with symbolic link loops. In the end only a hard limit on the number of
CE hops would help.)

So i now propose with the same sketch of a commit message, this change
(compiles but was but not tested):
------------------------------------------------------------------------

fs/iso9660: Prevent skipping CE or ST at start of continuation area

If processing of a SUSP CE entry leads to a continuation area which begins
by entry CE or ST, then these entries were skipped without interpretation.
In case of CE this would lead to premature end of processing the SUSP entries
of the file.
In case of ST this could cause following non-SUSP bytes to be interpreted
as SUSP entries.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net<mailto:scdbackup@gmx.net>>

--- grub-core/fs/iso9660.c.lidong_chen_patch_2 2022-12-15 11:46:50.654295924 +0100
+++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix_v2 2022-12-16 13:54:55.654651173 +0100
@@ -336,6 +336,21 @@ grub_iso9660_susp_iterate (grub_fshelp_n
   }

 entry = (struct grub_iso9660_susp_entry *) sua;
+
+  /* The hook function will not process CE or ST.
+     Advancing to the next entry would skip them. */
+  if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
+    {
+      ce = (struct grub_iso9660_susp_ce *) entry;
+      if (ce_block
+  != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
+  || off != grub_le_to_cpu32 (ce->off))
+ continue;
+      /* Ending up here indicates an endless loop by self reference.
+         So skip this bad CE as was done before december 2022. */
In the original the CE code, ‘off’ and ‘ce_block’ were assigned with
the values (highlighted below) that the above suggested fix tries to
check against. So, it looks like it will never end here. Are the above check
on ‘ce_block’ and ‘off’ still needed?

      /* The last entry.  */
      if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
        break;



      /* Additional entries are stored elsewhere.  */
      if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
        {
          struct grub_iso9660_susp_ce *ce;
          grub_disk_addr_t ce_block;

          ce = (struct grub_iso9660_susp_ce *) entry;
          sua_size = grub_le_to_cpu32 (ce->len);
          off = grub_le_to_cpu32 (ce->off);
          ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;

         ….
          entry = (struct grub_iso9660_susp_entry *) sua;
        }

      if (hook (entry, hook_arg))
        {
          grub_free (sua);
          return 0;
        }
+    }
+  if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
+    break;
}

      if (hook (entry, hook_arg))
------------------------------------------------------------------------

Thanks,
Lidong



Have a nice day :)

Thomas



[-- Attachment #2: Type: text/html, Size: 14687 bytes --]

Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Thomas Schmitt]

Hi,

i wrote on Fri, 16 Dec 2022 10:42:13 +0100:
> > If processing of a SUSP CE entry leads to a continuation area which begins
> > by entry CE or ST, then these entries were skipped without interpretation.
> > In case of CE this would lead to premature end of processing the SUSP
> > entries of the file.
> > In case of ST this could cause following non-SUSP bytes to be interpreted
> > as SUSP entries.
> > [...]
> >             }
> >
> >           entry = (struct grub_iso9660_susp_entry *) sua;
> > +
> > +         if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
> > +             || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> > +             /* The hook function will not process CE or ST.
> > +                Advancing to the next entry would skip them. */
> > +             continue;
> >         }
> >
> >        if (hook (entry, hook_arg))

I wrote on Fri, 16 Dec 2022 13:57:04 +0100:
> > i realize that my previous proposal opens a possibility for regression
> > with a very bad ISO image.
> > The danger is in an endless loop by a CE entry which points to itself.
> > [...] So i now propose:
> >             }
> >
> >           entry = (struct grub_iso9660_susp_entry *) sua;
> > +
> > +         /* The hook function will not process CE or ST.
> > +            Advancing to the next entry would skip them. */
> > +         if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
> > +           {
> > +             ce = (struct grub_iso9660_susp_ce *) entry;
> > +             if (ce_block
> > +                 != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
> > +                 || off != grub_le_to_cpu32 (ce->off))
> > +               continue;
> > +             /* Ending up here indicates an endless loop by self reference.
> > +                So skip this bad CE as was done before december 2022. */
> > +           }
> > +         if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> > +           break;
> >         }
> >
> >        if (hook (entry, hook_arg))

Lidong Chen wrote:
> In the original the CE code, ‘off’ and ‘ce_block’ were assigned with 
> the values (highlighted below) that the above suggested fix tries to 
> check against. So, it looks like it will never end here.
>         off = grub_le_to_cpu32 (ce->off);
>         ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;

This happens before "entry" gets pointed to newly allocated memory which
then gets filled with the data from the continuation area

          grub_free (sua);
          sua = grub_malloc (sua_size);
          if (!sua)
            return grub_errno;

          /* Load a part of the System Usage Area.  */
          err = grub_disk_read (node->data->disk, ce_block, off,
                                sua_size, sua);
          if (err)
            {
              grub_free (sua);
              return err;
            }

          entry = (struct grub_iso9660_susp_entry *) sua;

Afterwards my proposed check shall peek ahead whether the suspicious new
CE at the begin of this continuation area is a simple hoax which points
to itself.

------------------------------------------------------------------------

But meanwhile i think that a full fledged test for an endless loop is
more appropriate.
E.g. by a counter which breaks the loop:

--- grub-core/fs/iso9660.c.lidong_chen_patch_2	2022-12-15 11:46:50.654295924 +0100
+++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix_v3	2023-01-06 16:31:30.226698552 +0100
@@ -176,6 +176,8 @@ enum
     FLAG_MORE_EXTENTS = 0x80
   };

+#define GRUB_ISO9660_MAX_CE_HOPS	1000000
+
 static grub_dl_t my_mod;
 \f

@@ -270,6 +272,7 @@ grub_iso9660_susp_iterate (grub_fshelp_n
   char *sua;
   struct grub_iso9660_susp_entry *entry;
   grub_err_t err;
+  int ce_counter = 0;

   if (sua_size <= 0)
     return GRUB_ERR_NONE;
@@ -307,6 +310,13 @@ grub_iso9660_susp_iterate (grub_fshelp_n
 	  struct grub_iso9660_susp_ce *ce;
 	  grub_disk_addr_t ce_block;

+	  if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
+	    {
+	      grub_free (sua);
+	      return grub_error (GRUB_ERR_BAD_FS,
+				 "suspecting endless CE loop");
+	    }
+
 	  ce = (struct grub_iso9660_susp_ce *) entry;
 	  sua_size = grub_le_to_cpu32 (ce->len);
 	  off = grub_le_to_cpu32 (ce->off);

I don't add a signed-off-by because this is uncompiled and still needs
thought about the maximum number of continuation hops and the reaction
to a detected overflow of that number. Who does that work is the author
of the patch.
(1 million suffices for 2048 million bytes of SUSP data per file.
You could silently bail out if this number is surpassed.)


The check would be a separate patch, accompanied by my proposal v1 which
then would need no own safety check:

@@ -336,6 +346,12 @@ grub_iso9660_susp_iterate (grub_fshelp_n
 	    }

 	  entry = (struct grub_iso9660_susp_entry *) sua;
+
+	  if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
+	      || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
+	      /* The hook function will not process CE or ST.
+		 Advancing to the next entry would skip them. */
+	      continue;
 	}

       if (hook (entry, hook_arg))

(This one is already signed off by me.)


Have a nice dday :)

Thomas

Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Lidong Chen]

Hi Thomas,

> On Jan 6, 2023, at 8:00 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> i wrote on Fri, 16 Dec 2022 10:42:13 +0100:
>>> If processing of a SUSP CE entry leads to a continuation area which begins
>>> by entry CE or ST, then these entries were skipped without interpretation.
>>> In case of CE this would lead to premature end of processing the SUSP
>>> entries of the file.
>>> In case of ST this could cause following non-SUSP bytes to be interpreted
>>> as SUSP entries.
>>> [...]
>>>            }
>>> 
>>>          entry = (struct grub_iso9660_susp_entry *) sua;
>>> +
>>> +         if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
>>> +             || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
>>> +             /* The hook function will not process CE or ST.
>>> +                Advancing to the next entry would skip them. */
>>> +             continue;
>>>        }
>>> 
>>>       if (hook (entry, hook_arg))
> 
> I wrote on Fri, 16 Dec 2022 13:57:04 +0100:
>>> i realize that my previous proposal opens a possibility for regression
>>> with a very bad ISO image.
>>> The danger is in an endless loop by a CE entry which points to itself.
>>> [...] So i now propose:
>>>            }
>>> 
>>>          entry = (struct grub_iso9660_susp_entry *) sua;
>>> +
>>> +         /* The hook function will not process CE or ST.
>>> +            Advancing to the next entry would skip them. */
>>> +         if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
>>> +           {
>>> +             ce = (struct grub_iso9660_susp_ce *) entry;
>>> +             if (ce_block
>>> +                 != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
>>> +                 || off != grub_le_to_cpu32 (ce->off))
>>> +               continue;
>>> +             /* Ending up here indicates an endless loop by self reference.
>>> +                So skip this bad CE as was done before december 2022. */
>>> +           }
>>> +         if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
>>> +           break;
>>>        }
>>> 
>>>       if (hook (entry, hook_arg))
> 
> Lidong Chen wrote:
>> In the original the CE code, ‘off’ and ‘ce_block’ were assigned with 
>> the values (highlighted below) that the above suggested fix tries to 
>> check against. So, it looks like it will never end here.
>>         off = grub_le_to_cpu32 (ce->off);
>>         ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
> 
> This happens before "entry" gets pointed to newly allocated memory which
> then gets filled with the data from the continuation area
> 
>          grub_free (sua);
>          sua = grub_malloc (sua_size);
>          if (!sua)
>            return grub_errno;
> 
>          /* Load a part of the System Usage Area.  */
>          err = grub_disk_read (node->data->disk, ce_block, off,
>                                sua_size, sua);
>          if (err)
>            {
>              grub_free (sua);
>              return err;
>            }
> 
>          entry = (struct grub_iso9660_susp_entry *) sua;
> 
> Afterwards my proposed check shall peek ahead whether the suspicious new
> CE at the begin of this continuation area is a simple hoax which points
> to itself.
> 

Thanks for the clarification. I created a new patch for the fix and added you as the “Signed-off-by”.
My question is how to test it.  

The issues addressed by the other 4 patches were found by fuzzing. I restarted the fuzzing on 
those 4 patches, there was no crashes and hangs found. So, the patches fixed the issues.

For the new patch, since it wasn’t found by the fuzzer, I am not sure how to test it. 

> ------------------------------------------------------------------------
> 
> But meanwhile i think that a full fledged test for an endless loop is
> more appropriate.
> E.g. by a counter which breaks the loop:
> 
> --- grub-core/fs/iso9660.c.lidong_chen_patch_2	2022-12-15 11:46:50.654295924 +0100
> +++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix_v3	2023-01-06 16:31:30.226698552 +0100
> @@ -176,6 +176,8 @@ enum
>     FLAG_MORE_EXTENTS = 0x80
>   };
> 
> +#define GRUB_ISO9660_MAX_CE_HOPS	1000000
> +
> static grub_dl_t my_mod;
> 
> 
> @@ -270,6 +272,7 @@ grub_iso9660_susp_iterate (grub_fshelp_n
>   char *sua;
>   struct grub_iso9660_susp_entry *entry;
>   grub_err_t err;
> +  int ce_counter = 0;
> 
>   if (sua_size <= 0)
>     return GRUB_ERR_NONE;
> @@ -307,6 +310,13 @@ grub_iso9660_susp_iterate (grub_fshelp_n
> 	  struct grub_iso9660_susp_ce *ce;
> 	  grub_disk_addr_t ce_block;
> 
> +	  if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
> +	    {
> +	      grub_free (sua);
> +	      return grub_error (GRUB_ERR_BAD_FS,
> +				 "suspecting endless CE loop");
> +	    }
> +
> 	  ce = (struct grub_iso9660_susp_ce *) entry;
> 	  sua_size = grub_le_to_cpu32 (ce->len);
> 	  off = grub_le_to_cpu32 (ce->off);

I compiled the changes.  I have the same question for the testing changes.

Thanks,
Lidong

> 
> I don't add a signed-off-by because this is uncompiled and still needs
> thought about the maximum number of continuation hops and the reaction
> to a detected overflow of that number. Who does that work is the author
> of the patch.
> (1 million suffices for 2048 million bytes of SUSP data per file.
> You could silently bail out if this number is surpassed.)
> 
> 
> The check would be a separate patch, accompanied by my proposal v1 which
> then would need no own safety check:
> 
> @@ -336,6 +346,12 @@ grub_iso9660_susp_iterate (grub_fshelp_n
> 	    }
> 
> 	  entry = (struct grub_iso9660_susp_entry *) sua;
> +
> +	  if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
> +	      || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> +	      /* The hook function will not process CE or ST.
> +		 Advancing to the next entry would skip them. */
> +	      continue;
> 	}
> 
>       if (hook (entry, hook_arg))
> 
> (This one is already signed off by me.)
> 
> 
> Have a nice dday :)
> 
> Thomas
> 

Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Thomas Schmitt]

Hi,

Lidong Chen wrote:
> Thanks for the clarification. I created a new patch for the fix and added
> you as the “Signed-off-by”.
> My question is how to test it.

I just sent libisofs into an endless recursion loop. Grrr.
For this i created a small ISO with a file which surely needs a CE, changed
its fields so that it points to itself, and attempted to load it by xorriso.
(At least it ends by SIGSEGV on an exhausted stack and does not cycle
endlessly.)

--------------------------------------------------------------------------
ISO creation:

   # A dummy file as payload
   echo x >x

   # 250 fattr characters to surely exceed the size of a directory entry
   long_string=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789

   # Create ISO with the payload file and attached fattr named user.dummy
   test -e ce_loop.iso && rm ce_loop.iso
   xorriso -outdev ce_loop.iso \
           -xattr on \
           -map x /x \
           -setfattr user.dummy "$long_string" /x -- \
           -padding 0

Next i determined by a hex dumper the location of the only Rock Ridge
entry NM and then the location of the next following CE entry (the first
CE belongs to the root directory):
  Byte 102734 decimal = 50 * 2048 + 334 = LBA 0x32 + Offset 0x014e
The size of the continuation area is 270 = 0x010e
The length of CE is 28 = 0x1c

So i patched the own address and size of the CE entry into its fields:

  # The block address (little endian and big endian)
  echo $'\x32' | dd bs=1 seek=102738 count=1 conv=notrunc of=ce_loop.iso
  echo $'\x32' | dd bs=1 seek=102745 count=1 conv=notrunc of=ce_loop.iso

  # The byte offset in that block
  echo $'\x4e'$'\x01' | dd bs=1 seek=102746 count=2 conv=notrunc of=ce_loop.iso
  echo $'\x01'$'\x4e' | dd bs=1 seek=102752 count=2 conv=notrunc of=ce_loop.iso

  # The new size of the CE area is the length of the CE entry
  echo $'\x1c' | dd bs=1 seek=102754 count=1 conv=notrunc of=ce_loop.iso
  echo $'\x1c' | dd bs=1 seek=102761 count=1 conv=notrunc of=ce_loop.iso
  # Zeroize the higher valued bytes of the length field
  dd if=/dev/zero bs=1 seek=102755 count=6 conv=notrunc of=ce_loop.iso

This really bad CE entry then looks in the hex dump like:

00019140 :    00  7b  01  09  08  08  1c  00  4e  4d  06  01  00  78  43  45
                   {                           N   M               x   C   E
  102720 :     0 123   1   9   8   8  28   0  78  77   6   1   0 120  67  69

00019150 :    1c  01  32  00  00  00  00  00  00  32  4e  01  00  00  00  00
                       2                           2   N
  102736 :    28   1  50   0   0   0   0   0   0  50  78   1   0   0   0   0

00019160 :    01  4e  1c  00  00  00  00  00  00  1c  00  00  00  00  00  00
                   N
  102752 :     1  78  28   0   0   0   0   0   0  28   0   0   0   0   0   0

(Note: 00019140 is hex, not octal)

--------------------------------------------------------------------------

I will first try to fix libisofs before i upload this gremlin somewhere.

If you want to try in advance, run above shell commands and check whether
a hex editor shows the expected bytes afterwards. (xorriso is supposed to
be reproducible in respect to sizes and locations. Very old versions might
not fulfill that expectation, though.)


Have a nice day :)

Thomas

Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Thomas Schmitt]

Hi,

i created another bad ISO which i expect to lead to an endless loop in
existing GRUB (i.e. before applying the proposed change).

Both ISOs can be downloaded as gzip-compressed files now:

  http://scdbackup.webframe.org/ce_loop.iso.gz
  SHA256: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2

  http://scdbackup.webframe.org/ce_loop2.iso.gz
  SHA256: a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c

They are smaller than 1 KB and expand to 128 KiB, each.

(Please do not load these ISOs into existing xorriso programs by commands
like -indev. ce_loop.iso leads to SIGSEGV. ce_loop2.iso leads to an
endless loop. The libisofs code fix is only in git for now.)

------------------------------------------------------------------------
Test proposals for GRUB:

If reading ce_loop.iso by GRUB leads to an error message, then my proposed
change is in effect and protected against endless loops.
Original GRUB is supposed to just ignore that situation, because it skips
over the CE entry by mistake.

If reading ce_loop2.iso by GRUB leads to an error message, then the
proposed safety precaution against endless loops is in effect.
I expect unpatched GRUB to loop endlessly with this ISO.

------------------------------------------------------------------------
About the production of ce_loop2.iso:

The CE entry of its file /x points to a dummy SUSP entry XY of length 8
which sits directly before the CE entry. So this dummy entry is the
first to be read by GRUB from the pseudo-contination area, gets processed
by the hook() function (with no side effects), and is then followed by
the CE entry.
Because of the existence of the XY entry, i expect the CE entry not to
be skipped by existing GRUB.

  # Production begins by the bad ISO of January 9 where the CE entry
  # points to itself
  cp ce_loop.iso ce_loop2.iso

  # Cut out a copy of the bad CE entry
  dd if=ce_loop2.iso bs=1 skip=102734 count=28 of=ce_entry

  # After the CE entry is plenty of unused space in the same block.
  # The length of the directory entry of /x plus 8 will not exceed 255.
  # So put the copy back with an offset of 8 bytes.
  dd if=ce_entry bs=1 seek=102742 conv=notrunc of=ce_loop2.iso

  # Rename the old CE entry head to XY
  echo "XY" | dd bs=1 seek=102734 count=2 conv=notrunc of=ce_loop2.iso

  # Give it the length of 8
  echo $'\x08' | dd bs=1 seek=102736 count=1 conv=notrunc of=ce_loop2.iso

  # Set in the new CE entry the continuation area length to 8 + 28 = 36
  echo $'\x24' | dd bs=1 seek=102762 count=1 conv=notrunc of=ce_loop2.iso
  echo $'\x24' | dd bs=1 seek=102769 count=1 conv=notrunc of=ce_loop2.iso

  # Change the length of the directory record from 134 to 142
  echo $'\x8e' |  dd bs=1 seek=102628 count=1 conv=notrunc of=ce_loop2.iso

The resulting bytes of the whole directory record of /x are then:

000190e0 :    ..  ..  ..  ..  8e  00  37  00  00  00  00  00  00  37  02  00
               .   .   .   .           7                           7
  102624 :   ... ... ... ... 142   0  55   0   0   0   0   0   0  55   2   0

000190f0 :    00  00  00  00  00  02  7b  01  09  08  08  1c  00  00  00  00
                                       {
  102640 :     0   0   0   0   0   2 123   1   9   8   8  28   0   0   0   0

00019100 :    01  00  00  01  04  58  2e  3b  31  00  50  58  24  01  a4  81
                                   X   .   ;   1       P   X   $
  102656 :     1   0   0   1   4  88  46  59  49   0  80  88  36   1 164 129

00019110 :    00  00  00  00  81  a4  01  00  00  00  00  00  00  01  e8  03

  102672 :     0   0   0   0 129 164   1   0   0   0   0   0   0   1 232   3

00019120 :    00  00  00  00  03  e8  e8  03  00  00  00  00  03  e8  54  46
                                                                       T   F
  102688 :     0   0   0   0   3 232 232   3   0   0   0   0   3 232  84  70

00019130 :    1a  01  0e  7b  01  09  08  08  1c  00  7b  01  09  08  08  2f
                           {                           {                   /
  102704 :    26   1  14 123   1   9   8   8  28   0 123   1   9   8   8  47

00019140 :    00  7b  01  09  08  08  1c  00  4e  4d  06  01  00  78  58  59
                   {                           N   M               x   X   Y
  102720 :     0 123   1   9   8   8  28   0  78  77   6   1   0 120  88  89

00019150 :    08  01  32  00  00  00  43  45  1c  01  32  00  00  00  00  00
                       2               C   E           2
  102736 :     8   1  50   0   0   0  67  69  28   1  50   0   0   0   0   0

00019160 :    00  32  4e  01  00  00  00  00  01  4e  24  00  00  00  00  00
                   2   N                           N   $
  102752 :     0  50  78   1   0   0   0   0   1  78  36   0   0   0   0   0

00019170 :    00  24  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..
                   $   .   .   .   .   .   .   .   .   .   .   .   .   .   .
  102768 :     0  36 ... ... ... ... ... ... ... ... ... ... ... ... ... ...

------------------------------------------------------------------------

Have a nice day :)

Thomas

Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Lidong Chen]

Hi Thomas,

> On Jan 11, 2023, at 3:54 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> i created another bad ISO which i expect to lead to an endless loop in
> existing GRUB (i.e. before applying the proposed change).
> 
> Both ISOs can be downloaded as gzip-compressed files now:
> 
>  http://scdbackup.webframe.org/ce_loop.iso.gz
>  SHA256: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2
> 
>  http://scdbackup.webframe.org/ce_loop2.iso.gz
>  SHA256: a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c
> 
> They are smaller than 1 KB and expand to 128 KiB, each.
> 
> (Please do not load these ISOs into existing xorriso programs by commands
> like -indev. ce_loop.iso leads to SIGSEGV. ce_loop2.iso leads to an
> endless loop. The libisofs code fix is only in git for now.)
> 
I have downloaded the ISOs. Thanks!

> ------------------------------------------------------------------------
> Test proposals for GRUB:
> 
> If reading ce_loop.iso by GRUB leads to an error message, then my proposed
> change is in effect and protected against endless loops.
> Original GRUB is supposed to just ignore that situation, because it skips
> over the CE entry by mistake.
> 
> If reading ce_loop2.iso by GRUB leads to an error message, then the
> proposed safety precaution against endless loops is in effect.
> I expect unpatched GRUB to loop endlessly with this ISO.
> 
To test it, I am thinking to add the ISO entry in 40_custom script, then select
the ISO from Grub menu. Is it the right way to test it? Or, is there a better way
to it? 

> ------------------------------------------------------------------------
> About the production of ce_loop2.iso:
> 
> The CE entry of its file /x points to a dummy SUSP entry XY of length 8
> which sits directly before the CE entry. So this dummy entry is the
> first to be read by GRUB from the pseudo-contination area, gets processed
> by the hook() function (with no side effects), and is then followed by
> the CE entry.
> Because of the existence of the XY entry, i expect the CE entry not to
> be skipped by existing GRUB.
> 
>  # Production begins by the bad ISO of January 9 where the CE entry
>  # points to itself
>  cp ce_loop.iso ce_loop2.iso
> 
>  # Cut out a copy of the bad CE entry
>  dd if=ce_loop2.iso bs=1 skip=102734 count=28 of=ce_entry
> 
>  # After the CE entry is plenty of unused space in the same block.
>  # The length of the directory entry of /x plus 8 will not exceed 255.
>  # So put the copy back with an offset of 8 bytes.
>  dd if=ce_entry bs=1 seek=102742 conv=notrunc of=ce_loop2.iso
> 
>  # Rename the old CE entry head to XY
>  echo "XY" | dd bs=1 seek=102734 count=2 conv=notrunc of=ce_loop2.iso
> 
>  # Give it the length of 8
>  echo $'\x08' | dd bs=1 seek=102736 count=1 conv=notrunc of=ce_loop2.iso
> 
>  # Set in the new CE entry the continuation area length to 8 + 28 = 36
>  echo $'\x24' | dd bs=1 seek=102762 count=1 conv=notrunc of=ce_loop2.iso
>  echo $'\x24' | dd bs=1 seek=102769 count=1 conv=notrunc of=ce_loop2.iso
> 
>  # Change the length of the directory record from 134 to 142
>  echo $'\x8e' |  dd bs=1 seek=102628 count=1 conv=notrunc of=ce_loop2.iso
> 
> The resulting bytes of the whole directory record of /x are then:
> 
> 000190e0 :    ..  ..  ..  ..  8e  00  37  00  00  00  00  00  00  37  02  00
>               .   .   .   .           7                           7
>  102624 :   ... ... ... ... 142   0  55   0   0   0   0   0   0  55   2   0
> 
> 000190f0 :    00  00  00  00  00  02  7b  01  09  08  08  1c  00  00  00  00
>                                       {
>  102640 :     0   0   0   0   0   2 123   1   9   8   8  28   0   0   0   0
> 
> 00019100 :    01  00  00  01  04  58  2e  3b  31  00  50  58  24  01  a4  81
>                                   X   .   ;   1       P   X   $
>  102656 :     1   0   0   1   4  88  46  59  49   0  80  88  36   1 164 129
> 
> 00019110 :    00  00  00  00  81  a4  01  00  00  00  00  00  00  01  e8  03
> 
>  102672 :     0   0   0   0 129 164   1   0   0   0   0   0   0   1 232   3
> 
> 00019120 :    00  00  00  00  03  e8  e8  03  00  00  00  00  03  e8  54  46
>                                                                       T   F
>  102688 :     0   0   0   0   3 232 232   3   0   0   0   0   3 232  84  70
> 
> 00019130 :    1a  01  0e  7b  01  09  08  08  1c  00  7b  01  09  08  08  2f
>                           {                           {                   /
>  102704 :    26   1  14 123   1   9   8   8  28   0 123   1   9   8   8  47
> 
> 00019140 :    00  7b  01  09  08  08  1c  00  4e  4d  06  01  00  78  58  59
>                   {                           N   M               x   X   Y
>  102720 :     0 123   1   9   8   8  28   0  78  77   6   1   0 120  88  89
> 
> 00019150 :    08  01  32  00  00  00  43  45  1c  01  32  00  00  00  00  00
>                       2               C   E           2
>  102736 :     8   1  50   0   0   0  67  69  28   1  50   0   0   0   0   0
> 
> 00019160 :    00  32  4e  01  00  00  00  00  01  4e  24  00  00  00  00  00
>                   2   N                           N   $
>  102752 :     0  50  78   1   0   0   0   0   1  78  36   0   0   0   0   0
> 
> 00019170 :    00  24  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..
>                   $   .   .   .   .   .   .   .   .   .   .   .   .   .   .
>  102768 :     0  36 ... ... ... ... ... ... ... ... ... ... ... ... … …
> 
Thanks a lot for the detail instruction! It is very helpful for the test as well as for my learning.

Regards,
Lidong

> ------------------------------------------------------------------------
> 
> Have a nice day :)
> 
> Thomas
> 

Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Thomas Schmitt]

Hi,

Lidong Chen wrote:
> To test it, I am thinking to add the ISO entry in 40_custom script, then select
> the ISO from Grub menu. Is it the right way to test it? Or, is there a better way
> to it?

I have to leave the answer to the experienced GRUB developers.

Testing is my weak spot with GRUB. About 6 weeks ago i tried to demonstrate
a risk for memory fault and grub-fstest simply did not want to fail.

This reminds me that i should have tested my ISOs with grub-fstest before
posting them. To my luck the program as pulled 6 weeks ago behaves like i
predicted:

  $ ./grub-fstest ce_loop.iso ls /
  x
  $ ./grub-fstest ce_loop2.iso ls /
  ^C
  $

I waited about half a minute (on a 4 GHz Xeon) for the second run to end.
Then i aborted it by Ctrl+C.
As i am at it, i tried with Linux kernel "5.10.0-13-amd64 #1 SMP Debian
5.10.106-1 (2022-03-17)":

  # mount ce_loop.iso /mnt/iso
  mount: /mnt/iso: WARNING: source write-protected, mounted read-only.
  # ls -l /mnt/iso
  total 0
  #

No file /x but also no special messages in dmesg. Only:
  [ ...] loop: module loaded
  [ ...] ISO 9660 Extensions: RRIP_1991A

Same behavior with the ISO which drives grub-fstest into the endless loop:

  # umount /mnt/iso
  # mount ce_loop2.iso /mnt/iso
  mount: /mnt/iso: WARNING: source write-protected, mounted read-only.
  # ls -l /mnt/iso
  total 0
  #

So Linux seems to be safe against this hack.
(I will have a look into the source in order to learn how this situation
gets handled.)


> Thanks a lot for the detail instruction! It is very helpful for the test as
> well as for my learning.

That's the topic where i can be of use.
Don't hesitate to ask for explanations, pointers to the specs, or nastily
manipulated ISOs.


Have a nice day :)

Thomas

[PATCH 3/4] fs/iso9660: Avoid reading past the entry boundary

[Lidong Chen]

Added a check for the SP entry data boundary before reading it.

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

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 9170fa820..67aa8451c 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -408,6 +408,9 @@ set_rockridge (struct grub_iso9660_data *data)
   if (!sua_size)
     return GRUB_ERR_NONE;
 
+  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
+    return grub_error (GRUB_ERR_BAD_FS, "invalid rock ridge entry size");
+
   sua = grub_malloc (sua_size);
   if (! sua)
     return grub_errno;
@@ -434,8 +437,17 @@ set_rockridge (struct grub_iso9660_data *data)
       rootnode.have_symlink = 0;
       rootnode.dirents[0] = data->voldesc.rootdir;
 
-      /* The 2nd data byte stored how many bytes are skipped every time
-	 to get to the SUA (System Usage Area).  */
+      /*
+       * The 2nd data byte stored how many bytes are skipped every time
+       * to get to the SUA (System Usage Area).
+       */
+      if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ + 2 ||
+	  entry->len < GRUB_ISO9660_SUSP_HEADER_SZ + 2)
+	{
+	  grub_free (sua);
+	  return grub_error (GRUB_ERR_BAD_FS, "corrupted rock ridge entry");
+	}
+
       data->susp_skip = entry->data[2];
       entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
 
-- 
2.35.1

Re: [PATCH 3/4] fs/iso9660: Avoid reading past the entry boundary

[Thomas Schmitt]

Hi,

On Wed, 14 Dec 2022 18:55:04 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> Added a check for the SP entry data boundary before reading it.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/iso9660.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 9170fa820..67aa8451c 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -408,6 +408,9 @@ set_rockridge (struct grub_iso9660_data *data)
>    if (!sua_size)
>      return GRUB_ERR_NONE;
>
> +  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)

As mentioned in my review of patch [2/4], GRUB_ISO9660_SUSP_HEADER_SZ should
be defined as 4, rather than as 5. Else entry RE could trigger this error:

> +    return grub_error (GRUB_ERR_BAD_FS, "invalid rock ridge entry size");
> +
>    sua = grub_malloc (sua_size);
>    if (! sua)
>      return grub_errno;
> @@ -434,8 +437,17 @@ set_rockridge (struct grub_iso9660_data *data)
>        rootnode.have_symlink = 0;
>        rootnode.dirents[0] = data->voldesc.rootdir;
>
> -      /* The 2nd data byte stored how many bytes are skipped every time
> -	 to get to the SUA (System Usage Area).  */
> +      /*
> +       * The 2nd data byte stored how many bytes are skipped every time
> +       * to get to the SUA (System Usage Area).
> +       */
> +      if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ + 2 ||
> +	  entry->len < GRUB_ISO9660_SUSP_HEADER_SZ + 2)

This interprets an SP entry, which is specified to have 7 bytes.
So if GRUB_ISO9660_SUSP_HEADER_SZ gets corrected to 4, then the size demand
will have to be (GRUB_ISO9660_SUSP_HEADER_SZ + 3).

Like with the NM interpretation i would rather prefer a plain "7", maybe with
a comment which says that this is the fixed size of SP (version 1).


> +	{
> +	  grub_free (sua);
> +	  return grub_error (GRUB_ERR_BAD_FS, "corrupted rock ridge entry");
> +	}
> +
>        data->susp_skip = entry->data[2];
>        entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
>
> --
> 2.35.1
>

Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>

But the expression (GRUB_ISO9660_SUSP_HEADER_SZ + 2) will need correction if
my wish for
  #define GRUB_ISO9660_SUSP_HEADER_SZ 4
gets fulfilled. As said, i'd prefer a plain "7".


Have a nice day :)

Thomas

Re: [PATCH 3/4] fs/iso9660: Avoid reading past the entry boundary

[Lidong Chen]

> On Dec 15, 2022, at 10:08 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> On Wed, 14 Dec 2022 18:55:04 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
>> Added a check for the SP entry data boundary before reading it.
>> 
>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>> ---
>> grub-core/fs/iso9660.c | 16 ++++++++++++++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index 9170fa820..67aa8451c 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -408,6 +408,9 @@ set_rockridge (struct grub_iso9660_data *data)
>>   if (!sua_size)
>>     return GRUB_ERR_NONE;
>> 
>> +  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> 
> As mentioned in my review of patch [2/4], GRUB_ISO9660_SUSP_HEADER_SZ should
> be defined as 4, rather than as 5. Else entry RE could trigger this error:
> 
>> +    return grub_error (GRUB_ERR_BAD_FS, "invalid rock ridge entry size");
>> +
>>   sua = grub_malloc (sua_size);
>>   if (! sua)
>>     return grub_errno;
>> @@ -434,8 +437,17 @@ set_rockridge (struct grub_iso9660_data *data)
>>       rootnode.have_symlink = 0;
>>       rootnode.dirents[0] = data->voldesc.rootdir;
>> 
>> -      /* The 2nd data byte stored how many bytes are skipped every time
>> -	 to get to the SUA (System Usage Area).  */
>> +      /*
>> +       * The 2nd data byte stored how many bytes are skipped every time
>> +       * to get to the SUA (System Usage Area).
>> +       */
>> +      if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ + 2 ||
>> +	  entry->len < GRUB_ISO9660_SUSP_HEADER_SZ + 2)
> 
> This interprets an SP entry, which is specified to have 7 bytes.
> So if GRUB_ISO9660_SUSP_HEADER_SZ gets corrected to 4, then the size demand
> will have to be (GRUB_ISO9660_SUSP_HEADER_SZ + 3).
> 
> Like with the NM interpretation i would rather prefer a plain "7", maybe with
> a comment which says that this is the fixed size of SP (version 1).
> 
> 
>> +	{
>> +	  grub_free (sua);
>> +	  return grub_error (GRUB_ERR_BAD_FS, "corrupted rock ridge entry");
>> +	}
>> +
>>       data->susp_skip = entry->data[2];
>>       entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
>> 
>> --
>> 2.35.1
>> 
> 
> Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
> 
> But the expression (GRUB_ISO9660_SUSP_HEADER_SZ + 2) will need correction if
> my wish for
>  #define GRUB_ISO9660_SUSP_HEADER_SZ 4
> gets fulfilled. As said, i'd prefer a plain "7".
> 

Ok, I will change it to a plain ‘7’ and add a comment for it.

Thanks,
Lidong

> 
> Have a nice day :)
> 
> Thomas
> 

[PATCH 4/4] fs/iso9660: Incorrect check for entry boudary

[Lidong Chen]

An entry consists of the entry info and the component area.
The entry info should take up 5 bytes instead of sizeof (*entry).
The area after the first 5 bytes is the component area. The code
uses the sizeof (*entry) to check the boundary which is incorrect.
Also, an entry may not have component record. Added a check for
for the component length before reading the component record.

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

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 67aa8451c..af432ee82 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -662,10 +662,22 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
   else if (grub_strncmp ("SL", (char *) entry->sig, 2) == 0)
     {
       unsigned int pos = 1;
+      unsigned int csize;
 
-      /* The symlink is not stored as a POSIX symlink, translate it.  */
-      while (pos + sizeof (*entry) < entry->len)
+      /* The symlink is not stored as a POSIX symlink, translate it. */
+      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)
 	{
+	  /*
+	   * entry->len is GRUB_ISO9660_SUSP_HEADER_SZ plus the
+	   * length of the 'Component Record'. The length of the
+	   * record is 2 (pos and pos + 1) plus the actual record
+	   * starting at pos + 2. pos stores the 'Component Flags',
+	   * pos + 1 specifies the length of actual record.
+	   */
+          csize = entry->data[pos + 1] + 2;
+          if (csize + GRUB_ISO9660_SUSP_HEADER_SZ > entry->len)
+            break;
+
 	  /* The current position is the `Component Flag'.  */
 	  switch (entry->data[pos] & 30)
 	    {
@@ -681,8 +693,11 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
 		      return grub_errno;
 		  }
 
-		add_part (ctx, (char *) &entry->data[pos + 2],
-			  entry->data[pos + 1]);
+		if (entry->data[pos + 1] > 0)
+		  {
+		    add_part (ctx, (char *) &entry->data[pos + 2],
+			      entry->data[pos + 1]);
+		  }
 		ctx->was_continue = (entry->data[pos] & 1);
 		break;
 	      }
-- 
2.35.1

Re: [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary

[Thomas Schmitt]

Hi,

On Wed, 14 Dec 2022 18:55:05 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary

s/boudary/boundary/

> An entry consists of the entry info and the component area.

Component area is specific to the RRIP SL entry. So:

s/An entry/An SL entry/


> The entry info should take up 5 bytes instead of sizeof (*entry).
> The area after the first 5 bytes is the component area. The code
> uses the sizeof (*entry) to check the boundary which is incorrect.
> Also, an entry may not have component record. Added a check for
> for the component length before reading the component record.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/iso9660.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 67aa8451c..af432ee82 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -662,10 +662,22 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>    else if (grub_strncmp ("SL", (char *) entry->sig, 2) == 0)
>      {
>        unsigned int pos = 1;
> +      unsigned int csize;
>
> -      /* The symlink is not stored as a POSIX symlink, translate it.  */
> -      while (pos + sizeof (*entry) < entry->len)
> +      /* The symlink is not stored as a POSIX symlink, translate it. */
> +      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)

This loop is iterating over component records, not SUSP entries.
Minimum size of a component record is 2.

So i think the appropriate condition is:

         while (pos + 1 < entry->len)

Component records have no struct representation in GRUB. So no sizeof will
tell the correct value.

C-wise decisive is this line:

                add_part (ctx, (char *) &entry->data[pos + 2],
                          entry->data[pos + 1]);

which lower in this patch changes to

   		if (entry->data[pos + 1] > 0)
   		  {
   		    add_part (ctx, (char *) &entry->data[pos + 2],
   			      entry->data[pos + 1]);
   		  }

This change will alter the program behavior in respect to empty link
target path components.

The validity of entry->data[pos + 1] is guaranteed by my test proposal.
If entry->data[pos + 1] is 0, then &entry->data[pos + 2] can be an illegal
address, but 0 bytes from that address will be requested to be added.
(A 0-byte will be added by add_part() as separator between link target path
components.)

I am undecided whether add_part() should be skipped if
  (pos + 2 == entry->len)
which indicates an empty path component.
If add_part() shall be performed with length 0, then we should skip in it
the call of
  grub_memcpy (ctx->symlink + size, part, len2);
in case of (len2 == 0).


>  	{
> +	  /*
> +	   * entry->len is GRUB_ISO9660_SUSP_HEADER_SZ plus the
> +	   * length of the 'Component Record'. The length of the

With GRUB_ISO9660_SUSP_HEADER_SZ == 4 it will be

  GRUB_ISO9660_SUSP_HEADER_SZ + 1 + length of the Component Area.

The "+ 1" stands for the "FLAGS" byte. (RRIP-1.12, 4.1.3)
It should be 'Component Area' or plural 'Component Records' because
'Component Record' is a single path component.


> +	   * record is 2 (pos and pos + 1) plus the actual record
> +	   * starting at pos + 2. pos stores the 'Component Flags',
> +	   * pos + 1 specifies the length of actual record.
> +	   */

This describes a single component record, of which there are as many as
needed to represent the link target path.

entry->data[pos + 1] gives the length of the component, not of the component
record, of which the length is entry->data[pos + 1] + 2.


> +          csize = entry->data[pos + 1] + 2;
> +          if (csize + GRUB_ISO9660_SUSP_HEADER_SZ > entry->len)

With GRUB_ISO9660_SUSP_HEADER_SZ == 4 it will be

             if (csize + GRUB_ISO9660_SUSP_HEADER_SZ + 1 > entry->len)


> +            break;
> +
>  	  /* The current position is the `Component Flag'.  */
>  	  switch (entry->data[pos] & 30)
>  	    {
> @@ -681,8 +693,11 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>  		      return grub_errno;
>  		  }
>
> -		add_part (ctx, (char *) &entry->data[pos + 2],
> -			  entry->data[pos + 1]);
> +		if (entry->data[pos + 1] > 0)
> +		  {
> +		    add_part (ctx, (char *) &entry->data[pos + 2],
> +			      entry->data[pos + 1]);
> +		  }

As written above, this changes program behavior if a link target path
contains an empty component.


>  		ctx->was_continue = (entry->data[pos] & 1);
>  		break;
>  	      }
> --
> 2.35.1
>

So no "Reviewed-by:" yet.

Open issues:

The commit message should mention that it is about SL and not about general
SUSP or RRIP entries.

The while-condition of the component loop should neither use sizeof (*entry)
nor GRUB_ISO9660_SUSP_HEADER_SZ, but plain 1, because that's the minimum
size of an SL component record minus 1.
Probably this should be a separate bug-fix patch to be applied before this
patch [4/4].

Remaining mentioning of GRUB_ISO9660_SUSP_HEADER_SZ needs to become
  GRUB_ISO9660_SUSP_HEADER_SZ + 1
when/if GRUB_ISO9660_SUSP_HEADER_SZ gets defined to 4.

The change of program behavior by ignoring empty link target path components
should be mentioned in the commit message.

-------------------------------------------------------------------------

... whew. All four reviews done.
Probably i made some mistakes in them. Please check all my statements
before basing code changes on them. Inform me if you find such mistakes.

Now i'm looking forward to the next round of review, decisions by Daniel
Kiper, and comments from bystanders.


Have a nice day :)

Thomas

Re: [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary

[Lidong Chen]

> On Dec 15, 2022, at 10:20 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> On Wed, 14 Dec 2022 18:55:05 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
>> [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary
> 
> s/boudary/boundary/
> 
>> An entry consists of the entry info and the component area.
> 
> Component area is specific to the RRIP SL entry. So:
> 
> s/An entry/An SL entry/
> 
> 
>> The entry info should take up 5 bytes instead of sizeof (*entry).
>> The area after the first 5 bytes is the component area. The code
>> uses the sizeof (*entry) to check the boundary which is incorrect.
>> Also, an entry may not have component record. Added a check for
>> for the component length before reading the component record.
>> 
>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>> ---
>> grub-core/fs/iso9660.c | 23 +++++++++++++++++++----
>> 1 file changed, 19 insertions(+), 4 deletions(-)
>> 
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index 67aa8451c..af432ee82 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -662,10 +662,22 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>>   else if (grub_strncmp ("SL", (char *) entry->sig, 2) == 0)
>>     {
>>       unsigned int pos = 1;
>> +      unsigned int csize;
>> 
>> -      /* The symlink is not stored as a POSIX symlink, translate it.  */
>> -      while (pos + sizeof (*entry) < entry->len)
>> +      /* The symlink is not stored as a POSIX symlink, translate it. */
>> +      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)
> 
> This loop is iterating over component records, not SUSP entries.
> Minimum size of a component record is 2.
> 
> So i think the appropriate condition is:
> 
>         while (pos + 1 < entry->len)

My understanding is the entry->len is the size of the System Use Field. If that is
correct,  the while-loop condition is to make sure the component records are
within the System Use Field boundary.  “Pos” was initially set to ‘1’ and incremented
by the size of each component record.  So, “Pos” is relative to the Component Area, 
not the System Use Field of “SL”.  I think the fix should include the 5 bytes:

    While (pos + 5 < entry->len)



> 
> Component records have no struct representation in GRUB. So no sizeof will
> tell the correct value.
> 
> C-wise decisive is this line:
> 
>                add_part (ctx, (char *) &entry->data[pos + 2],
>                          entry->data[pos + 1]);
> 
> which lower in this patch changes to
> 
>   		if (entry->data[pos + 1] > 0)
>   		  {
>   		    add_part (ctx, (char *) &entry->data[pos + 2],
>   			      entry->data[pos + 1]);
>   		  }
> 
> This change will alter the program behavior in respect to empty link
> target path components.

My mistake. I will restore the original code. 
> 
> The validity of entry->data[pos + 1] is guaranteed by my test proposal.
> If entry->data[pos + 1] is 0, then &entry->data[pos + 2] can be an illegal
> address, but 0 bytes from that address will be requested to be added.
> (A 0-byte will be added by add_part() as separator between link target path
> components.)
> 
> I am undecided whether add_part() should be skipped if
>  (pos + 2 == entry->len)
Do you mean this within the while-loop?:

    If (pos + 2 == entry->len)
       continue

> which indicates an empty path component.
> If add_part() shall be performed with length 0, then we should skip in it
> the call of
>  grub_memcpy (ctx->symlink + size, part, len2);
> in case of (len2 == 0).
> 
> 
>> 	{
>> +	  /*
>> +	   * entry->len is GRUB_ISO9660_SUSP_HEADER_SZ plus the
>> +	   * length of the 'Component Record'. The length of the
> 
> With GRUB_ISO9660_SUSP_HEADER_SZ == 4 it will be
> 
>  GRUB_ISO9660_SUSP_HEADER_SZ + 1 + length of the Component Area.
> 
> The "+ 1" stands for the "FLAGS" byte. (RRIP-1.12, 4.1.3)
> It should be 'Component Area' or plural 'Component Records' because
> 'Component Record' is a single path component.
> 
> 
>> +	   * record is 2 (pos and pos + 1) plus the actual record
>> +	   * starting at pos + 2. pos stores the 'Component Flags',
>> +	   * pos + 1 specifies the length of actual record.
>> +	   */
> 
> This describes a single component record, of which there are as many as
> needed to represent the link target path.
> 
> entry->data[pos + 1] gives the length of the component, not of the component
> record, of which the length is entry->data[pos + 1] + 2.
> 
> 
>> +          csize = entry->data[pos + 1] + 2;
>> +          if (csize + GRUB_ISO9660_SUSP_HEADER_SZ > entry->len)
> 
> With GRUB_ISO9660_SUSP_HEADER_SZ == 4 it will be
> 
>             if (csize + GRUB_ISO9660_SUSP_HEADER_SZ + 1 > entry->len)

I will add plain ‘5’ and add the comment for it. 
> 
> 
>> +            break;
>> +
>> 	  /* The current position is the `Component Flag'.  */
>> 	  switch (entry->data[pos] & 30)
>> 	    {
>> @@ -681,8 +693,11 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>> 		      return grub_errno;
>> 		  }
>> 
>> -		add_part (ctx, (char *) &entry->data[pos + 2],
>> -			  entry->data[pos + 1]);
>> +		if (entry->data[pos + 1] > 0)
>> +		  {
>> +		    add_part (ctx, (char *) &entry->data[pos + 2],
>> +			      entry->data[pos + 1]);
>> +		  }
> 
> As written above, this changes program behavior if a link target path
> contains an empty component.
> 
> 
>> 		ctx->was_continue = (entry->data[pos] & 1);
>> 		break;
>> 	      }
>> --
>> 2.35.1
>> 
> 
> So no "Reviewed-by:" yet.
> 
> Open issues:
> 
> The commit message should mention that it is about SL and not about general
> SUSP or RRIP entries.
I will fix it.

> 
> The while-condition of the component loop should neither use sizeof (*entry)
> nor GRUB_ISO9660_SUSP_HEADER_SZ, but plain 1, because that's the minimum
> size of an SL component record minus 1.
Please see my response above.

> Probably this should be a separate bug-fix patch to be applied before this
> patch [4/4].
> 

> Remaining mentioning of GRUB_ISO9660_SUSP_HEADER_SZ needs to become
>  GRUB_ISO9660_SUSP_HEADER_SZ + 1
> when/if GRUB_ISO9660_SUSP_HEADER_SZ gets defined to 4.
> 
> The change of program behavior by ignoring empty link target path components
> should be mentioned in the commit message.
> 
> -------------------------------------------------------------------------
> 
> ... whew. All four reviews done.
> Probably i made some mistakes in them. Please check all my statements
> before basing code changes on them. Inform me if you find such mistakes.
> 
> Now i'm looking forward to the next round of review, decisions by Daniel
> Kiper, and comments from bystanders.
> 
Thank you for taking the time to explain and review!  Your explanation and comments
 are very helpful, appreciated it. 

Li

> 
> Have a nice day :)
> 
> Thomas

> 

Re: [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary

[Thomas Schmitt]

Hi,

in summary: Your objections to my objections are correct.


On Wed, 14 Dec 2022 18:55:05 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> >> +      /* The symlink is not stored as a POSIX symlink, translate it. */
> >> +      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)

On Dec 15, 2022, at 10:20 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> > This loop is iterating over component records, not SUSP entries.
> > Minimum size of a component record is 2.
> >
> > So i think the appropriate condition is:
> >
> >         while (pos + 1 < entry->len)

On Mon, 19 Dec 2022 21:00:23 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> My understanding is the entry->len is the size of the System Use Field.
> the while-loop condition is to make sure the component records are
> within the System Use Field boundary.
> “Pos” was initially set to ‘1’ and
>  incremented by the size of each component record.  So, “Pos” is relative
> to the Component Area, not the System Use Field of “SL”.
> I think the fix should include the 5 bytes:
>
>     While (pos + 5 < entry->len)

You are right.

My "+ 1" would be correct if entry->len was the size of the SL entry
payload (FLAGS byte and COMPONENT AREA). But further "+ 4" are needed
because of the SUSP header size of the SL entry.

(One could use "+ GRUB_ISO9660_SUSP_HEADER_SZ" instead of "+ 4" in this
 case. But at other places you followed my proposal to refer with plain
 numbers to the description in the RRIP specs. So in the sum "+ 5" is
 good.)


> > I am undecided whether add_part() should be skipped if
> >  (pos + 2 == entry->len)

> Do you mean this within the while-loop?:
>
>    If (pos + 2 == entry->len)
>       continue

I meant your change of program behavior:

-               add_part (ctx, (char *) &entry->data[pos + 2],
-                         entry->data[pos + 1]);
+               if (entry->data[pos + 1] > 0)
+                 {
+                   add_part (ctx, (char *) &entry->data[pos + 2],
+                             entry->data[pos + 1]);
+                 }

I obviously made some inappropriate thought jumps when writing
  (pos + 2 == entry->len)
Apologies for being confusing.

(It's not only about the end of the SL entry but about the size of any
component record, regardless of its position in the entry's payload.
Aggravating is that the condition i mentioned is wrong by the missing
"+ 4" for the SUSP header, even if it was about the SL entry's end.)

Whatever, you already decided not to change program behavior by
skipping over empty text components.


> > Open issues:
> > [...]
> > The while-condition of the component loop should neither use sizeof (*entry)
> > nor GRUB_ISO9660_SUSP_HEADER_SZ, but plain 1, because that's the minimum
> > size of an SL component record minus 1.
> > Probably this should be a separate bug-fix patch to be applied before this
> > patch [4/4].

So i now retract this issue.

> > The change of program behavior by ignoring empty link target path components
> > should be mentioned in the commit message.

This issue is now obsolete.


Have a nice day :)

Thomas

Re: [PATCH 0/4] fs/iso9660: Fix out-of-bounds read

[Thomas Schmitt]

Hi,

i will review the patches hopefully tomorrow.

But in [PATCH 2/4] and [4/4] i stumble over the statement that a SUSP
entry has 5 bytes of size.  This is not true. The minimum size is 4.
In SUSP there are "Padding" PD (if its byte LEN_PD is 0) and "Stop" ST,
which have 4 bytes. In RRIP there is "Relocated" RE.
Other SUSP-compliant protocols could define 4-byte entries, too.

I will have to analyze the patches whether the assumption of 5 bytes
minimum can cause real harm.

But i see at least two inappropriate applications of the minimum size:

In [2/4] a RRIP NM entry is processed:
  -         csize = entry->len - 5;
  +         csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;
The number 5 is meant to skip over the 4 bytes of SUSP header and the one
byte of "FLAGS" to reach to the "NAME CONTENT" bytes. This is specific to
NM (version 1, to be exacting), not to SUSP in general.
I propose to leave the 5 as is.

In [4/4] a RRIP SL entry is processed:
-      /* The symlink is not stored as a POSIX symlink, translate it.  */
-      while (pos + sizeof (*entry) < entry->len)
+      /* The symlink is not stored as a POSIX symlink, translate it. */
+      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)
At least in a quick test in GNU/Linux userspace
  struct grub_iso9660_susp_entry {
    uint8_t sig[2];
    uint8_t len;
    uint8_t version;
    uint8_t data[0];
  };
has sizeof 4, not 5.
So i see the risk that this change alters program behavior in situations
where we don't perceive a problem yet.

It is too late in the evening to analyze what sizeof(*entry) has to do
with reading the component records of SL. The component records are the
components of the link target path with 2 bytes header plus the name
bytes. So a size of 3 is plausible like in .../b/... Even a size of 2
is not totally illegal, as Linux allows paths like ...//....


Have a nice day :)

Thomas

Re: [PATCH 0/4] fs/iso9660: Fix out-of-bounds read

[Lidong Chen]

> On Dec 14, 2022, at 1:42 PM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> i will review the patches hopefully tomorrow.
> 
> But in [PATCH 2/4] and [4/4] i stumble over the statement that a SUSP
> entry has 5 bytes of size.  This is not true. The minimum size is 4.
> In SUSP there are "Padding" PD (if its byte LEN_PD is 0) and "Stop" ST,
> which have 4 bytes. In RRIP there is "Relocated" RE.
> Other SUSP-compliant protocols could define 4-byte entries, too.

Thank you for the clarification about the SUSP entry size. I was confused with ’NM’ entry.
I will fix it.

> 
> I will have to analyze the patches whether the assumption of 5 bytes
> minimum can cause real harm.
> 
> But i see at least two inappropriate applications of the minimum size:
> 
> In [2/4] a RRIP NM entry is processed:
>  -         csize = entry->len - 5;
>  +         csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;
> The number 5 is meant to skip over the 4 bytes of SUSP header and the one
> byte of "FLAGS" to reach to the "NAME CONTENT" bytes. This is specific to
> NM (version 1, to be exacting), not to SUSP in general.
> I propose to leave the 5 as is.

I will revert this change and restore the plain 5 as it is.

Thanks,
Lidong

> 
> In [4/4] a RRIP SL entry is processed:
> -      /* The symlink is not stored as a POSIX symlink, translate it.  */
> -      while (pos + sizeof (*entry) < entry->len)
> +      /* The symlink is not stored as a POSIX symlink, translate it. */
> +      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)
> At least in a quick test in GNU/Linux userspace
>  struct grub_iso9660_susp_entry {
>    uint8_t sig[2];
>    uint8_t len;
>    uint8_t version;
>    uint8_t data[0];
>  };
> has sizeof 4, not 5.
> So i see the risk that this change alters program behavior in situations
> where we don't perceive a problem yet.
> 
> It is too late in the evening to analyze what sizeof(*entry) has to do
> with reading the component records of SL. The component records are the
> components of the link target path with 2 bytes header plus the name
> bytes. So a size of 3 is plausible like in .../b/... Even a size of 2
> is not totally illegal, as Linux allows paths like ...//....
> 
> 
> Have a nice day :)
> 
> Thomas
>