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
>