* fix: edge cases and workarounds for ledger signer

* feat: add tracing to ledger signer

* chore: Changelog
This commit is contained in:
James Prestwich 2023-02-27 12:13:55 -08:00 committed by GitHub
parent 9d4b9b5be5
commit cc3156db7a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 81 additions and 19 deletions

View File

@ -284,6 +284,8 @@
### Unreleased ### 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 - `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) 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. - [1983](https://github.com/gakonst/ethers-rs/pull/1983) Added a `from_bytes` function for the `Wallet` type.

View File

@ -29,6 +29,7 @@ yubihsm = { version = "0.41.0", features = ["secp256k1", "http", "usb"], optiona
futures-util = { version = "^0.3", optional = true } futures-util = { version = "^0.3", optional = true }
futures-executor = { version = "^0.3", optional = true } futures-executor = { version = "^0.3", optional = true }
semver = { version = "1.0.16", 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 = [ trezor-client = { version = "0.0.7", optional = true, default-features = false, features = [
"f_ethereum", "f_ethereum",
] } ] }
@ -36,7 +37,6 @@ trezor-client = { version = "0.0.7", optional = true, default-features = false,
# aws # aws
rusoto_core = { version = "0.48.0", default-features = false, optional = true } rusoto_core = { version = "0.48.0", default-features = false, optional = true }
rusoto_kms = { 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 } spki = { version = "0.6.0", optional = true }
[target.'cfg(not(target_arch = "wasm32"))'.dependencies] [target.'cfg(not(target_arch = "wasm32"))'.dependencies]
@ -60,5 +60,5 @@ futures = ["futures-util", "futures-executor"]
celo = ["ethers-core/celo"] celo = ["ethers-core/celo"]
ledger = ["coins-ledger", "futures", "semver"] ledger = ["coins-ledger", "futures", "semver"]
yubi = ["yubihsm"] 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"] trezor = ["trezor-client", "futures", "semver", "home"]

View File

@ -29,6 +29,16 @@ pub struct LedgerEthereum {
pub(crate) address: Address, 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"; const EIP712_MIN_VERSION: &str = ">=1.6.0";
impl LedgerEthereum { impl LedgerEthereum {
@ -68,6 +78,7 @@ impl LedgerEthereum {
Self::get_address_with_path_transport(&transport, derivation).await Self::get_address_with_path_transport(&transport, derivation).await
} }
#[tracing::instrument(skip(transport))]
async fn get_address_with_path_transport( async fn get_address_with_path_transport(
transport: &Ledger, transport: &Ledger,
derivation: &DerivationType, derivation: &DerivationType,
@ -82,6 +93,7 @@ impl LedgerEthereum {
response_len: None, response_len: None,
}; };
tracing::debug!("Dispatching get_address request to ethereum app");
let answer = block_on(transport.exchange(&command))?; let answer = block_on(transport.exchange(&command))?;
let result = answer.data().ok_or(LedgerError::UnexpectedNullResponse)?; let result = answer.data().ok_or(LedgerError::UnexpectedNullResponse)?;
@ -93,7 +105,7 @@ impl LedgerEthereum {
address.copy_from_slice(&hex::decode(address_str)?); address.copy_from_slice(&hex::decode(address_str)?);
Address::from(address) Address::from(address)
}; };
tracing::debug!(?address, "Received address from device");
Ok(address) Ok(address)
} }
@ -109,10 +121,15 @@ impl LedgerEthereum {
response_len: None, response_len: None,
}; };
tracing::debug!("Dispatching get_version");
let answer = block_on(transport.exchange(&command))?; let answer = block_on(transport.exchange(&command))?;
let result = answer.data().ok_or(LedgerError::UnexpectedNullResponse)?; let result = answer.data().ok_or(LedgerError::UnexpectedNullResponse)?;
if result.len() < 4 {
Ok(format!("{}.{}.{}", result[1], result[2], result[3])) 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) /// Signs an Ethereum transaction (requires confirmation on the ledger)
@ -125,7 +142,7 @@ impl LedgerEthereum {
let mut payload = Self::path_to_bytes(&self.derivation); let mut payload = Self::path_to_bytes(&self.derivation);
payload.extend_from_slice(tx_with_chain.rlp().as_ref()); 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 // modify `v` value of signature to match EIP-155 for chains with large chain ID
// The logic is derived from Ledger's library // 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.len() as u32).to_be_bytes());
payload.extend_from_slice(message); 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 /// Signs an EIP712 encoded domain separator and message
@ -185,16 +202,20 @@ impl LedgerEthereum {
payload.extend_from_slice(&domain_separator); payload.extend_from_slice(&domain_separator);
payload.extend_from_slice(&struct_hash); 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 // Helper function for signing either transaction data, personal messages or EIP712 derived
// structs // structs
pub async fn sign_payload( pub async fn sign_payload(
&self, &self,
command: INS, command: INS,
mut payload: Vec<u8>, payload: &Vec<u8>,
) -> Result<Signature, LedgerError> { ) -> Result<Signature, LedgerError> {
if payload.is_empty() {
return Err(LedgerError::EmptyPayload)
}
let transport = self.transport.lock().await; let transport = self.transport.lock().await;
let mut command = APDUCommand { let mut command = APDUCommand {
ins: command as u8, ins: command as u8,
@ -204,25 +225,47 @@ impl LedgerEthereum {
response_len: None, 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 // Iterate in 255 byte chunks
while !payload.is_empty() { let span = tracing::debug_span!("send_loop", index = 0, chunk = "");
let chunk_size = std::cmp::min(payload.len(), 255); let guard = span.entered();
let data = payload.drain(0..chunk_size).collect::<Vec<_>>(); for (index, chunk) in payload.chunks(chunk_size).enumerate() {
command.data = APDUData::new(&data); guard.record("index", index);
guard.record("chunk", hex::encode(chunk));
command.data = APDUData::new(chunk);
let answer = block_on(transport.exchange(&command))?; tracing::debug!("Dispatching packet to device");
result = answer.data().ok_or(LedgerError::UnexpectedNullResponse)?.to_vec(); 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 // We need more data
command.p1 = P1::MORE as u8; 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 v = result[0] as u64;
let r = U256::from_big_endian(&result[1..33]); let r = U256::from_big_endian(&result[1..33]);
let s = U256::from_big_endian(&result[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 // helper which converts a derivation path to bytes

View File

@ -38,7 +38,6 @@ pub enum LedgerError {
/// Device response was unexpectedly none /// Device response was unexpectedly none
#[error("Received unexpected response from device. Expected data in response, found none.")] #[error("Received unexpected response from device. Expected data in response, found none.")]
UnexpectedNullResponse, UnexpectedNullResponse,
#[error(transparent)] #[error(transparent)]
/// Error when converting from a hex string /// Error when converting from a hex string
HexError(#[from] hex::FromHexError), HexError(#[from] hex::FromHexError),
@ -51,6 +50,12 @@ pub enum LedgerError {
/// Error when signing EIP712 struct with not compatible Ledger ETH app /// Error when signing EIP712 struct with not compatible Ledger ETH app
#[error("Ledger ethereum app requires at least version: {0:?}")] #[error("Ledger ethereum app requires at least version: {0:?}")]
UnsupportedAppVersion(String), 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; pub const P1_FIRST: u8 = 0x00;
@ -66,6 +71,18 @@ pub enum INS {
SIGN_ETH_EIP_712 = 0x0C, 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)] #[repr(u8)]
#[derive(Debug, Copy, Clone, Eq, PartialEq)] #[derive(Debug, Copy, Clone, Eq, PartialEq)]
#[allow(non_camel_case_types)] #[allow(non_camel_case_types)]