app-bitcoin icon indicating copy to clipboard operation
app-bitcoin copied to clipboard

bip32_print_path(): 30-char path splitting for TARGET_BLUE is not correct integrity-wise

Open dgpv opened this issue 5 years ago • 1 comments

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 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.

  1. 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

  2. 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.

dgpv avatar Jan 27 '20 12:01 dgpv

Thanks, a fix will be deployed in the next version of this app :)

TamtamHero avatar Jan 31 '20 15:01 TamtamHero