Re: Possible memory fault in fs/iso9660 (correction)

[Fengtao (fengtao, Euler)]

Hi, Thomas
Sorry for the delay, I am also not familiar with ISO format.
But, i have check the cdrkit src-code[1] and syslinux src-code[2]

I think:
        (char *) entry < (char *) sua + sua_size - 3 && entry->len > 0
is ok, or:
        (char *) entry <= (char *) sua + sua_size - 4 && entry->len > 0


[1]:https://github.com/Distrotech/cdrkit/blob/distrortech-cdrkit/genisoimage/diag/isoinfo.c#L373
[2]:https://github.com/geneC/syslinux/blob/master/core/fs/iso9660/susp_rr.c#L145

On 2022/11/24 23:16, Thomas Schmitt wrote:
> Hi,
> 
> (Again i Cc t.feng in the hope that the review is not finished yet. :))
> 
> Daniel Kiper wrote:
>> I am not an ISO format expert but your thinking LGTM.
> 
> So you agree that "3" is really the right number if any remaining bytes
> fewer than 4 shall be ignored ?
> (I don't trust myself, although i made an example with finger counting.)
> 
> 
>> could you send a patch fixing this issue?
> 
> This should be possible. But how to test ?
> 
> Normal ISOs made with GNU/Linux will cause (entry == sua + sua_size) at
> the end of the SUSP data. So provoking the problem and checking whether
> it is solved will need a hacked ISO.
> I will think about creating such an ISO by help of xorriso and dd.
> 
> 
> While exploring the SUSP/RRIP entry types which are interpreted by GRUB,
> i got to more credulence towards the ISO producer.
> E.g. in grub-core/fs/iso9660.c line 404 ff.
> 
>       /* The 2nd data byte stored how many bytes are skipped every time
>        to get to the SUA (System Usage Area).  */
>       data->susp_skip = entry->data[2];
> 
> This is a memory fault if (sua_size < 7). I see no check between
>   sua = grub_malloc (sua_size);
> and the read access to entry->data[2].
> 
> Another example:
> grub_iso9660_susp_iterate() calls its parameter hook() without checking
> that the alleged entry size is within the range of sua_size. The hook()
> function does not get sua_size to check on its own.
> 
> In general:
> How mistrusting should GRUB be towards the bytes in the filesystem ?
> 
> 
> Have a nice day :)
> 
> Thomas
> 
> .
>