Workaround for LedgerHQ/app-ethereum#409 (#2192)

* fix: edge cases and workarounds for ledger signer * feat: add tracing to ledger signer * chore: Changelog
gakonst · Feb 27, 2023 · cc3156d · cc3156d
1 parent 9d4b9b5
commit cc3156d
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 20 deletions.
diff --git 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
@@ -29,14 +29,14 @@ 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",
 ] }
 
 # 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
@@ -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<u8>,
+        payload: &Vec<u8>,
     ) -> Result<Signature, LedgerError> {
+        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::<Vec<_>>();
-            command.data = APDUData::new(&data);
-
-            let answer = block_on(transport.exchange(&command))?;
-            result = answer.data().ok_or(LedgerError::UnexpectedNullResponse)?.to_vec();
+        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);
+
+            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
@@ -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)]