On Tue, Nov 29, 2022 at 05:32:56PM +0800, Fengtao (fengtao, Euler) via Grub-devel wrote: > 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
I think you are right... > 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. Yeah, that would be perfect... > > 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. Huh! Could you fix these issues too? > > In general: > > How mistrusting should GRUB be towards the bytes in the filesystem ? I think as little as possible. Especially if incorrect values may lead to OOB writes... Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
