aergoio/ledger-app-aergo

it cause buffer overflow because amount_str can be 39 length

Closed this issue · 7 comments

convertUint256BE(buf, len, &uint256);

Overflow here in the tmp variable, right?

uint8_t tmp[32];
os_memset(tmp, 0, 32);
os_memmove(tmp + 32 - length, data, length);

I will fix it.

Thanks.

You can also send PRs if you want.

Well, I checked and it will not overflow because this function is receiving a value in binary big endian format. It is not converted to string yet.

Could you check?

at line 6 when length is 38 then os_memmove (tmp -6, data, 38) so it can memory violation

This function is receiving the amount encoded as variable size big endian integer from protobuf and will convert it to an uint256 first and then to a string.

The encoded amount cannot be so big (38 bytes). It is only this size when in string format, right?

Note that the same function is also used on the Ethereum app:

https://github.com/LedgerHQ/ledger-app-eth/blob/d43b530ff1bac20f70728b97256dddc7764febe6/src/utils.c#L36-L41

if (str_len > 40) goto loc_invalid;

according above line, i think amount length to 40. but protobuf limit size to uint256 then ok

Oh, right! Maybe we can fix this line. I will verify the limits. Thanks

Fixed. Currently on the develop branch.

It could indeed generate some problems.