Re: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Jincheng,

jc.w4ng@gmail.com wrote on Sun, 31 Oct 2021 18:24:47 +0800:

> ---------- Forwarded message ---------
> 发件人： Jincheng Wang <jc.w4ng@gmail.com>
> Date: 2021年10月31日周日 下午6:23
> Subject: Re: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list
> instead of free items
> To: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> 
> Hello,
> 
> Apologize for a late reply ,and I made a mistake in wirting mail.
> 
> Here is the email before I commit the patch.
> 
> Double free vulnerability in sqfs commands ("sqfsls" and "sqfsload")
> 
> On Tue, Oct 12, 2021 at 04:07:43PM +0800, Jincheng Wang wrote:
> 
> > Hello U-Boot lists!
> > I had been doing a fuzz testing in U-Boot .
> > There is a double free bug in U-Boot, in /fs/squashfs/sqfs.c in the
> > function sqfs_tokenize( ).
> > On the line 307, tokens[i] is being freed and the ret is being set  
> -ENOMEM,
> > it will go to the out: label and free tokens[i] again (just like
> > CVE-2020-8432, double free in do_rename_gpt_parts() ).
> > Here is  a sample command cause to crash in sandbox environment:
> >     host bind 0 test.sqsh
> >     sqfsls host 0 1//3  
> 
> ------------------------------
> 
> The vulnerability can be fixed with a new return value and a new label. But
> I didn't want to creat  these, I chose another way.

You need to answer my questions below if you don't want to propose
another solution. But creating a new goto label is perhaps the good way
to solve this, provided that there is actually a leak (I am waiting for
the explanation of how it happens).

Cheers,
Miquèl

> 
> Regards,
> Jincheng
> 
> 
> Miquel Raynal <miquel.raynal@bootlin.com> 于2021年10月27日周三 下午4:17写道：
> 
> > Hello,
> >
> > trini@konsulko.com wrote on Tue, 26 Oct 2021 15:25:56 -0400:
> >  
> > > On Sat, Oct 16, 2021 at 10:19:48AM +0800, Jincheng Wang wrote:
> > >  
> > > > We can delete two lines of code to avoid double free bug, but still a  
> > wild  
> > > > pointers bug.
> > > >
> > > > A test for wild pointers:
> > > >     sqfsls host 0  1//2/3//4/5
> > > >
> > > > Fill the tokens list can solve it well.
> > > >
> > > >
> > > > Signed-off-by: Jincheng Wang <jc.w4ng@gmail.com>
> > > > ---
> > > >  fs/squashfs/sqfs.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > >   }
> > > >
> > > > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> > > > index e2d91c654c..50d3f8b71e 100644
> > > > --- a/fs/squashfs/sqfs.c
> > > > +++ b/fs/squashfs/sqfs.c
> > > > @@ -303,8 +303,9 @@ static int sqfs_tokenize(char **tokens, int count,
> > > > const char *str)
> > > >   aux = strtok(!j ? strc : NULL, "/");
> > > >   tokens[j] = strdup(aux);
> > > >   if (!tokens[j]) {
> > > > - for (i = 0; i < j; i++)
> > > > - free(tokens[i]);  
> >
> > Where is the double free here? Why do you stop freeing the tokens list?
> >  
> > > > + /* fill tokens list to avoid wild pointers being freed*/
> > > > + for (i = j + 1; i < count; i++)
> > > > + tokens[i] = 0;  
> >
> > This does not feel right. Perhaps it's just me not getting through the
> > lack of spacing successfully but this deserves more explanations.
> >  
> > > >   ret = -ENOMEM;
> > > >   goto free_strc;  
> > >
> > > Aside from the whitespace having been destroyed, any comments from the
> > > maintainers / reviewers?  Thanks!
> > >  
> >
> > Thanks,
> > Miquèl
> >