> On Dec 15, 2022, at 10:00 AM, Thomas Schmitt <scdbac...@gmx.net> wrote:
>
> Hi,
>
> On Wed, 14 Dec 2022 18:55:03 +0000 Lidong Chen <lidong.c...@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.c...@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
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
