Skip to content

Commit

Permalink
fix: avoid an underflow in Ledger BIP32 path parsing (#6482)
Browse files Browse the repository at this point in the history
Description
---
Avoids a possible underflow in Ledger BIP32 path parsing.

Motivation and Context
---
The Ledger application code includes `TryFrom<&[u8]>` parsing for BIP32
paths that is intended to return an error on an empty slice. However,
the initial path length computation will underflow if this occurs.

While this won't panic in release mode, it would almost certainly fail
an unrelated check that the path length does not exceed a generic
maximum, instead of triggering a subsequent test that the input slice is
not empty. This almost certainly would not result in unintended
behavior, and would return an error. However, it has a bad code smell.

This PR moves the empty slice check so it is done prior to computing the
path length, in order to ensure that no underflow occurs.

How Has This Been Tested?
---
No automated testing appears to exist for this.

What process can a PR reviewer use to test or verify this change?
---
Check that the underflow condition can no longer occur.
  • Loading branch information
AaronFeickert authored Aug 20, 2024
1 parent 5801aa6 commit fed0bdf
Showing 1 changed file with 7 additions and 4 deletions.
11 changes: 7 additions & 4 deletions applications/minotari_ledger_wallet/wallet/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,14 @@ impl<const S: usize> TryFrom<&[u8]> for Bip32Path<S> {
///
/// * `data` - Encoded BIP32 path. First byte is the length of the path, as encoded by ragger.
fn try_from(data: &[u8]) -> Result<Self, Self::Error> {
// Assert the data is not empty; we need at least a length byte!
if data.is_empty() {
return Err(AppSW::WrongApduLength);
}

// We cannot have too many elements in the path, and must have `u32` path elements
let input_path_len = (data.len() - 1) / 4;
// Check data length
if data.is_empty() // At least the length byte is required
|| (input_path_len > S)
|| (data[0] as usize * 4 != data.len() - 1)
if input_path_len > S || data[0] as usize * 4 != data.len() - 1
{
return Err(AppSW::WrongApduLength);
}
Expand Down

0 comments on commit fed0bdf

Please sign in to comment.