From cc3156db7a38d57c882e602d0c117b19d8316e24 Mon Sep 17 00:00:00 2001 From: James Prestwich <10149425+prestwich@users.noreply.github.com> Date: Mon, 27 Feb 2023 12:13:55 -0800 Subject: [PATCH] Workaround for https://github.com/LedgerHQ/app-ethereum/issues/409 (#2192) * fix: edge cases and workarounds for ledger signer * feat: add tracing to ledger signer * chore: Changelog --- CHANGELOG.md | 2 + ethers-signers/Cargo.toml | 4 +- ethers-signers/src/ledger/app.rs | 75 +++++++++++++++++++++++------- ethers-signers/src/ledger/types.rs | 19 +++++++- 4 files changed, 81 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 255d604d..7534ccd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -284,6 +284,8 @@ ### Unreleased +- fix: `LedgerSigner` has improved tracing and a ledger app bug mitigation + [#2192](https://github.com/gakonst/ethers-rs/pull/2192) - `eth-keystore-rs` crate updated. Allow an optional name for the to-be-generated keystore file [#910](https://github.com/gakonst/ethers-rs/pull/910) - [1983](https://github.com/gakonst/ethers-rs/pull/1983) Added a `from_bytes` function for the `Wallet` type. diff --git a/ethers-signers/Cargo.toml b/ethers-signers/Cargo.toml index c75ee9e1..505db6a7 100644 --- a/ethers-signers/Cargo.toml +++ b/ethers-signers/Cargo.toml @@ -29,6 +29,7 @@ yubihsm = { version = "0.41.0", features = ["secp256k1", "http", "usb"], optiona futures-util = { version = "^0.3", optional = true } futures-executor = { version = "^0.3", optional = true } semver = { version = "1.0.16", optional = true } +tracing = { version = "0.1.37" } trezor-client = { version = "0.0.7", optional = true, default-features = false, features = [ "f_ethereum", ] } @@ -36,7 +37,6 @@ trezor-client = { version = "0.0.7", optional = true, default-features = false, # aws rusoto_core = { version = "0.48.0", default-features = false, optional = true } rusoto_kms = { version = "0.48.0", default-features = false, optional = true } -tracing = { version = "0.1.37", optional = true } spki = { version = "0.6.0", optional = true } [target.'cfg(not(target_arch = "wasm32"))'.dependencies] @@ -60,5 +60,5 @@ futures = ["futures-util", "futures-executor"] celo = ["ethers-core/celo"] ledger = ["coins-ledger", "futures", "semver"] yubi = ["yubihsm"] -aws = ["rusoto_core/rustls", "rusoto_kms/rustls", "tracing", "spki"] +aws = ["rusoto_core/rustls", "rusoto_kms/rustls", "spki"] trezor = ["trezor-client", "futures", "semver", "home"] diff --git a/ethers-signers/src/ledger/app.rs b/ethers-signers/src/ledger/app.rs index 5932f0b8..05adbc2f 100644 --- a/ethers-signers/src/ledger/app.rs +++ b/ethers-signers/src/ledger/app.rs @@ -29,6 +29,16 @@ pub struct LedgerEthereum { pub(crate) address: Address, } +impl std::fmt::Display for LedgerEthereum { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "LedgerApp. Key at index {} with address {:?} on chain_id {}", + self.derivation, self.address, self.chain_id + ) + } +} + const EIP712_MIN_VERSION: &str = ">=1.6.0"; impl LedgerEthereum { @@ -68,6 +78,7 @@ impl LedgerEthereum { Self::get_address_with_path_transport(&transport, derivation).await } + #[tracing::instrument(skip(transport))] async fn get_address_with_path_transport( transport: &Ledger, derivation: &DerivationType, @@ -82,6 +93,7 @@ impl LedgerEthereum { response_len: None, }; + tracing::debug!("Dispatching get_address request to ethereum app"); let answer = block_on(transport.exchange(&command))?; let result = answer.data().ok_or(LedgerError::UnexpectedNullResponse)?; @@ -93,7 +105,7 @@ impl LedgerEthereum { address.copy_from_slice(&hex::decode(address_str)?); Address::from(address) }; - + tracing::debug!(?address, "Received address from device"); Ok(address) } @@ -109,10 +121,15 @@ impl LedgerEthereum { response_len: None, }; + tracing::debug!("Dispatching get_version"); let answer = block_on(transport.exchange(&command))?; let result = answer.data().ok_or(LedgerError::UnexpectedNullResponse)?; - - Ok(format!("{}.{}.{}", result[1], result[2], result[3])) + if result.len() < 4 { + return Err(LedgerError::ShortResponse { got: result.len(), at_least: 4 }) + } + let version = format!("{}.{}.{}", result[1], result[2], result[3]); + tracing::debug!(version, "Retrieved version from device"); + Ok(version) } /// Signs an Ethereum transaction (requires confirmation on the ledger) @@ -125,7 +142,7 @@ impl LedgerEthereum { let mut payload = Self::path_to_bytes(&self.derivation); payload.extend_from_slice(tx_with_chain.rlp().as_ref()); - let mut signature = self.sign_payload(INS::SIGN, payload).await?; + let mut signature = self.sign_payload(INS::SIGN, &payload).await?; // modify `v` value of signature to match EIP-155 for chains with large chain ID // The logic is derived from Ledger's library @@ -158,7 +175,7 @@ impl LedgerEthereum { payload.extend_from_slice(&(message.len() as u32).to_be_bytes()); payload.extend_from_slice(message); - self.sign_payload(INS::SIGN_PERSONAL_MESSAGE, payload).await + self.sign_payload(INS::SIGN_PERSONAL_MESSAGE, &payload).await } /// Signs an EIP712 encoded domain separator and message @@ -185,16 +202,20 @@ impl LedgerEthereum { payload.extend_from_slice(&domain_separator); payload.extend_from_slice(&struct_hash); - self.sign_payload(INS::SIGN_ETH_EIP_712, payload).await + self.sign_payload(INS::SIGN_ETH_EIP_712, &payload).await } + #[tracing::instrument(err, skip_all, fields(command = %command, payload = hex::encode(payload)))] // Helper function for signing either transaction data, personal messages or EIP712 derived // structs pub async fn sign_payload( &self, command: INS, - mut payload: Vec, + payload: &Vec, ) -> Result { + if payload.is_empty() { + return Err(LedgerError::EmptyPayload) + } let transport = self.transport.lock().await; let mut command = APDUCommand { ins: command as u8, @@ -204,25 +225,47 @@ impl LedgerEthereum { response_len: None, }; - let mut result = Vec::new(); + let mut answer = None; + // workaround for https://github.com/LedgerHQ/app-ethereum/issues/409 + // TODO: remove in future version + let chunk_size = + (0..=255).rev().find(|i| payload.len() % i != 3).expect("true for any length"); // Iterate in 255 byte chunks - while !payload.is_empty() { - let chunk_size = std::cmp::min(payload.len(), 255); - let data = payload.drain(0..chunk_size).collect::>(); - command.data = APDUData::new(&data); + let span = tracing::debug_span!("send_loop", index = 0, chunk = ""); + let guard = span.entered(); + for (index, chunk) in payload.chunks(chunk_size).enumerate() { + guard.record("index", index); + guard.record("chunk", hex::encode(chunk)); + command.data = APDUData::new(chunk); - let answer = block_on(transport.exchange(&command))?; - result = answer.data().ok_or(LedgerError::UnexpectedNullResponse)?.to_vec(); + tracing::debug!("Dispatching packet to device"); + answer = Some(block_on(transport.exchange(&command))?); + + let data = answer.as_ref().expect("just assigned").data(); + if data.is_none() { + return Err(LedgerError::UnexpectedNullResponse) + } + tracing::debug!( + response = hex::encode(data.expect("just checked")), + "Received response from device" + ); // We need more data command.p1 = P1::MORE as u8; } - + drop(guard); + let answer = answer.expect("payload is non-empty, therefore loop ran"); + let result = answer.data().expect("check in loop"); + if result.len() < 65 { + return Err(LedgerError::ShortResponse { got: result.len(), at_least: 65 }) + } let v = result[0] as u64; let r = U256::from_big_endian(&result[1..33]); let s = U256::from_big_endian(&result[33..]); - Ok(Signature { r, s, v }) + let sig = Signature { r, s, v }; + tracing::debug!(sig = %sig, "Received signature from device"); + Ok(sig) } // helper which converts a derivation path to bytes diff --git a/ethers-signers/src/ledger/types.rs b/ethers-signers/src/ledger/types.rs index 73ed6663..b33f6d5c 100644 --- a/ethers-signers/src/ledger/types.rs +++ b/ethers-signers/src/ledger/types.rs @@ -38,7 +38,6 @@ pub enum LedgerError { /// Device response was unexpectedly none #[error("Received unexpected response from device. Expected data in response, found none.")] UnexpectedNullResponse, - #[error(transparent)] /// Error when converting from a hex string HexError(#[from] hex::FromHexError), @@ -51,6 +50,12 @@ pub enum LedgerError { /// Error when signing EIP712 struct with not compatible Ledger ETH app #[error("Ledger ethereum app requires at least version: {0:?}")] UnsupportedAppVersion(String), + /// Got a response, but it didn't contain as much data as expected + #[error("Cannot deserialize ledger response, insufficient bytes. Got {got} expected at least {at_least}")] + ShortResponse { got: usize, at_least: usize }, + /// Payload is empty + #[error("Payload must not be empty")] + EmptyPayload, } pub const P1_FIRST: u8 = 0x00; @@ -66,6 +71,18 @@ pub enum INS { SIGN_ETH_EIP_712 = 0x0C, } +impl std::fmt::Display for INS { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + INS::GET_PUBLIC_KEY => write!(f, "GET_PUBLIC_KEY"), + INS::SIGN => write!(f, "SIGN"), + INS::GET_APP_CONFIGURATION => write!(f, "GET_APP_CONFIGURATION"), + INS::SIGN_PERSONAL_MESSAGE => write!(f, "SIGN_PERSONAL_MESSAGE"), + INS::SIGN_ETH_EIP_712 => write!(f, "SIGN_ETH_EIP_712"), + } + } +} + #[repr(u8)] #[derive(Debug, Copy, Clone, Eq, PartialEq)] #[allow(non_camel_case_types)]