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

[Lidong Chen]

On Jan 19, 2023, at 3:58 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:

Hi,

i wrote:

libisofs and xorriso are in the process of getting an adjustable curb to
prevent the production of ISO filesystems with files which would not show
up in Linux. I decided for 100,000 hops as hard limit but set the default
to 31.

Lidong Chen wrote:

I am not sure I understand the hard limit vs the default in terms of
checking the number of hops.

xorriso produces and reads ISO 9660 filesystems.

At production time, the limit will be adjustable. The default will be 31
to prevent files which don't show up when the filesystem is mounted by
Linux. The maximum adjustable limit at production time will be 100,000.

At read time, e.g. for extracting files or for adding another session to
the ISO, the limit will be 100,000.

Since the limit of CE hops has been decided,

I meanwhile deem the proposed 1 million allowed hops quite high.
An opinion by experienced GRUB developers would be helpful.

if the previous proposed fix still stands, I can create a patch for it.

The fix by hop counting is the right thing to do.

The tough part for me is the testing.

You can use for testing
 https://urldefense.com/v3/__http://scdbackup.webframe.org/ce_loop.iso.gz__;!!ACWV5N9M2RV99hQ!OXku0fFl7fyFeTon9H7cdACNDHk8OrsvozKrR5iTCzbjwS90Ys6gZI8kpfDlpAgRKh2q3YKxK7ROWT5K7cg$
 SHA256: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2
and
 https://urldefense.com/v3/__http://scdbackup.webframe.org/ce_loop2.iso.gz__;!!ACWV5N9M2RV99hQ!OXku0fFl7fyFeTon9H7cdACNDHk8OrsvozKrR5iTCzbjwS90Ys6gZI8kpfDlpAgRKh2q3YKxK7RO7e3E4Lw$
 SHA256: a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c

ce_loop.iso currently causes no endless loop in grub-fstest, because
the CE entry at the start of the (bad) continuation area is ignored,
against the prescriptions of SUSP.
It will cause an endless loop after patch 5/5 is applied and the
self-pointing CE entry is not ignored any more by mistake.
 ./grub-fstest ce_loop.iso ls /

ce_loop2.iso already now causes an endless loop with
 ./grub-fstest ce_loop2.iso ls /

Both endless loops should be detected and cause a GRUB error when the
CE hop counter and loop breaker is in effect.

I ran grub-fastest with both ce_loop ISO files. The endless loops were detected

and Grub exited accordingly. I didn't know where the grub error message

were stored in case of grub-fastest. But, I traced with gdb, and saw the 

code reported the error. If the diff looks good, I will send the v3 patches set.

+#define GRUB_ISO9660_MAX_CE_HOPS       100000

 

   struct grub_iso9660_susp_entry *entry;

   grub_err_t err;

+  int ce_counter = 0;

 

          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;

          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

+             || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)

+           continue;

        }

 

       if (hook (entry, hook_arg))

Thanks,

Lidong

(I can meanwhile provide ISOs which have 32+ CE hops without loop, i.e.
righteously storing 64+ KiB of data in the chain of SUSP entries of a
file. But that's mainly interesting for testing Linux, not for GRUB.)


Have a nice day :)

Thomas