From cd549721bff262b3f8662335b4bb227c9fd52e36 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 29 Nov 2023 15:42:53 -0800 Subject: [PATCH 1/8] fix: greatly simplify rusb usage --- ledger/Cargo.toml | 1 + ledger/src/common.rs | 2 +- ledger/src/transports/mod.rs | 1 + ledger/src/transports/native/error.rs | 50 ++++++ ledger/src/transports/native/hid.rs | 238 ++++++++++---------------- ledger/src/transports/native/mod.rs | 10 +- ledger/src/transports/wasm.rs | 13 ++ ledger/tests/integration.rs | 4 +- 8 files changed, 168 insertions(+), 151 deletions(-) create mode 100644 ledger/src/transports/native/error.rs diff --git a/ledger/Cargo.toml b/ledger/Cargo.toml index 6673fb25..9c78256a 100644 --- a/ledger/Cargo.toml +++ b/ledger/Cargo.toml @@ -20,6 +20,7 @@ crate-type = ["cdylib", "rlib"] async-trait = "0.1" cfg-if = "1.0" hex = "0.4" +once_cell = "1.18.0" serde = { version = "1.0", features = ["derive"] } tap = "1.0" thiserror = "1.0" diff --git a/ledger/src/common.rs b/ledger/src/common.rs index 77da51eb..7305df4e 100644 --- a/ledger/src/common.rs +++ b/ledger/src/common.rs @@ -265,7 +265,7 @@ impl APDUResponseCodes { } APDUResponseCodes::InvalidP1P2 => "[APDU_CODE_INVALIDP1P2] Wrong parameter(s) P1-P2", APDUResponseCodes::InsNotSupported => { - "[APDU_CODE_INS_NOT_SUPPORTED] Instruction code not supported or invalid" + "[APDU_CODE_INS_NOT_SUPPORTED] Instruction code not supported or invalid. Hint: Is the correct application open on the device?" } APDUResponseCodes::ClaNotSupported => { "[APDU_CODE_CLA_NOT_SUPPORTED] Class not supported" diff --git a/ledger/src/transports/mod.rs b/ledger/src/transports/mod.rs index c461e053..dffd9b22 100644 --- a/ledger/src/transports/mod.rs +++ b/ledger/src/transports/mod.rs @@ -27,6 +27,7 @@ cfg_if::cfg_if! { /// A Ledger device connection. This wraps the default transport type. In /// native code, this is the `hidapi` library. When the `node` or `browser` /// feature is selected, it is a Ledger JS transport library. +#[derive(Debug)] pub struct Ledger(DefaultTransport); #[cfg_attr(not(target_arch = "wasm32"), async_trait)] diff --git a/ledger/src/transports/native/error.rs b/ledger/src/transports/native/error.rs new file mode 100644 index 00000000..8f55f896 --- /dev/null +++ b/ledger/src/transports/native/error.rs @@ -0,0 +1,50 @@ +use thiserror::Error; + +// Mock the type in other target_os +#[cfg(not(target_os = "linux"))] +mod nix { + #[derive(thiserror::Error, Debug)] + pub enum Error { + #[error("")] + Unimplemented, + } +} + +/// Ledger transport errors +#[derive(Error, Debug)] +pub enum NativeTransportError { + /// Device not found error + #[error("Ledger device not found")] + DeviceNotFound, + /// Device open error. + #[error("Error opening device. {0}. Hint: This usually means that the device is already in use by another transport instance.")] + CantOpen(hidapi_rusb::HidError), + /// SequenceMismatch + #[error("Sequence mismatch. Got {got} from device. Expected {expected}")] + SequenceMismatch { + /// The sequence returned by the device + got: u16, + /// The expected sequence + expected: u16, + }, + /// Communication error + #[error("Ledger device: communication error `{0}`")] + Comm(&'static str), + /// Ioctl error + #[error(transparent)] + Ioctl(#[from] nix::Error), + /// i/o error + #[error(transparent)] + Io(#[from] std::io::Error), + /// HID error + #[error(transparent)] + Hid(#[from] hidapi_rusb::HidError), + /// UT8F error + #[error(transparent)] + UTF8(#[from] std::str::Utf8Error), + /// Termux USB FD env var does not exist or fails to parse. This error is + /// only returned by android-specific code paths, and may be safely ignored + /// by non-android users + #[error("Invalid TERMUX_USB_FD variable. Are you using termux-usb?")] + InvalidTermuxUsbFd, +} diff --git a/ledger/src/transports/native/hid.rs b/ledger/src/transports/native/hid.rs index 7d61cdd5..ae221036 100644 --- a/ledger/src/transports/native/hid.rs +++ b/ledger/src/transports/native/hid.rs @@ -1,150 +1,119 @@ //! Native HID APDU transport for Ledger Nano hardware wallets -use lazy_static::lazy_static; -use thiserror::Error; use crate::{ common::{APDUAnswer, APDUCommand}, errors::LedgerError, }; -use std::{ffi::CString, io::Cursor}; - use byteorder::{BigEndian, ReadBytesExt}; -use hidapi_rusb::HidDevice; -use std::cell::RefCell; -use std::sync::{Arc, Mutex, Weak}; +use hidapi_rusb::{DeviceInfo, HidApi, HidDevice}; +use once_cell::sync::Lazy; +use std::{io::Cursor, sync::Mutex}; -// Mock the type in other target_os -#[cfg(not(target_os = "linux"))] -mod nix { - #[derive(thiserror::Error, Debug)] - pub enum Error { - #[error("")] - Unimplemented, - } -} +use super::NativeTransportError; const LEDGER_VID: u16 = 0x2c97; - #[cfg(not(target_os = "linux"))] const LEDGER_USAGE_PAGE: u16 = 0xFFA0; const LEDGER_CHANNEL: u16 = 0x0101; -const LEDGER_PACKET_SIZE: u8 = 64; - +// for Windows compatability, we prepend the buffer with a 0x00 +// so the actual buffer is 64 bytes +const LEDGER_PACKET_WRITE_SIZE: u8 = 65; +const LEDGER_PACKET_READ_SIZE: u8 = 64; const LEDGER_TIMEOUT: i32 = 10_000_000; -/// Ledger transport errors -#[derive(Error, Debug)] -pub enum NativeTransportError { - /// Device not found error - #[error("Ledger device not found")] - DeviceNotFound, - /// SequenceMismatch - #[error("Sequence mismatch. Got {got} from device. Expected {expected}")] - SequenceMismatch { - /// The sequence returned by the device - got: u16, - /// The expected sequence - expected: u16, - }, - /// Communication error - #[error("Ledger device: communication error `{0}`")] - Comm(&'static str), - /// Ioctl error - #[error(transparent)] - Ioctl(#[from] nix::Error), - /// i/o error - #[error(transparent)] - Io(#[from] std::io::Error), - /// HID error - #[error(transparent)] - Hid(#[from] hidapi_rusb::HidError), - /// UT8F error - #[error(transparent)] - UTF8(#[from] std::str::Utf8Error), - /// Termux USB FD env var does not exist or fails to parse. This error is - /// only returned by androd-specific code paths, and may be safely ignored - /// by non-android users - #[error("Invalid TERMUX_USB_FD variable. Are you using termux-usb?")] - InvalidTermuxUsbFd, -} +/// The HID API instance. +pub static HIDAPI: Lazy = + Lazy::new(|| HidApi::new().expect("Failed to initialize HID API")); -struct HidApiWrapper { - _api: RefCell>>, +/// Native HID transport for Ledger Nano hardware wallets +pub struct TransportNativeHID { + device: Mutex, } -/// The transport struct. Holds a `Mutex` on the underlying `HidAPI` instance. -/// -/// Instantiate with [`new`][TransportNativeHID::new]. -pub struct TransportNativeHID { - api_mutex: Arc>, - device: HidDevice, - guard: Mutex, +#[cfg(not(target_os = "linux"))] +fn is_ledger(dev: &DeviceInfo) -> bool { + dev.vendor_id() == LEDGER_VID && dev.usage_page() == LEDGER_USAGE_PAGE } -unsafe impl Send for HidApiWrapper {} +#[cfg(target_os = "linux")] +fn is_ledger(dev: &DeviceInfo) -> bool { + dev.vendor_id() == LEDGER_VID +} -lazy_static! { - static ref HIDAPIWRAPPER: Arc> = - Arc::new(Mutex::new(HidApiWrapper::new())); +/// Get a list of ledger devices available +fn list_ledgers(api: &HidApi) -> impl Iterator { + api.device_list().filter(|dev| is_ledger(dev)) } -impl HidApiWrapper { - fn new() -> Self { - HidApiWrapper { - _api: RefCell::new(Weak::new()), - } - } +#[tracing::instrument(skip_all, err)] +fn first_ledger(api: &HidApi) -> Result { + let device = list_ledgers(api) + .next() + .ok_or(NativeTransportError::DeviceNotFound)?; - fn get(&self) -> Result>, NativeTransportError> { - let tmp = self._api.borrow().upgrade(); - if let Some(api_mutex) = tmp { - return Ok(api_mutex); - } + open_device(api, device) +} - let hidapi = hidapi_rusb::HidApi::new()?; - let tmp = Arc::new(Mutex::new(hidapi)); - self._api.replace(Arc::downgrade(&tmp)); - Ok(tmp) - } +/// Read the 5-byte response header. +fn read_response_header(rdr: &mut Cursor<&[u8]>) -> Result<(u16, u8, u16), NativeTransportError> { + let rcv_channel = rdr.read_u16::()?; + let rcv_tag = rdr.read_u8()?; + let rcv_seq_idx = rdr.read_u16::()?; + Ok((rcv_channel, rcv_tag, rcv_seq_idx)) +} + +/// Open a specific ledger device +/// +/// # Note +/// No checks are made to ensure the device is a ledger device +/// +/// # Warning +/// Opening the same device concurrently will lead to device lock after the first handle is closed +/// see [issue](https://github.com/ruabmbua/hidapi-rs/issues/81) +fn open_device(api: &HidApi, device: &DeviceInfo) -> Result { + let device = device + .open_device(api) + .map_err(NativeTransportError::CantOpen)?; + let _ = device.set_blocking_mode(true); + + Ok(device) } impl TransportNativeHID { - #[cfg(not(target_os = "linux"))] - fn find_ledger_device_path(api: &hidapi_rusb::HidApi) -> Result { - for device in api.device_list() { - if device.vendor_id() == LEDGER_VID && device.usage_page() == LEDGER_USAGE_PAGE { - return Ok(device.path().into()); - } + /// Instantiate from a device. + fn from_device(device: HidDevice) -> Self { + Self { + device: Mutex::new(device), } - Err(NativeTransportError::DeviceNotFound) } - #[cfg(target_os = "linux")] - fn find_ledger_device_path(api: &hidapi_rusb::HidApi) -> Result { - for device in api.device_list() { - if device.vendor_id() == LEDGER_VID { - return Ok(device.path().into()); - } - } - Err(NativeTransportError::DeviceNotFound) + /// Get manufacturer string. Returns None on error, or on no string. + pub fn get_manufacturer_string(&self) -> Option { + let device = self.device.lock().unwrap(); + device.get_manufacturer_string().unwrap_or_default() } - /// Get the device path string - #[allow(dead_code)] - pub fn device_path(&self) -> Result { - Self::find_ledger_device_path(&self.api_mutex.lock().unwrap()) + /// Open all ledger devices. + pub fn open_all_devices() -> Result, NativeTransportError> { + let api = &HIDAPI; + let devices = list_ledgers(api) + .map(|dev| open_device(api, dev)) + .collect::, _>>()?; + + Ok(devices.into_iter().map(Self::from_device).collect()) } - /// Get a new TransportNativeHID by acquiring a lock on the global `hidapi_rusb::HidAPI`. - /// Note that this may block forever if the resource is in use. + /// Create a new HID transport, connecting to the first ledger found + /// + /// # Warning + /// Opening the same device concurrently will lead to device lock after the first handle is closed + /// see [issue](https://github.com/ruabmbua/hidapi-rs/issues/81) pub fn new() -> Result { - let apiwrapper = HIDAPIWRAPPER.lock().expect("Could not lock api wrapper"); - let api_mutex = apiwrapper.get().expect("Error getting api_mutex"); - let api = api_mutex.lock().expect("Could not lock"); + let api = &HIDAPI; #[cfg(target_os = "android")] - let device = { + { // Using runtime detection since it's impossible to statically target Termux. let is_termux = match std::env::var("PREFIX") { Ok(prefix_var) => prefix_var.contains("/com.termux/"), @@ -157,50 +126,40 @@ impl TransportNativeHID { .map_err(|_| NativeTransportError::InvalidTermuxUsbFd)? .parse::() .map_err(|_| NativeTransportError::InvalidTermuxUsbFd)?; - api.wrap_sys_device(usb_fd, -1)? + api.wrap_sys_device(usb_fd, -1).map(Self::from_device)? } else { // Not sure how we should handle non-Termux Android here. This likely won't work. - let device_path = TransportNativeHID::find_ledger_device_path(&api)?; - api.open_path(&device_path)? + first_ledger(api).map(Self::from_device) } - }; + } #[cfg(not(target_os = "android"))] - let device = { - let device_path = TransportNativeHID::find_ledger_device_path(&api)?; - api.open_path(&device_path)? - }; - - let ledger = TransportNativeHID { - device, - guard: Mutex::new(0), - api_mutex: api_mutex.clone(), - }; - - Ok(ledger) + first_ledger(api).map(Self::from_device) } fn write_apdu(&self, channel: u16, apdu_command: &[u8]) -> Result { + let device = self.device.lock().unwrap(); + let command_length = apdu_command.len(); let mut in_data = Vec::with_capacity(command_length + 2); in_data.push(((command_length >> 8) & 0xFF) as u8); in_data.push((command_length & 0xFF) as u8); in_data.extend_from_slice(apdu_command); - let mut buffer = [0u8; LEDGER_PACKET_SIZE as usize]; + let mut buffer = [0u8; LEDGER_PACKET_WRITE_SIZE as usize]; buffer[0] = ((channel >> 8) & 0xFF) as u8; // channel big endian buffer[1] = (channel & 0xFF) as u8; // channel big endian buffer[2] = 0x05u8; for (sequence_idx, chunk) in in_data - .chunks((LEDGER_PACKET_SIZE - 5) as usize) + .chunks((LEDGER_PACKET_WRITE_SIZE - 5) as usize) .enumerate() { buffer[3] = ((sequence_idx >> 8) & 0xFF) as u8; // sequence_idx big endian buffer[4] = (sequence_idx & 0xFF) as u8; // sequence_idx big endian buffer[5..5 + chunk.len()].copy_from_slice(chunk); - let result = self.device.write(&buffer); + let result = device.write(&buffer); match result { Ok(size) => { @@ -216,19 +175,11 @@ impl TransportNativeHID { Ok(1) } - /// Read the 5-byte response header. - fn read_response_header( - rdr: &mut Cursor<&[u8]>, - ) -> Result<(u16, u8, u16), NativeTransportError> { - let rcv_channel = rdr.read_u16::()?; - let rcv_tag = rdr.read_u8()?; - let rcv_seq_idx = rdr.read_u16::()?; - Ok((rcv_channel, rcv_tag, rcv_seq_idx)) - } - /// Read a response APDU from the ledger channel. fn read_response_apdu(&self, _channel: u16) -> Result, NativeTransportError> { - let mut response_buffer = [0u8; LEDGER_PACKET_SIZE as usize]; + let device = self.device.lock().unwrap(); + + let mut response_buffer = [0u8; LEDGER_PACKET_READ_SIZE as usize]; let mut sequence_idx = 0u16; let mut expected_response_len = 0usize; let mut offset = 0; @@ -236,16 +187,14 @@ impl TransportNativeHID { let mut answer_buf = vec![]; loop { - let res = self - .device - .read_timeout(&mut response_buffer, LEDGER_TIMEOUT)?; + let res = device.read_timeout(&mut response_buffer, LEDGER_TIMEOUT)?; if (sequence_idx == 0 && res < 7) || res < 5 { return Err(NativeTransportError::Comm("Read error. Incomplete header")); } let mut rdr = Cursor::new(&response_buffer[..]); - let (_, _, rcv_seq_idx) = Self::read_response_header(&mut rdr)?; + let (_, _, rcv_seq_idx) = read_response_header(&mut rdr)?; if rcv_seq_idx != sequence_idx { return Err(NativeTransportError::SequenceMismatch { @@ -287,9 +236,6 @@ impl TransportNativeHID { /// If the method errors, the buf may contain a partially written response. It is not advised /// to read this. pub fn exchange(&self, command: &APDUCommand) -> Result { - // acquire the internal communication lock - let _guard = self.guard.lock().unwrap(); - self.write_apdu(LEDGER_CHANNEL, &command.serialize())?; let answer_buf = self.read_response_apdu(LEDGER_CHANNEL)?; @@ -315,7 +261,7 @@ impl TransportNativeHID { } /******************************************************************************* -* (c) 2018 ZondaX GmbH +* (c) 2018-2022 ZondaX GmbH * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/ledger/src/transports/native/mod.rs b/ledger/src/transports/native/mod.rs index d1ef40bc..c38b9dc7 100644 --- a/ledger/src/transports/native/mod.rs +++ b/ledger/src/transports/native/mod.rs @@ -2,8 +2,11 @@ use tokio::sync::{mpsc, oneshot}; use crate::{APDUAnswer, APDUCommand, LedgerError}; +mod error; +pub use error::NativeTransportError; + pub mod hid; -pub use hid::{NativeTransportError, TransportNativeHID}; +use hid::TransportNativeHID; /// A packet exchange request. struct APDUExchange { @@ -46,7 +49,10 @@ impl LedgerTask { while let Some(exchange) = self.rx.recv().await { // blocking IO let answer = self.ledger.exchange(&exchange.command); - let _ = exchange.answer.send(answer); + let answer = exchange.answer.send(answer); + if let Err(Err(err)) = answer { + tracing::error!(err = %err, "could not send answer to exchange"); + } } }; diff --git a/ledger/src/transports/wasm.rs b/ledger/src/transports/wasm.rs index 233f84a0..a4e19a6e 100644 --- a/ledger/src/transports/wasm.rs +++ b/ledger/src/transports/wasm.rs @@ -47,6 +47,19 @@ extern "C" { #[wasm_bindgen] pub struct LedgerTransport(Transport); +impl std::fmt::Debug for LedgerTransport { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + #[cfg(feature = "node")] + let transport_type = "@ledgerhq/hw-transport-node-hid"; + #[cfg(feature = "browser")] + let transport_type = "@ledgerhq/hw-transport-webusb"; + + f.debug_struct("LedgerTransport") + .field("transport_library", &transport_type) + .finish() + } +} + /// Transport Impl for wasm impl LedgerTransport { /// Send an APDU command to the device, and receive a response diff --git a/ledger/tests/integration.rs b/ledger/tests/integration.rs index 54a67083..f18b5c1c 100644 --- a/ledger/tests/integration.rs +++ b/ledger/tests/integration.rs @@ -12,8 +12,8 @@ fn ledger_device_path() { let transport = hid::TransportNativeHID::new().unwrap(); // TODO: Extend to discover two devices - let ledger_path = transport.device_path().expect("Could not find a device"); - println!("{ledger_path:?}"); + let manufacturer = transport.get_manufacturer_string(); + println!("{manufacturer:?}"); } #[tokio::test] From 0da609951c8234fb976cf58e99b3d143ea109d46 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 29 Nov 2023 15:49:19 -0800 Subject: [PATCH 2/8] lints: clippies and more --- ledger/Cargo.toml | 8 +------- ledger/src/common.rs | 7 ++++--- ledger/src/lib.rs | 16 ++++++++++++++-- ledger/src/protocol.rs | 3 +++ ledger/src/transports/native/error.rs | 2 +- ledger/src/transports/native/hid.rs | 11 ++++++----- ledger/src/transports/native/mod.rs | 6 +++--- ledger/src/transports/wasm.rs | 1 + 8 files changed, 33 insertions(+), 21 deletions(-) diff --git a/ledger/Cargo.toml b/ledger/Cargo.toml index 9c78256a..2e3a987f 100644 --- a/ledger/Cargo.toml +++ b/ledger/Cargo.toml @@ -20,17 +20,12 @@ crate-type = ["cdylib", "rlib"] async-trait = "0.1" cfg-if = "1.0" hex = "0.4" -once_cell = "1.18.0" -serde = { version = "1.0", features = ["derive"] } -tap = "1.0" thiserror = "1.0" # native [target.'cfg(not(target_arch = "wasm32"))'.dependencies] -lazy_static = "1.4" +once_cell = "1.18.0" byteorder = "1.5" -libc = "0.2" -matches = "0.1" tracing = "0.1" hidapi-rusb = "1.3" tokio = { version = "1.34", features = ["sync", "rt"] } @@ -45,7 +40,6 @@ wasm-bindgen = "0.2.88" wasm-bindgen-futures = "0.4.38" js-sys = "0.3.65" log = "0.4" -getrandom = { version = "0.2", features = ["js"] } [dev-dependencies] serial_test = "2" diff --git a/ledger/src/common.rs b/ledger/src/common.rs index 7305df4e..927eeb07 100644 --- a/ledger/src/common.rs +++ b/ledger/src/common.rs @@ -32,6 +32,7 @@ impl APDUData { } /// Consume the struct and get the underlying data + #[allow(clippy::missing_const_for_fn)] // false positive pub fn data(self) -> Vec { self.0 } @@ -234,12 +235,12 @@ impl std::fmt::Display for APDUResponseCodes { impl APDUResponseCodes { /// True if the response is a success, else false. - pub fn is_success(self) -> bool { - self == APDUResponseCodes::NoError + pub const fn is_success(self) -> bool { + matches!(self, APDUResponseCodes::NoError) } /// Return a description of the response code. - pub fn description(self) -> &'static str { + pub const fn description(self) -> &'static str { match self { APDUResponseCodes::NoError => "[APDU_CODE_NOERROR]", APDUResponseCodes::ExecutionError => { diff --git a/ledger/src/lib.rs b/ledger/src/lib.rs index b15a4709..539279ec 100644 --- a/ledger/src/lib.rs +++ b/ledger/src/lib.rs @@ -1,7 +1,18 @@ //! Ledger utilites and transports -#![warn(missing_docs)] -#![warn(unused_extern_crates)] +#![warn( + missing_copy_implementations, + missing_debug_implementations, + missing_docs, + unreachable_pub, + clippy::missing_const_for_fn, + rustdoc::all +)] +#![deny(unused_must_use, rust_2018_idioms)] + +#![cfg_attr(not(test), warn(unused_crate_dependencies))] +#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] + /// APDU utilities. pub mod common; @@ -19,3 +30,4 @@ pub use { }; mod protocol; +pub use protocol::LedgerProtocol; diff --git a/ledger/src/protocol.rs b/ledger/src/protocol.rs index 32c08451..44b64705 100644 --- a/ledger/src/protocol.rs +++ b/ledger/src/protocol.rs @@ -9,6 +9,7 @@ use crate::{Ledger, LedgerError}; /// be invoked. The protocol may also fail to recover, in which case future /// uses of the device may fail. pub trait LedgerProtocol { + /// The output of the protocol. type Output; /// Run the protocol. This sends commands to the device, and receives @@ -29,6 +30,8 @@ pub trait LedgerProtocol { Ok(()) } + /// Run the protocol. This sends commands to the device, and receives + /// responses. The transport is locked while this function is running. fn run(&mut self, transport: &mut Ledger) -> Result { match self.execute(transport) { Ok(output) => Ok(output), diff --git a/ledger/src/transports/native/error.rs b/ledger/src/transports/native/error.rs index 8f55f896..3e47db76 100644 --- a/ledger/src/transports/native/error.rs +++ b/ledger/src/transports/native/error.rs @@ -3,7 +3,7 @@ use thiserror::Error; // Mock the type in other target_os #[cfg(not(target_os = "linux"))] mod nix { - #[derive(thiserror::Error, Debug)] + #[derive(thiserror::Error, Debug, Copy, Clone)] pub enum Error { #[error("")] Unimplemented, diff --git a/ledger/src/transports/native/hid.rs b/ledger/src/transports/native/hid.rs index ae221036..10869121 100644 --- a/ledger/src/transports/native/hid.rs +++ b/ledger/src/transports/native/hid.rs @@ -31,6 +31,12 @@ pub struct TransportNativeHID { device: Mutex, } +impl std::fmt::Debug for TransportNativeHID { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("TransportNativeHID").finish() + } +} + #[cfg(not(target_os = "linux"))] fn is_ledger(dev: &DeviceInfo) -> bool { dev.vendor_id() == LEDGER_VID && dev.usage_page() == LEDGER_USAGE_PAGE @@ -253,11 +259,6 @@ impl TransportNativeHID { } } } - - // TODO: why does this exist? - #[doc(hidden)] - #[allow(dead_code)] - pub fn close(self) {} } /******************************************************************************* diff --git a/ledger/src/transports/native/mod.rs b/ledger/src/transports/native/mod.rs index c38b9dc7..7f12cf84 100644 --- a/ledger/src/transports/native/mod.rs +++ b/ledger/src/transports/native/mod.rs @@ -11,9 +11,9 @@ use hid::TransportNativeHID; /// A packet exchange request. struct APDUExchange { /// The command to send to the device. - pub command: APDUCommand, + command: APDUCommand, /// The channel to send the answer back on. - pub answer: oneshot::Sender>, + answer: oneshot::Sender>, } impl APDUExchange { @@ -44,7 +44,7 @@ impl LedgerTask { } /// Spawn the task that will run Ledger protocols. - pub fn spawn(mut self) { + fn spawn(mut self) { let fut = async move { while let Some(exchange) = self.rx.recv().await { // blocking IO diff --git a/ledger/src/transports/wasm.rs b/ledger/src/transports/wasm.rs index a4e19a6e..65c5d835 100644 --- a/ledger/src/transports/wasm.rs +++ b/ledger/src/transports/wasm.rs @@ -88,6 +88,7 @@ impl LedgerTransport { } /// Instantiate from a js transport object + #[allow(clippy::missing_const_for_fn)] // not allowed on wasm bindgen fns pub fn from_js_transport(transport: Transport) -> Self { Self(transport) } From ddd8aa3f1671595366c0899faf5f6a31755e82c8 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 29 Nov 2023 15:54:48 -0800 Subject: [PATCH 3/8] fix: restore getrandom dep --- ledger/Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/ledger/Cargo.toml b/ledger/Cargo.toml index 2e3a987f..a9cd561c 100644 --- a/ledger/Cargo.toml +++ b/ledger/Cargo.toml @@ -40,6 +40,7 @@ wasm-bindgen = "0.2.88" wasm-bindgen-futures = "0.4.38" js-sys = "0.3.65" log = "0.4" +getrandom = { version = "0.2", features = ["js"] } [dev-dependencies] serial_test = "2" From 1c724ed9ed9dca3d9893daf021f671358643a5f9 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 29 Nov 2023 15:55:04 -0800 Subject: [PATCH 4/8] fix: fmt --- ledger/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/ledger/src/lib.rs b/ledger/src/lib.rs index 539279ec..af215606 100644 --- a/ledger/src/lib.rs +++ b/ledger/src/lib.rs @@ -9,11 +9,9 @@ rustdoc::all )] #![deny(unused_must_use, rust_2018_idioms)] - #![cfg_attr(not(test), warn(unused_crate_dependencies))] #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] - /// APDU utilities. pub mod common; From 9f86de4edf71b4c631f870ed95548362e196ead7 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 30 Nov 2023 10:54:46 -0800 Subject: [PATCH 5/8] fix: avoid locking mutex twice --- ledger/src/transports/native/hid.rs | 196 +++++++++++++++------------- 1 file changed, 102 insertions(+), 94 deletions(-) diff --git a/ledger/src/transports/native/hid.rs b/ledger/src/transports/native/hid.rs index 10869121..1223fcef 100644 --- a/ledger/src/transports/native/hid.rs +++ b/ledger/src/transports/native/hid.rs @@ -8,7 +8,10 @@ use crate::{ use byteorder::{BigEndian, ReadBytesExt}; use hidapi_rusb::{DeviceInfo, HidApi, HidDevice}; use once_cell::sync::Lazy; -use std::{io::Cursor, sync::Mutex}; +use std::{ + io::Cursor, + sync::{Mutex, MutexGuard}, +}; use super::NativeTransportError; @@ -69,6 +72,99 @@ fn read_response_header(rdr: &mut Cursor<&[u8]>) -> Result<(u16, u8, u16), Nativ Ok((rcv_channel, rcv_tag, rcv_seq_idx)) } +fn write_apdu( + device: &mut MutexGuard<'_, HidDevice>, + channel: u16, + apdu_command: &[u8], +) -> Result { + let command_length = apdu_command.len(); + let mut in_data = Vec::with_capacity(command_length + 2); + in_data.push(((command_length >> 8) & 0xFF) as u8); + in_data.push((command_length & 0xFF) as u8); + in_data.extend_from_slice(apdu_command); + + let mut buffer = [0u8; LEDGER_PACKET_WRITE_SIZE as usize]; + buffer[0] = ((channel >> 8) & 0xFF) as u8; // channel big endian + buffer[1] = (channel & 0xFF) as u8; // channel big endian + buffer[2] = 0x05u8; + + for (sequence_idx, chunk) in in_data + .chunks((LEDGER_PACKET_WRITE_SIZE - 5) as usize) + .enumerate() + { + buffer[3] = ((sequence_idx >> 8) & 0xFF) as u8; // sequence_idx big endian + buffer[4] = (sequence_idx & 0xFF) as u8; // sequence_idx big endian + buffer[5..5 + chunk.len()].copy_from_slice(chunk); + + let result = device.write(&buffer); + + match result { + Ok(size) => { + if size < buffer.len() { + return Err(NativeTransportError::Comm( + "USB write error. Could not send whole message", + )); + } + } + Err(x) => return Err(NativeTransportError::Hid(x)), + } + } + Ok(1) +} + +/// Read a response APDU from the ledger channel. +fn read_response_apdu( + device: &mut MutexGuard<'_, HidDevice>, + _channel: u16, +) -> Result, NativeTransportError> { + let mut response_buffer = [0u8; LEDGER_PACKET_READ_SIZE as usize]; + let mut sequence_idx = 0u16; + let mut expected_response_len = 0usize; + let mut offset = 0; + + let mut answer_buf = vec![]; + + loop { + let res = device.read_timeout(&mut response_buffer, LEDGER_TIMEOUT)?; + + if (sequence_idx == 0 && res < 7) || res < 5 { + return Err(NativeTransportError::Comm("Read error. Incomplete header")); + } + + let mut rdr = Cursor::new(&response_buffer[..]); + let (_, _, rcv_seq_idx) = read_response_header(&mut rdr)?; + + if rcv_seq_idx != sequence_idx { + return Err(NativeTransportError::SequenceMismatch { + got: rcv_seq_idx, + expected: sequence_idx, + }); + } + + // The header packet contains the number of bytes of response data + if rcv_seq_idx == 0 { + expected_response_len = rdr.read_u16::()? as usize; + } + + let remaining_in_buf = response_buffer.len() - rdr.position() as usize; + let missing = expected_response_len - offset; + let end_p = rdr.position() as usize + std::cmp::min(remaining_in_buf, missing); + + let new_chunk = &response_buffer[rdr.position() as usize..end_p]; + + // Copy the response to the answer + answer_buf.extend(new_chunk); + // answer_buf[offset..offset + new_chunk.len()].copy_from_slice(new_chunk); + offset += new_chunk.len(); + + if offset >= expected_response_len { + return Ok(answer_buf); + } + + sequence_idx += 1; + } +} + /// Open a specific ledger device /// /// # Note @@ -143,96 +239,6 @@ impl TransportNativeHID { first_ledger(api).map(Self::from_device) } - fn write_apdu(&self, channel: u16, apdu_command: &[u8]) -> Result { - let device = self.device.lock().unwrap(); - - let command_length = apdu_command.len(); - let mut in_data = Vec::with_capacity(command_length + 2); - in_data.push(((command_length >> 8) & 0xFF) as u8); - in_data.push((command_length & 0xFF) as u8); - in_data.extend_from_slice(apdu_command); - - let mut buffer = [0u8; LEDGER_PACKET_WRITE_SIZE as usize]; - buffer[0] = ((channel >> 8) & 0xFF) as u8; // channel big endian - buffer[1] = (channel & 0xFF) as u8; // channel big endian - buffer[2] = 0x05u8; - - for (sequence_idx, chunk) in in_data - .chunks((LEDGER_PACKET_WRITE_SIZE - 5) as usize) - .enumerate() - { - buffer[3] = ((sequence_idx >> 8) & 0xFF) as u8; // sequence_idx big endian - buffer[4] = (sequence_idx & 0xFF) as u8; // sequence_idx big endian - buffer[5..5 + chunk.len()].copy_from_slice(chunk); - - let result = device.write(&buffer); - - match result { - Ok(size) => { - if size < buffer.len() { - return Err(NativeTransportError::Comm( - "USB write error. Could not send whole message", - )); - } - } - Err(x) => return Err(NativeTransportError::Hid(x)), - } - } - Ok(1) - } - - /// Read a response APDU from the ledger channel. - fn read_response_apdu(&self, _channel: u16) -> Result, NativeTransportError> { - let device = self.device.lock().unwrap(); - - let mut response_buffer = [0u8; LEDGER_PACKET_READ_SIZE as usize]; - let mut sequence_idx = 0u16; - let mut expected_response_len = 0usize; - let mut offset = 0; - - let mut answer_buf = vec![]; - - loop { - let res = device.read_timeout(&mut response_buffer, LEDGER_TIMEOUT)?; - - if (sequence_idx == 0 && res < 7) || res < 5 { - return Err(NativeTransportError::Comm("Read error. Incomplete header")); - } - - let mut rdr = Cursor::new(&response_buffer[..]); - let (_, _, rcv_seq_idx) = read_response_header(&mut rdr)?; - - if rcv_seq_idx != sequence_idx { - return Err(NativeTransportError::SequenceMismatch { - got: rcv_seq_idx, - expected: sequence_idx, - }); - } - - // The header packet contains the number of bytes of response data - if rcv_seq_idx == 0 { - expected_response_len = rdr.read_u16::()? as usize; - } - - let remaining_in_buf = response_buffer.len() - rdr.position() as usize; - let missing = expected_response_len - offset; - let end_p = rdr.position() as usize + std::cmp::min(remaining_in_buf, missing); - - let new_chunk = &response_buffer[rdr.position() as usize..end_p]; - - // Copy the response to the answer - answer_buf.extend(new_chunk); - // answer_buf[offset..offset + new_chunk.len()].copy_from_slice(new_chunk); - offset += new_chunk.len(); - - if offset >= expected_response_len { - return Ok(answer_buf); - } - - sequence_idx += 1; - } - } - /// Exchange an APDU with the device. The response data will be written to `answer_buf`, and a /// `APDUAnswer` struct will be created with a reference to `answer_buf`. /// @@ -242,9 +248,11 @@ impl TransportNativeHID { /// If the method errors, the buf may contain a partially written response. It is not advised /// to read this. pub fn exchange(&self, command: &APDUCommand) -> Result { - self.write_apdu(LEDGER_CHANNEL, &command.serialize())?; - - let answer_buf = self.read_response_apdu(LEDGER_CHANNEL)?; + let answer_buf = { + let mut device = self.device.lock().unwrap(); + write_apdu(&mut device, LEDGER_CHANNEL, &command.serialize())?; + read_response_apdu(&mut device, LEDGER_CHANNEL)? + }; let apdu_answer = APDUAnswer::from_answer(answer_buf)?; From 1d578d4e00e24bdd5e4c89174c105107f3469931 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Fri, 1 Dec 2023 10:11:49 -0700 Subject: [PATCH 6/8] fix: android compilation error (#130) --- ledger/src/transports/native/hid.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ledger/src/transports/native/hid.rs b/ledger/src/transports/native/hid.rs index 1223fcef..c0458ac5 100644 --- a/ledger/src/transports/native/hid.rs +++ b/ledger/src/transports/native/hid.rs @@ -228,7 +228,7 @@ impl TransportNativeHID { .map_err(|_| NativeTransportError::InvalidTermuxUsbFd)? .parse::() .map_err(|_| NativeTransportError::InvalidTermuxUsbFd)?; - api.wrap_sys_device(usb_fd, -1).map(Self::from_device)? + Ok(api.wrap_sys_device(usb_fd, -1).map(Self::from_device)?) } else { // Not sure how we should handle non-Termux Android here. This likely won't work. first_ledger(api).map(Self::from_device) From 678bcf62626945518919458c36c105c0d0e7d5c5 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 2 Dec 2023 09:46:53 -0800 Subject: [PATCH 7/8] fix: correct buffer in write --- ledger/Cargo.toml | 1 + ledger/src/transports/mod.rs | 2 +- ledger/src/transports/native/hid.rs | 100 +++++++++++++++++----------- 3 files changed, 63 insertions(+), 40 deletions(-) diff --git a/ledger/Cargo.toml b/ledger/Cargo.toml index a9cd561c..e7d1a74c 100644 --- a/ledger/Cargo.toml +++ b/ledger/Cargo.toml @@ -49,3 +49,4 @@ tokio = { version = "1.34", features = ["rt-multi-thread", "macros"] } [features] browser = [] node = [] + diff --git a/ledger/src/transports/mod.rs b/ledger/src/transports/mod.rs index dffd9b22..00dc6c6a 100644 --- a/ledger/src/transports/mod.rs +++ b/ledger/src/transports/mod.rs @@ -69,7 +69,7 @@ impl LedgerAsync for Ledger { "Received response from device" ) } - Err(e) => error!(err = format!("{}", &e), "Received error from device"), + Err(err) => error!(%err, "Error during communication"), } resp } diff --git a/ledger/src/transports/native/hid.rs b/ledger/src/transports/native/hid.rs index c0458ac5..71619201 100644 --- a/ledger/src/transports/native/hid.rs +++ b/ledger/src/transports/native/hid.rs @@ -76,40 +76,46 @@ fn write_apdu( device: &mut MutexGuard<'_, HidDevice>, channel: u16, apdu_command: &[u8], -) -> Result { +) -> Result<(), NativeTransportError> { + tracing::debug!(apdu = %hex::encode(&apdu_command), bytes = apdu_command.len(), "Writing APDU to device"); + let command_length = apdu_command.len(); + + // TODO: allocation-free method let mut in_data = Vec::with_capacity(command_length + 2); in_data.push(((command_length >> 8) & 0xFF) as u8); in_data.push((command_length & 0xFF) as u8); in_data.extend_from_slice(apdu_command); let mut buffer = [0u8; LEDGER_PACKET_WRITE_SIZE as usize]; - buffer[0] = ((channel >> 8) & 0xFF) as u8; // channel big endian - buffer[1] = (channel & 0xFF) as u8; // channel big endian - buffer[2] = 0x05u8; + // Windows platform requires 0x00 prefix and Linux/Mac tolerate this as + // well. So we leave buffer[0] as 0x00 + buffer[1] = ((channel >> 8) & 0xFF) as u8; // channel big endian + buffer[2] = (channel & 0xFF) as u8; // channel big endian + buffer[3] = 0x05u8; for (sequence_idx, chunk) in in_data - .chunks((LEDGER_PACKET_WRITE_SIZE - 5) as usize) + .chunks((LEDGER_PACKET_WRITE_SIZE - 6) as usize) .enumerate() { - buffer[3] = ((sequence_idx >> 8) & 0xFF) as u8; // sequence_idx big endian - buffer[4] = (sequence_idx & 0xFF) as u8; // sequence_idx big endian - buffer[5..5 + chunk.len()].copy_from_slice(chunk); - - let result = device.write(&buffer); - - match result { - Ok(size) => { - if size < buffer.len() { - return Err(NativeTransportError::Comm( - "USB write error. Could not send whole message", - )); - } - } - Err(x) => return Err(NativeTransportError::Hid(x)), + buffer[4] = ((sequence_idx >> 8) & 0xFF) as u8; // sequence_idx big endian + buffer[5] = (sequence_idx & 0xFF) as u8; // sequence_idx big endian + buffer[6..6 + chunk.len()].copy_from_slice(chunk); + + tracing::trace!( + buffer = hex::encode(&buffer), + sequence_idx, + bytes = chunk.len(), + "Writing chunk to device", + ); + let result = device.write(&buffer).map_err(NativeTransportError::Hid)?; + if result < buffer.len() { + return Err(NativeTransportError::Comm( + "USB write error. Could not send whole message", + )); } } - Ok(1) + Ok(()) } /// Read a response APDU from the ledger channel. @@ -125,15 +131,30 @@ fn read_response_apdu( let mut answer_buf = vec![]; loop { + let remaining = expected_response_len + .checked_sub(offset) + .unwrap_or_default(); + + tracing::trace!( + sequence_idx, + expected_response_len, + remaining, + answer_size = answer_buf.len(), + "Reading response from device.", + ); + let res = device.read_timeout(&mut response_buffer, LEDGER_TIMEOUT)?; + // The first packet contains the response length as u16, successive + // packets do not. if (sequence_idx == 0 && res < 7) || res < 5 { return Err(NativeTransportError::Comm("Read error. Incomplete header")); } - let mut rdr = Cursor::new(&response_buffer[..]); + let mut rdr: Cursor<&[u8]> = Cursor::new(&response_buffer[..]); let (_, _, rcv_seq_idx) = read_response_header(&mut rdr)?; + // Check sequence index. A mismatch means someone else read a packet.s if rcv_seq_idx != sequence_idx { return Err(NativeTransportError::SequenceMismatch { got: rcv_seq_idx, @@ -144,8 +165,14 @@ fn read_response_apdu( // The header packet contains the number of bytes of response data if rcv_seq_idx == 0 { expected_response_len = rdr.read_u16::()? as usize; + tracing::trace!( + expected_response_len, + "Received response length from device", + ); } + // Read either until the end of the buffer, or until we have read the + // expected response length let remaining_in_buf = response_buffer.len() - rdr.position() as usize; let missing = expected_response_len - offset; let end_p = rdr.position() as usize + std::cmp::min(remaining_in_buf, missing); @@ -154,7 +181,6 @@ fn read_response_apdu( // Copy the response to the answer answer_buf.extend(new_chunk); - // answer_buf[offset..offset + new_chunk.len()].copy_from_slice(new_chunk); offset += new_chunk.len(); if offset >= expected_response_len { @@ -190,12 +216,6 @@ impl TransportNativeHID { } } - /// Get manufacturer string. Returns None on error, or on no string. - pub fn get_manufacturer_string(&self) -> Option { - let device = self.device.lock().unwrap(); - device.get_manufacturer_string().unwrap_or_default() - } - /// Open all ledger devices. pub fn open_all_devices() -> Result, NativeTransportError> { let api = &HIDAPI; @@ -228,17 +248,19 @@ impl TransportNativeHID { .map_err(|_| NativeTransportError::InvalidTermuxUsbFd)? .parse::() .map_err(|_| NativeTransportError::InvalidTermuxUsbFd)?; - Ok(api.wrap_sys_device(usb_fd, -1).map(Self::from_device)?) - } else { - // Not sure how we should handle non-Termux Android here. This likely won't work. - first_ledger(api).map(Self::from_device) + return Ok(api.wrap_sys_device(usb_fd, -1).map(Self::from_device)?); } } - #[cfg(not(target_os = "android"))] first_ledger(api).map(Self::from_device) } + /// Get manufacturer string. Returns None on error, or on no string. + pub fn get_manufacturer_string(&self) -> Option { + let device = self.device.lock().unwrap(); + device.get_manufacturer_string().unwrap_or_default() + } + /// Exchange an APDU with the device. The response data will be written to `answer_buf`, and a /// `APDUAnswer` struct will be created with a reference to `answer_buf`. /// @@ -248,19 +270,19 @@ impl TransportNativeHID { /// If the method errors, the buf may contain a partially written response. It is not advised /// to read this. pub fn exchange(&self, command: &APDUCommand) -> Result { - let answer_buf = { + let answer = { let mut device = self.device.lock().unwrap(); write_apdu(&mut device, LEDGER_CHANNEL, &command.serialize())?; read_response_apdu(&mut device, LEDGER_CHANNEL)? }; - let apdu_answer = APDUAnswer::from_answer(answer_buf)?; + let answer = APDUAnswer::from_answer(answer)?; - match apdu_answer.response_status() { - None => Ok(apdu_answer), + match answer.response_status() { + None => Ok(answer), Some(response) => { if response.is_success() { - Ok(apdu_answer) + Ok(answer) } else { Err(response.into()) } From 9891bd9c09321da0682e3d922a4679e91bab75f1 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 2 Dec 2023 09:50:28 -0800 Subject: [PATCH 8/8] clippy --- ledger/src/transports/native/hid.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ledger/src/transports/native/hid.rs b/ledger/src/transports/native/hid.rs index 71619201..0f2be4a2 100644 --- a/ledger/src/transports/native/hid.rs +++ b/ledger/src/transports/native/hid.rs @@ -77,7 +77,7 @@ fn write_apdu( channel: u16, apdu_command: &[u8], ) -> Result<(), NativeTransportError> { - tracing::debug!(apdu = %hex::encode(&apdu_command), bytes = apdu_command.len(), "Writing APDU to device"); + tracing::debug!(apdu = %hex::encode(apdu_command), bytes = apdu_command.len(), "Writing APDU to device"); let command_length = apdu_command.len(); @@ -103,7 +103,7 @@ fn write_apdu( buffer[6..6 + chunk.len()].copy_from_slice(chunk); tracing::trace!( - buffer = hex::encode(&buffer), + buffer = hex::encode(buffer), sequence_idx, bytes = chunk.len(), "Writing chunk to device",