grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel Kiper
Subject: Re: Possible memory fault in fs/iso9660 (correction)
Date: Tue, 29 Nov 2022 15:26:07 +0100
User-agent: NeoMutt/20170113 (1.7.2)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]