Skip to content

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

@dgpv

Description

@dgpv

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions