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

["Thomas Schmitt" <scdbackup@gmx.net>]

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