Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area

[Thomas Schmitt]

Hi,

On Wed, 14 Dec 2022 18:55:03 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> In the code, the for loop advanced the entry pointer to the
> next entry before checking if the next entry is within the
> system use area boundary. Another issue in the code was that
> there is no check for the size of system use area. For a
> corrupted system, the size of system use area can be less than
> the size of SUSP entry size (5 bytes).

The minimum size of a SUSP entry is 4, not 5. Examples of 4-byte entries
are ST in SUSP and RE in RRIP. (Ending before ST would be harmless, because
ST marks the end of the SUSP entry chain. But RE may appear before the end.)


> These can cause buffer
> overrun. The fixes added the checks to ensure the read is
> valid and within the boundary.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/iso9660.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 4f4cd6165..9170fa820 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define GRUB_ISO9660_VOLDESC_PART    3
>  #define GRUB_ISO9660_VOLDESC_END     255
>
> +#define GRUB_ISO9660_SUSP_HEADER_SZ  5

So i think this should be 4, not 5.
If we find reason why it should be 5, then the name SUSP_HEADER_SZ is not
appropriate. The SUSP header size is surely 4.


> +
>  /* The head of a volume descriptor.  */
>  struct grub_iso9660_voldesc
>  {
> @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>    if (sua_size <= 0)
>      return GRUB_ERR_NONE;
>
> +  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> +    return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
> +

Here we need 4.


>    sua = grub_malloc (sua_size);
>    if (!sua)
>      return grub_errno;
> @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>    if (err)
>      return err;
>
> -  for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry < 
> (char *) sua + sua_size - 1 && entry->len > 0;
> -       entry = (struct grub_iso9660_susp_entry *)
> -      ((char *) entry + entry->len))
> +  entry = (struct grub_iso9660_susp_entry *) sua;
> +
> +  while (entry->len > 0)
>      {
> +      /* Ensure the entry is within System Use Area */
> +      if ((char *) entry + entry->len > (sua + sua_size))
> +        break;
> +
>        /* The last entry.  */
>        if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
>       break;
> @@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>         off = grub_le_to_cpu32 (ce->off);
>         ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
>
> +       if (sua_size <= 0)
> +         break;
> +
> +       if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)

4 would be appropriate here.


> +         {
> +           grub_free (sua);
> +           return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");
> +         }
> +
>         grub_free (sua);
>         sua = grub_malloc (sua_size);
>         if (!sua)
> @@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>         grub_free (sua);
>         return 0;
>       }
> +
> +      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + 
> entry->len);

This is equivalent to the third statement in the "for" which was
replaced by a while-loop. So far so good.

But i believe to see an old bug. This advancing of entry breaks the
chain of CE if the first entry of the Continuation Area is again a CE.

          err = grub_disk_read (node->data->disk, ce_block, off,
                                sua_size, sua);
          ...
          entry = (struct grub_iso9660_susp_entry *) sua;
        }

      if (hook (entry, hook_arg))
        {
          grub_free (sua);
          return 0;
        }

      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);

hook() will not interpret CE (and has no means to advance "entry").
entry = entry + entry->len; will then skip over the entry so that the loop
will not interpret it either.
The SUSP area will be perceived as having ended, although more SUSP entries
are present for the file in question.

I hereby ask for more opinions about this. Maybe i misinterpret the old loop.

Background:
CE points to the block and offset where the chain of SUSP entries goes on.
It is needed if the SUSP entry set exceeds the size limit of 255 bytes for
a directory record.
The byte at (ce->blk * block_size + ce->off) is the first byte of the next
SUSP entry in the chain.
Linux hates SUSP crossing block boundaries. So we ISO producers use further
CE entries to hop over block boundaries.

libisofs can produce a Continuation Area with first and only entry CE,
which then points to a new block with more SUSP payload. This happens if a
continuation block offers not much more than 28 free bytes, so that only
the 28 bytes of CE have room.


(GRUB is not really correct by performing the CE jump immediately when CE is
seen. The specs allow more SUSP entries to follow up to the end of the
directory record's SUSP range. Only after interpreting them, an encountered
CE should cause the jump to its Continuation Area. SUSP-1.12, 5.1:
  "The "CE" System Use Entry indicates a Continuation Area that shall be
   processed after the current System Use field or Continuation Area is
   processed."
mkisofs and libisofs both put their CE at the end of the directory record
of Continuation Area. So an exotic ISO would be needed to demonstrate a
problem with GRUB's immediate CE interpretation.)


> +
> +      if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
> +        break;

4 would be appropriate.


>      }
>
>    grub_free (sua);
> @@ -574,7 +597,7 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>         char *old;
>         grub_size_t sz;
>
> -       csize = entry->len - 5;
> +       csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;

Here it is not appropriate to use GRUB_ISO9660_SUSP_HEADER_SZ.
This code subtracts the count of the 4 header bytes of an NM entry and of
the 1 FLAGS byte of NM. Goal is to compute the size of the name string.
So if GRUB_ISO9660_SUSP_HEADER_SZ is defined as 4, then
(GRUB_ISO9660_SUSP_HEADER_SZ + 1) would be ok. But a plain 5 is ok, too,
because this is a specific property of the NM definition of version 1.

(Actually one should verify (entry->version == 1) before interpreting
an NM entry that way. Well, i see that libisofs doesn't check either.
Excuse is that the RRIP specs did not change since the mid 1990s.)

Whatever gets decided, the "5", which is six lines higher, should be changed
to the same expression:

      else if (entry->len >= 5)


>         old = ctx->filename;
>         if (ctx->filename_alloc)
>           {
> --
> 2.35.1
>

So no "Reviewed-by:" yet.

Open issues:

The definition of GRUB_ISO9660_SUSP_HEADER_SZ should be changed to 4.

Within the interpretation of NM this change should not happen:
> -       csize = entry->len - 5;
> +       csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;

We need to decide what to do with the potential old bug that a CE entry
as the first entry of a Continuation Area gets skipped without
interpretation.


Have a nice day :)

Thomas