You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Decided to take a quick look at ledger-app-btc code, and noticed that the code referenced above have potential integrity issues, although with current code the issues do not lead to actual problems because bip32_print_path is only ever called on vars.tmp_warning.derivation_pathand vars is a union that contains other member structs that has the size much larger than tmp_warning, and writing beyond derivation_path will just trumple that unused space.
That said, I see two issues that, if not fixed and then bip32_print_path used with other destinations, could lead to data integrity issue that can actually manifest.
uint8_t len=strnlen(out, MAX_DERIV_PATH_ASCII_LENGTH); uses constant instead of max_out_len argument, making assumption that is not tested inside the function; I think max_out_len should be used instead of MAX_DERIV_PATH_ASCII_LENGTH
the loop makes up to 4 moves (MAX_DERIV_PATH_ASCII_LENGTH/30) of the data 'forward', thus extending the length of the data up to 4 bytes, and potentially making the resulting data length 4 bytes longer than the max allowed length, overwriting 4 bytes of unrelated data.
The text was updated successfully, but these errors were encountered:
https://github.com/LedgerHQ/ledger-app-btc/blob/03d0cbcad04fd84394a8af4de28594deb302b980/src/btchip_helpers.c#L402-L411
Decided to take a quick look at ledger-app-btc code, and noticed that the code referenced above have potential integrity issues, although with current code the issues do not lead to actual problems because
bip32_print_path
is only ever called onvars.tmp_warning.derivation_path
andvars
is a union that contains other member structs that has the size much larger thantmp_warning
, and writing beyondderivation_path
will just trumple that unused space.That said, I see two issues that, if not fixed and then
bip32_print_path
used with other destinations, could lead to data integrity issue that can actually manifest.uint8_t len=strnlen(out, MAX_DERIV_PATH_ASCII_LENGTH);
uses constant instead ofmax_out_len
argument, making assumption that is not tested inside the function; I thinkmax_out_len
should be used instead ofMAX_DERIV_PATH_ASCII_LENGTH
the loop makes up to 4 moves (MAX_DERIV_PATH_ASCII_LENGTH/30) of the data 'forward', thus extending the length of the data up to 4 bytes, and potentially making the resulting data length 4 bytes longer than the max allowed length, overwriting 4 bytes of unrelated data.
The text was updated successfully, but these errors were encountered: