Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dgpv opened this issue Jan 27, 2020 · 1 comment

Comments

@dgpv
Copy link

dgpv commented Jan 27, 2020

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.

@TamtamHero
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants