Avoids leak in wasm details by catenacyber · Pull Request #1372 · capstone-engine/capstone

catenacyber · 2019-02-07T07:54:24Z

Extending cs_detail in capstone.h

Another way would have been to change the prototype of cs_free
This change seemed to me less disruptive for existing programs (no need to change the code using capstone, just a bit more memory consumption)

This was found by oss-fuzz :
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12952

(And this likely hides other bugs from fuzzing)

aquynh · 2019-02-07T08:45:06Z

this is nice, but i am wondering if we can fix it differently by having operands as a fixed size array, @Spikel?

catenacyber · 2019-02-07T08:51:43Z

We have some line
MI->flat_insn->detail->wasm.operands = cs_mem_malloc(sizeof(cs_wasm_op) * (length + 2))
which makes fixed size impossible
This is (as I understand) a jump table of a switch.
Maybe it would be best to retain only memory address and size of this jump table...

There seem to me to be other problems with this code (lack of check before allocating much arbitrary memory size)

aquynh · 2019-02-07T11:00:29Z

 		cs_wasm wasm;	///< Web Assembly architecture
 		cs_mos65xx mos65xx;	///< MOS65XX architecture (including MOS6502)
 	};
+	cs_arch arch; ///Used architecture (for architecture-specific free)


this does not look good, but sounds like we cannot avoid adding this field?

another solution is to have a new csh argument for cs_free()

Indeed.
As a recap, I see three solutions :

this patch (but more memory consumption)

changing cs_free prototype (but changing interface is painful for people using this project)

changing how WASM handles this "switch" jump table (but I am not sure if this relevant for WASM)

Which is the best ?

yes we try best to avoid breaking compatibility, but in this case i think that is still better than having the extra arch field in cs_insn. finally, it was a mistake not to have csh arg in every API. we may afford changing API in the version 5, which will have major update in all archs.

if there is upper limit for brtable, we can avoid this mess by having fixed size array for operands. lets see what @Spikel has to say when he is back.

catenacyber · 2019-02-14T19:40:32Z

So, oss-fuzz indeed found the bug for arbitrary size memory allocation :
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12952

So I pushed another commit adding a safety check...

aquynh · 2019-02-15T14:50:18Z

nice work! i think @Spikel will get back from holidays next week to look at this bug.

aquynh · 2019-02-15T14:52:24Z

oh now this PR mixes with an old commit, from arch/TMS320C64x/TMS320C64xMapping.c.
can you try to sync your fork, and see if that goes away?

catenacyber · 2019-02-15T16:34:22Z

I rebased to remove this other commit

aquynh · 2019-02-15T17:48:38Z

Another way to avoid breaking compatibility is to have a new API, lets say cs_free_ex(), which accepts csh as its first param, and promote it as future replacement for cs_free() in next few version.

catenacyber · 2019-02-16T09:18:21Z

Newest version changes how WASM handles this switch table, this seems better for #1381 as well

Extending cs_detail in capstone.h

This reverts commit 03f822b.

aquynh · 2019-02-18T04:20:36Z

so the main change is that operands[] now has upper limit of 2?

catenacyber · 2019-02-18T07:05:39Z

Yes :

no more allocs and frees : no need to change the API :-), no arbitrary size memory allocation
no overlong (>512) instruction printing

But the user has to read the variable uint32 targets himself...

SpikeI · 2019-02-20T02:32:43Z

Yes :

no more allocs and frees : no need to change the API :-), no arbitrary size memory allocation

no overlong (>512) instruction printing

But the user has to read the variable uint32 targets himself...

I want to try another solution, before I malloc the memory, I do some basic check, to make sure there are enough bytes to satisfied this length. then I will do malloc, if not, I will return false. what's more, I do not malloc such large memory space at one time, first I will malloc 1/4 of the needed space, and realloc twice. the first malloc make sure the space is the same as the binary size itself, If we have a big binary we also need a big memory space. I think it is reasonable.
The purpose of this plan is to avoid the user gives us a fake length, and we should do some thing to check the code if we can trust this length.
and I dont think there is a limit of br_table param length, so I cant use fixed size array. Or It may influence the user who use it in a right way.
that's my opinion, If you have a better solution. Im glad to accept it.

aquynh · 2019-02-20T02:46:23Z

 		}

-		data = data + ((code[i] & 0x7f) << (i * 7));
+		data = data + (((uint64_t) code[i] & 0x7f) << (i * 7));


this should fit in a separate pull req, so i can merge it independently?

indeed, do you want me to submit a separate pull request ?

yes, please

or if @SpikeI agree with every changes here, i can merge all of them, without needing you to create a new PR.

