From 6445f12c2da974de3ea9dfc3385ca9a397e4c6fd Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Tue, 27 Feb 2018 16:45:16 +0100 Subject: [PATCH] Hardware-wallet/usb-subscribe-refactor (#7860) * Hardware-wallet fix * More fine-grained initilization of callbacks by vendorID, productID and usb class * Each device manufacturer gets a seperate handle thread each * Replaced "dummy for loop" with a delay to wait for the device to boot-up properly * Haven't been very carefully with checking dependencies cycles etc * Inline comments explaining where shortcuts have been taken * Need to test this on Windows machine and with Ledger (both models) Signed-off-by: niklasad1 * Validate product_id of detected ledger devices * closed_device => unlocked_device * address comments * add target in debug * Address feedback * Remove thread joining in HardwareWalletManager * Remove thread handlers in HardwareWalletManager because this makes them unused --- hw/src/ledger.rs | 82 ++++++++++++++++++++++++++++------ hw/src/lib.rs | 114 +++++++++++++++++++++++------------------------ hw/src/trezor.rs | 81 ++++++++++++++++++++++----------- 3 files changed, 180 insertions(+), 97 deletions(-) diff --git a/hw/src/ledger.rs b/hw/src/ledger.rs index f2c2c0ec5..130149c47 100644 --- a/hw/src/ledger.rs +++ b/hw/src/ledger.rs @@ -20,18 +20,23 @@ use std::cmp::min; use std::fmt; use std::str::FromStr; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use std::time::Duration; +use std::thread; use ethereum_types::{H256, Address}; use ethkey::Signature; use hidapi; +use libusb; use parking_lot::{Mutex, RwLock}; use super::{WalletInfo, KeyPath}; -const LEDGER_VID: u16 = 0x2c97; -const LEDGER_PIDS: [u16; 2] = [0x0000, 0x0001]; // Nano S and Blue +/// Ledger vendor ID +pub const LEDGER_VID: u16 = 0x2c97; +/// Legder product IDs: [Nano S and Blue] +pub const LEDGER_PIDS: [u16; 2] = [0x0000, 0x0001]; + const ETH_DERIVATION_PATH_BE: [u8; 17] = [4, 0x80, 0, 0, 44, 0x80, 0, 0, 60, 0x80, 0, 0, 0, 0, 0, 0, 0]; // 44'/60'/0'/0 const ETC_DERIVATION_PATH_BE: [u8; 21] = [5, 0x80, 0, 0, 44, 0x80, 0, 0, 60, 0x80, 0x02, 0x73, 0xd0, 0x80, 0, 0, 0, 0, 0, 0, 0]; // 44'/60'/160720'/0'/0 @@ -54,10 +59,14 @@ pub enum Error { Protocol(&'static str), /// Hidapi error. Usb(hidapi::HidError), + /// Libusb error + LibUsb(libusb::Error), /// Device with request key is not available. KeyNotFound, /// Signing has been cancelled by user. UserCancel, + /// Invalid Device + InvalidDevice, } impl fmt::Display for Error { @@ -65,8 +74,10 @@ impl fmt::Display for Error { match *self { Error::Protocol(ref s) => write!(f, "Ledger protocol error: {}", s), Error::Usb(ref e) => write!(f, "USB communication error: {}", e), + Error::LibUsb(ref e) => write!(f, "LibUSB communication error: {}", e), Error::KeyNotFound => write!(f, "Key not found"), Error::UserCancel => write!(f, "Operation has been cancelled"), + Error::InvalidDevice => write!(f, "Unsupported product was entered"), } } } @@ -77,6 +88,12 @@ impl From for Error { } } +impl From for Error { + fn from(err: libusb::Error) -> Error { + Error::LibUsb(err) + } +} + /// Ledger device manager. pub struct Manager { usb: Arc>, @@ -234,16 +251,7 @@ impl Manager { fn open_path(&self, f: F) -> Result where F: Fn() -> Result { - let mut err = Error::KeyNotFound; - // Try to open device a few times. - for _ in 0..10 { - match f() { - Ok(handle) => return Ok(handle), - Err(e) => err = From::from(e), - } - ::std::thread::sleep(Duration::from_millis(200)); - } - Err(err) + f().map_err(Into::into) } fn send_apdu(handle: &hidapi::HidDevice, command: u8, p1: u8, p2: u8, data: &[u8]) -> Result, Error> { @@ -333,6 +341,54 @@ impl Manager { message.truncate(new_len); Ok(message) } + + fn is_valid_ledger(device: &libusb::Device) -> Result<(), Error> { + let desc = device.device_descriptor()?; + let vendor_id = desc.vendor_id(); + let product_id = desc.product_id(); + + if vendor_id == LEDGER_VID && LEDGER_PIDS.contains(&product_id) { + Ok(()) + } else { + Err(Error::InvalidDevice) + } + } + +} + +/// Ledger event handler +/// A seperate thread is handling incoming events +pub struct EventHandler { + ledger: Weak, +} + +impl EventHandler { + /// Ledger event handler constructor + pub fn new(ledger: Weak) -> Self { + Self { ledger: ledger } + } +} + +impl libusb::Hotplug for EventHandler { + fn device_arrived(&mut self, device: libusb::Device) { + if let (Some(ledger), Ok(_)) = (self.ledger.upgrade(), Manager::is_valid_ledger(&device)) { + debug!(target: "hw", "Ledger arrived"); + // Wait for the device to boot up + thread::sleep(Duration::from_millis(1000)); + if let Err(e) = ledger.update_devices() { + debug!(target: "hw", "Ledger connect error: {:?}", e); + } + } + } + + fn device_left(&mut self, device: libusb::Device) { + if let (Some(ledger), Ok(_)) = (self.ledger.upgrade(), Manager::is_valid_ledger(&device)) { + debug!(target: "hw", "Ledger left"); + if let Err(e) = ledger.update_devices() { + debug!(target: "hw", "Ledger disconnect error: {:?}", e); + } + } + } } #[test] diff --git a/hw/src/lib.rs b/hw/src/lib.rs index a1e90ea3e..828c15d70 100644 --- a/hw/src/lib.rs +++ b/hw/src/lib.rs @@ -33,13 +33,15 @@ use ethkey::{Address, Signature}; use parking_lot::Mutex; use std::fmt; -use std::sync::{Arc, Weak}; +use std::sync::Arc; use std::sync::atomic; use std::sync::atomic::AtomicBool; use std::thread; use std::time::Duration; use ethereum_types::U256; +const USB_DEVICE_CLASS_DEVICE: u8 = 0; + /// Hardware wallet error. #[derive(Debug)] pub enum Error { @@ -128,84 +130,78 @@ impl From for Error { /// Hardware wallet management interface. pub struct HardwareWalletManager { - update_thread: Option>, exiting: Arc, ledger: Arc, trezor: Arc, } -struct EventHandler { - ledger: Weak, - trezor: Weak, -} - -impl libusb::Hotplug for EventHandler { - fn device_arrived(&mut self, _device: libusb::Device) { - debug!("USB Device arrived"); - if let (Some(l), Some(t)) = (self.ledger.upgrade(), self.trezor.upgrade()) { - for _ in 0..10 { - let l_devices = l.update_devices().unwrap_or_else(|e| { - debug!("Error enumerating Ledger devices: {}", e); - 0 - }); - let t_devices = t.update_devices().unwrap_or_else(|e| { - debug!("Error enumerating Trezor devices: {}", e); - 0 - }); - if l_devices + t_devices > 0 { - break; - } - thread::sleep(Duration::from_millis(200)); - } - } - } - - fn device_left(&mut self, _device: libusb::Device) { - debug!("USB Device lost"); - if let (Some(l), Some(t)) = (self.ledger.upgrade(), self.trezor.upgrade()) { - l.update_devices().unwrap_or_else(|e| {debug!("Error enumerating Ledger devices: {}", e); 0}); - t.update_devices().unwrap_or_else(|e| {debug!("Error enumerating Trezor devices: {}", e); 0}); - } - } -} impl HardwareWalletManager { + /// Hardware wallet constructor pub fn new() -> Result { - let usb_context = Arc::new(libusb::Context::new()?); + let usb_context_trezor = Arc::new(libusb::Context::new()?); + let usb_context_ledger = Arc::new(libusb::Context::new()?); let hidapi = Arc::new(Mutex::new(hidapi::HidApi::new().map_err(|e| Error::Hid(e.to_string().clone()))?)); let ledger = Arc::new(ledger::Manager::new(hidapi.clone())); let trezor = Arc::new(trezor::Manager::new(hidapi.clone())); - usb_context.register_callback( - None, None, None, - Box::new(EventHandler { - ledger: Arc::downgrade(&ledger), - trezor: Arc::downgrade(&trezor), - }), - )?; + + // Subscribe to TREZOR V1 + // Note, this support only TREZOR V1 becasue TREZOR V2 has another vendorID for some reason + // Also, we now only support one product as the second argument specifies + usb_context_trezor.register_callback( + Some(trezor::TREZOR_VID), Some(trezor::TREZOR_PIDS[0]), Some(USB_DEVICE_CLASS_DEVICE), + Box::new(trezor::EventHandler::new(Arc::downgrade(&trezor))))?; + + // Subscribe to all Ledger Devices + // This means that we need to check that the given productID is supported + // None => LIBUSB_HOTPLUG_MATCH_ANY, in other words that all are subscribed to + // More info can be found: http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea + usb_context_ledger.register_callback( + Some(ledger::LEDGER_VID), None, Some(USB_DEVICE_CLASS_DEVICE), + Box::new(ledger::EventHandler::new(Arc::downgrade(&ledger))))?; + let exiting = Arc::new(AtomicBool::new(false)); - let thread_exiting = exiting.clone(); + let thread_exiting_ledger = exiting.clone(); + let thread_exiting_trezor = exiting.clone(); let l = ledger.clone(); let t = trezor.clone(); - let thread = thread::Builder::new() - .name("hw_wallet".to_string()) + + // Ledger event thread + thread::Builder::new() + .name("hw_wallet_ledger".to_string()) .spawn(move || { if let Err(e) = l.update_devices() { - debug!("Error updating ledger devices: {}", e); - } - if let Err(e) = t.update_devices() { - debug!("Error updating trezor devices: {}", e); + debug!(target: "hw", "Ledger couldn't connect at startup, error: {}", e); + //debug!("Ledger could not connect at startup, error: {}", e); } loop { - usb_context.handle_events(Some(Duration::from_millis(500))) - .unwrap_or_else(|e| debug!("Error processing USB events: {}", e)); - if thread_exiting.load(atomic::Ordering::Acquire) { + usb_context_ledger.handle_events(Some(Duration::from_millis(500))) + .unwrap_or_else(|e| debug!(target: "hw", "Ledger event handler error: {}", e)); + if thread_exiting_ledger.load(atomic::Ordering::Acquire) { break; } } }) .ok(); + + // Trezor event thread + thread::Builder::new() + .name("hw_wallet_trezor".to_string()) + .spawn(move || { + if let Err(e) = t.update_devices() { + debug!(target: "hw", "Trezor couldn't connect at startup, error: {}", e); + } + loop { + usb_context_trezor.handle_events(Some(Duration::from_millis(500))) + .unwrap_or_else(|e| debug!(target: "hw", "Trezor event handler error: {}", e)); + if thread_exiting_trezor.load(atomic::Ordering::Acquire) { + break; + } + } + }) + .ok(); + Ok(HardwareWalletManager { - update_thread: thread, exiting: exiting, ledger: ledger, trezor: trezor, @@ -259,10 +255,10 @@ impl HardwareWalletManager { impl Drop for HardwareWalletManager { fn drop(&mut self) { + // Indicate to the USB Hotplug handlers that they + // shall terminate but don't wait for them to terminate. + // If they don't terminate for some reason USB Hotplug events will be handled + // even if the HardwareWalletManger has been dropped self.exiting.store(true, atomic::Ordering::Release); - if let Some(thread) = self.update_thread.take() { - thread.thread().unpark(); - thread.join().ok(); - } } } diff --git a/hw/src/trezor.rs b/hw/src/trezor.rs index 1cde53402..4105bb7b5 100644 --- a/hw/src/trezor.rs +++ b/hw/src/trezor.rs @@ -23,25 +23,29 @@ use super::{WalletInfo, TransactionInfo, KeyPath}; use std::cmp::{min, max}; use std::fmt; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use std::time::Duration; +use std::thread; use ethereum_types::{U256, H256, Address}; use ethkey::Signature; use hidapi; +use libusb; use parking_lot::{Mutex, RwLock}; use protobuf; use protobuf::{Message, ProtobufEnum}; use trezor_sys::messages::{EthereumAddress, PinMatrixAck, MessageType, EthereumTxRequest, EthereumSignTx, EthereumGetAddress, EthereumTxAck, ButtonAck}; -const TREZOR_VID: u16 = 0x534c; -const TREZOR_PIDS: [u16; 1] = [0x0001]; // Trezor v1, keeping this as an array to leave room for Trezor v2 which is in progress +/// Trezor v1 vendor ID +pub const TREZOR_VID: u16 = 0x534c; +/// Trezor product IDs +pub const TREZOR_PIDS: [u16; 1] = [0x0001]; + const ETH_DERIVATION_PATH: [u32; 5] = [0x8000002C, 0x8000003C, 0x80000000, 0, 0]; // m/44'/60'/0'/0/0 const ETC_DERIVATION_PATH: [u32; 5] = [0x8000002C, 0x8000003D, 0x80000000, 0, 0]; // m/44'/61'/0'/0/0 - /// Hardware wallet error. #[derive(Debug)] pub enum Error { @@ -56,7 +60,7 @@ pub enum Error { /// The Message Type given in the trezor RPC call is not something we recognize BadMessageType, /// Trying to read from a closed device at the given path - ClosedDevice(String), + LockedDevice(String), } impl fmt::Display for Error { @@ -67,7 +71,7 @@ impl fmt::Display for Error { Error::KeyNotFound => write!(f, "Key not found"), Error::UserCancel => write!(f, "Operation has been cancelled"), Error::BadMessageType => write!(f, "Bad Message Type in RPC call"), - Error::ClosedDevice(ref s) => write!(f, "Device is closed, needs PIN to perform operations: {}", s), + Error::LockedDevice(ref s) => write!(f, "Device is locked, needs PIN to perform operations: {}", s), } } } @@ -84,11 +88,11 @@ impl From for Error { } } -/// Ledger device manager. +/// Ledger device manager pub struct Manager { usb: Arc>, devices: RwLock>, - closed_devices: RwLock>, + locked_devices: RwLock>, key_path: RwLock, } @@ -110,7 +114,7 @@ impl Manager { Manager { usb: hidapi, devices: RwLock::new(Vec::new()), - closed_devices: RwLock::new(Vec::new()), + locked_devices: RwLock::new(Vec::new()), key_path: RwLock::new(KeyPath::Ethereum), } } @@ -121,7 +125,7 @@ impl Manager { usb.refresh_devices(); let devices = usb.devices(); let mut new_devices = Vec::new(); - let mut closed_devices = Vec::new(); + let mut locked_devices = Vec::new(); let mut error = None; for usb_device in devices { let is_trezor = usb_device.vendor_id == TREZOR_VID; @@ -140,7 +144,7 @@ impl Manager { } match self.read_device_info(&usb, &usb_device) { Ok(device) => new_devices.push(device), - Err(Error::ClosedDevice(path)) => closed_devices.push(path.to_string()), + Err(Error::LockedDevice(path)) => locked_devices.push(path.to_string()), Err(e) => { warn!("Error reading device: {:?}", e); error = Some(e); @@ -148,9 +152,9 @@ impl Manager { } } let count = new_devices.len(); - trace!("Got devices: {:?}, closed: {:?}", new_devices, closed_devices); + trace!("Got devices: {:?}, closed: {:?}", new_devices, locked_devices); *self.devices.write() = new_devices; - *self.closed_devices.write() = closed_devices; + *self.locked_devices.write() = locked_devices; match error { Some(e) => Err(e), None => Ok(count), @@ -174,7 +178,7 @@ impl Manager { }, }) } - Ok(None) => Err(Error::ClosedDevice(dev_info.path.clone())), + Ok(None) => Err(Error::LockedDevice(dev_info.path.clone())), Err(e) => Err(e), } } @@ -190,7 +194,7 @@ impl Manager { } pub fn list_locked_devices(&self) -> Vec { - (*self.closed_devices.read()).clone() + (*self.locked_devices.read()).clone() } /// Get wallet info. @@ -201,16 +205,7 @@ impl Manager { fn open_path(&self, f: F) -> Result where F: Fn() -> Result { - let mut err = Error::KeyNotFound; - // Try to open device a few times. - for _ in 0..10 { - match f() { - Ok(handle) => return Ok(handle), - Err(e) => err = From::from(e), - } - ::std::thread::sleep(Duration::from_millis(200)); - } - Err(err) + f().map_err(Into::into) } pub fn pin_matrix_ack(&self, device_path: &str, pin: &str) -> Result { @@ -407,6 +402,42 @@ impl Manager { } } +/// Trezor event handler +/// A separate thread is handeling incoming events +pub struct EventHandler { + trezor: Weak, +} + +impl EventHandler { + // Trezor event handler constructor + pub fn new(trezor: Weak) -> Self { + Self { trezor: trezor } + } +} + +impl libusb::Hotplug for EventHandler { + fn device_arrived(&mut self, _device: libusb::Device) { + debug!(target: "hw", "Trezor V1 arrived"); + if let Some(trezor) = self.trezor.upgrade() { + // Wait for the device to boot up + thread::sleep(Duration::from_millis(1000)); + if let Err(e) = trezor.update_devices() { + debug!(target: "hw", "Trezor V1 connect error: {:?}", e); + } + + } + } + + fn device_left(&mut self, _device: libusb::Device) { + debug!(target: "hw", "Trezor V1 left"); + if let Some(trezor) = self.trezor.upgrade() { + if let Err(e) = trezor.update_devices() { + debug!(target: "hw", "Trezor V1 disconnect error: {:?}", e); + } + } + } +} + #[test] #[ignore] /// This test can't be run without an actual trezor device connected