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. */

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