catenacyber · 2019-02-20T08:48:36Z

@Spikel this will not solve yet the overlong instruction printing from #1381 : capstone limits it to 512 bytes, and so we cannot print a WASM brtable with some hundreds of targets...

For the safety checks, you can look at 6e53ff1

This proposed solution looks to me more like switch instruction in other architectures : the jump table's address is given and the user has to decode all the targets himself. The difference with other architectures is that with WASM, targets are encoded with variable length...

@aquynh what do you think ?

aquynh · 2019-02-20T15:05:14Z

Wasm is really special, because it has this br_table instruction, that can be arbitrary long, thus does fit into the existing model.

@catenacyber i think your approach is reasonable. @SpikeI may come up with a similar fix.

SpikeI

@Spikel this will not solve yet the overlong instruction printing from #1381 : capstone limits it to 512 bytes, and so we cannot print a WASM brtable with some hundreds of targets...

For the safety checks, you can look at 6e53ff1

This proposed solution looks to me more like switch instruction in other architectures : the jump table's address is given and the user has to decode all the targets himself. The difference with other architectures is that with WASM, targets are encoded with variable length...

@aquynh what do you think ?

I see, you are right. Its my fault to leave many memory mistakes.
the safety check is good, and the table we can not print. you are right.
the only inconvenient thing is that user should read varuint32 themself?

SpikeI · 2019-02-20T03:46:21Z

 		}

-		MI->wasm_data.brtable.target[i] = get_varuint32(code + tmp_len, code_len - tmp_len, &var_len);
+		get_varuint32(code + tmp_len, code_len - tmp_len, &var_len);


why we do not keep this data?

SpikeI · 2019-02-20T15:32:50Z

Wasm is really special, because it has this br_table instruction, that can be arbitrary long, thus does fit into the existing model.

@catenacyber i think your approach is reasonable. @SpikeI may come up with a similar fix.

@catenacyber @aquynh
yes, I agree with this fix, please merge

aquynh · 2019-02-20T15:38:17Z

merged, thanks!

catenacyber · 2019-02-20T16:23:30Z

Thanks @SpikeI for agreeing and congratulations for your work :-)

aquynh reviewed Feb 7, 2019

View reviewed changes

catenacyber mentioned this pull request Feb 15, 2019

Avoid overflow in SStream.c #1381

Merged

catenacyber force-pushed the leakwasm branch from 5641768 to c3df800 Compare February 15, 2019 16:33

catenacyber added 4 commits February 16, 2019 10:25

Avoids leak in wasm details

fef277e

Extending cs_detail in capstone.h

Safety checks before allocating memory for brtable in WASM

6e53ff1

Revert "Avoids leak in wasm details"

8bc7fb3

This reverts commit 03f822b.

Refactoring brtable for WASM

14e068f

catenacyber force-pushed the leakwasm branch from dcd8232 to 14e068f Compare February 16, 2019 09:25

Fix undefined shift in WASM get_varuint64

89e1725

aquynh reviewed Feb 20, 2019

View reviewed changes

SpikeI reviewed Feb 20, 2019

View reviewed changes

aquynh merged commit 0e51445 into capstone-engine:next Feb 20, 2019

riptl mentioned this pull request Jul 22, 2022

Add PowerPC paired-singles support #1898

Merged

6 tasks

Conversation

catenacyber commented Feb 7, 2019

Uh oh!

aquynh commented Feb 7, 2019

Uh oh!

catenacyber commented Feb 7, 2019

Uh oh!

aquynh Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

catenacyber Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

aquynh Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catenacyber commented Feb 14, 2019

Uh oh!

aquynh commented Feb 15, 2019

Uh oh!

aquynh commented Feb 15, 2019

Uh oh!

catenacyber commented Feb 15, 2019

Uh oh!

aquynh commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

catenacyber commented Feb 16, 2019

Uh oh!

aquynh commented Feb 18, 2019

Uh oh!

catenacyber commented Feb 18, 2019

Uh oh!

SpikeI commented Feb 20, 2019

Uh oh!

aquynh Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

catenacyber Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

aquynh Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

aquynh Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

catenacyber commented Feb 20, 2019

Uh oh!

aquynh commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SpikeI left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SpikeI Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

SpikeI commented Feb 20, 2019

Uh oh!

aquynh commented Feb 20, 2019

Uh oh!

catenacyber commented Feb 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aquynh Feb 8, 2019 •

edited

Loading

aquynh commented Feb 15, 2019 •

edited

Loading

aquynh commented Feb 20, 2019 •

edited

Loading

SpikeI left a comment •

edited

Loading