Fwd: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items
---------- 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.
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
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
participants (2)
-
Jincheng Wang -
Miquel Raynal