Hi Thomas, > On Jan 6, 2023, at 8:00 AM, Thomas Schmitt <scdbac...@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 > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel