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

[Lidong Chen]

> On Dec 15, 2022, at 10:00 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> 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.)
> 

I will fix it.

> 
>> 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.

You are right. SUSP-1.12 stated: 
“If the remaining allocated space following the last recorded System Use
 Entry in a System Use field or Continuation Area is less than 4 bytes long,
 It cannot contain a System Use Entry and should be ignored”

It implied the minimum SUSP Entry size is 4.  I will fix it.

> 
> 
>> +
>> /* 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.

Ok, I will revert the change and keep the plain ‘5’ as it is. 

> 
> (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.)

Ok, for that excuse, we don’t add the check for the version till it is needed 
then.

> 
> 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;

I will fix it.

Thanks,
Lidong

> 
> 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
>