Vulnerability: Incorrect Account Used for Signing ### Impact

Anybody using this library to sign with a BIP44 account other than the first account may be affected. If a user is signing with the first account (i.e. the account at index `0`), or with the legacy MEW/MyCrypto HD path, they are not affected.

The vulnerability impacts cases where the user signs a personal message or transaction without first adding the account. This includes cases where the user has already added the account in a previous session (i.e. they added the account, reset the application, then signed something). The serialization/deserialization process does restore a previously added account, but it doesn&#39;t restore the index instructing the keyring to use that account for signing. As a result, after serializing then deserializing the keyring state, the account at index `0` is always used for signing even if it isn&#39;t the current account.

### Patches

This has been patched ([#14](https://github.com/MetaMask/eth-ledger-bridge-keyring/pull/14)) in version &gt;=0.2.1 of [`eth-ledger-bridge-keyring`](https://www.npmjs.com/package/eth-ledger-bridge-keyring), and in version &gt;=0.2.2 of [`@metamask/eth-ledger-bridge-keyring`](https://www.npmjs.com/package/@metamask/eth-ledger-bridge-keyring). Users are encouraged to migrate to the new package name.

### Workarounds

To work around this problem without updating, you should remove then re-add the account before use. As long as the account was added during the lifetime of that process, signing with that account should work correctly.

### For more information

If you have any questions or comments about this advisory:

* Open an issue in [MetaMask/eth-ledger-bridge-keyring on GitHub](https://github.com/MetaMask/eth-ledger-bridge-keyring)
* Email the MetaMask team at [hello@metamask.io](mailto:hello@metamask.io)
Vulnerability ID: GHSA-vg44-fw64-cpjx
CWE: CWE-287
Severity score: 7.5
Project: https://github.com/MetaMask/eth-ledger-bridge-keyring
Published: 2020-03-24
File: index.js
Commit message: Always sign transactions and messages with the correct account (#14)

The account used to sign transactions and messages should be the one
the transaction or message is from. Instead, the last connected account
was being used to sign any messages or transactions.

This was especially problematic considering the last connected account
was not persisted, meaning that signatures were being performed with
the wrong account after a reset (unless the last connected account
happened to be account 0, which was the default).

A mapping of addresses to indexes as been added to the keyring state,
and this mapping has been persisted. This should ensure the correct
account index is used, and thus the correct hd path, each time this
keyring is used for signing.
Commit author: Mark Stacey

Vulnerable version (before fix):
 const NETWORK_API_URLS = { class LedgerBridgeKeyring extends EventEmitter {   constructor (opts = {}) {     super()     this.bridgeUrl = null     this.type = type     this.page = 0 class LedgerBridgeKeyring extends EventEmitter {     return Promise.resolve({       hdPath: this.hdPath,       accounts: this.accounts,       bridgeUrl: this.bridgeUrl,       implementFullBIP44: false,     }) class LedgerBridgeKeyring extends EventEmitter {     this.hdPath = opts.hdPath || hdPathString     this.bridgeUrl = opts.bridgeUrl || BRIDGE_URL     this.accounts = opts.accounts || []     this.implementFullBIP44 = opts.implementFullBIP44 || false     return Promise.resolve()   } class LedgerBridgeKeyring extends EventEmitter {             if (this._isBIP44()) {               const path = this._getPathForIndex(i)               address = await this.unlock(path)             } else {               address = this._addressFromIndex(pathBase, i)             } class LedgerBridgeKeyring extends EventEmitter {       throw new Error(`Address ${address} not found in this keyring`)     }     this.accounts = this.accounts.filter(a => a.toLowerCase() !== address.toLowerCase())   }    // tx is an instance of the ethereumjs-transaction class. class LedgerBridgeKeyring extends EventEmitter {            let hdPath           if (this._isBIP44()) {             hdPath = this._getPathForIndex(this.unlockedAccount)           } else {             hdPath = this._toLedgerPath(this._pathFromAddress(address))           } class LedgerBridgeKeyring extends EventEmitter {         .then(_ => {           let hdPath           if (this._isBIP44()) {             hdPath = this._getPathForIndex(this.unlockedAccount)           } else {             hdPath = this._toLedgerPath(this._pathFromAddress(withAccount))           }

Fixed version (after fix):
 const NETWORK_API_URLS = { class LedgerBridgeKeyring extends EventEmitter {   constructor (opts = {}) {     super()     this.accountIndexes = {}     this.bridgeUrl = null     this.type = type     this.page = 0 class LedgerBridgeKeyring extends EventEmitter {     return Promise.resolve({       hdPath: this.hdPath,       accounts: this.accounts,       accountIndexes: this.accountIndexes,       bridgeUrl: this.bridgeUrl,       implementFullBIP44: false,     }) class LedgerBridgeKeyring extends EventEmitter {     this.hdPath = opts.hdPath || hdPathString     this.bridgeUrl = opts.bridgeUrl || BRIDGE_URL     this.accounts = opts.accounts || []     this.accountIndexes = opts.accountIndexes || {}     this.implementFullBIP44 = opts.implementFullBIP44 || false     return Promise.resolve()   } class LedgerBridgeKeyring extends EventEmitter {             if (this._isBIP44()) {               const path = this._getPathForIndex(i)               address = await this.unlock(path)               this.accountIndexes[ethUtil.toChecksumAddress(address)] = i             } else {               address = this._addressFromIndex(pathBase, i)             } class LedgerBridgeKeyring extends EventEmitter {       throw new Error(`Address ${address} not found in this keyring`)     }     this.accounts = this.accounts.filter(a => a.toLowerCase() !== address.toLowerCase())     delete this.accountIndexes[ethUtil.toChecksumAddress(address)]   }    // tx is an instance of the ethereumjs-transaction class. class LedgerBridgeKeyring extends EventEmitter {            let hdPath           if (this._isBIP44()) {             const checksummedAddress = ethUtil.toChecksumAddress(address)             if (!this.accountIndexes[checksummedAddress]) {               reject(new Error(`Ledger: Index for address '${checksummedAddress}' not found`))             }             hdPath = this._getPathForIndex(this.accountIndexes[checksummedAddress])           } else {             hdPath = this._toLedgerPath(this._pathFromAddress(address))           } class LedgerBridgeKeyring extends EventEmitter {         .then(_ => {           let hdPath           if (this._isBIP44()) {             const checksummedAddress = ethUtil.toChecksumAddress(withAccount)             if (!this.accountIndexes[checksummedAddress]) {               reject(new Error(`Ledger: Index for address '${checksummedAddress}' not found`))             }             hdPath = this._getPathForIndex(this.accountIndexes[checksummedAddress])           } else {             hdPath = this._toLedgerPath(this._pathFromAddress(withAccount))           }

Patch:
patch: @@ -18,6 +18,7 @@ const NETWORK_API_URLS = {
 class LedgerBridgeKeyring extends EventEmitter {
   constructor (opts = {}) {
     super()
+    this.accountIndexes = {}
     this.bridgeUrl = null
     this.type = type
     this.page = 0
@@ -36,6 +37,7 @@ class LedgerBridgeKeyring extends EventEmitter {
     return Promise.resolve({
       hdPath: this.hdPath,
       accounts: this.accounts,
+      accountIndexes: this.accountIndexes,
       bridgeUrl: this.bridgeUrl,
       implementFullBIP44: false,
     })
@@ -45,6 +47,7 @@ class LedgerBridgeKeyring extends EventEmitter {
     this.hdPath = opts.hdPath || hdPathString
     this.bridgeUrl = opts.bridgeUrl || BRIDGE_URL
     this.accounts = opts.accounts || []
+    this.accountIndexes = opts.accountIndexes || {}
     this.implementFullBIP44 = opts.implementFullBIP44 || false
     return Promise.resolve()
   }
@@ -100,6 +103,7 @@ class LedgerBridgeKeyring extends EventEmitter {
             if (this._isBIP44()) {
               const path = this._getPathForIndex(i)
               address = await this.unlock(path)
+              this.accountIndexes[ethUtil.toChecksumAddress(address)] = i
             } else {
               address = this._addressFromIndex(pathBase, i)
             }
@@ -136,6 +140,7 @@ class LedgerBridgeKeyring extends EventEmitter {
       throw new Error(`Address ${address} not found in this keyring`)
     }
     this.accounts = this.accounts.filter(a => a.toLowerCase() !== address.toLowerCase())
+    delete this.accountIndexes[ethUtil.toChecksumAddress(address)]
   }
 
   // tx is an instance of the ethereumjs-transaction class.
@@ -150,7 +155,11 @@ class LedgerBridgeKeyring extends EventEmitter {
 
           let hdPath
           if (this._isBIP44()) {
-            hdPath = this._getPathForIndex(this.unlockedAccount)
+            const checksummedAddress = ethUtil.toChecksumAddress(address)
+            if (!this.accountIndexes[checksummedAddress]) {
+              reject(new Error(`Ledger: Index for address '${checksummedAddress}' not found`))
+            }
+            hdPath = this._getPathForIndex(this.accountIndexes[checksummedAddress])
           } else {
             hdPath = this._toLedgerPath(this._pathFromAddress(address))
           }
@@ -195,7 +204,11 @@ class LedgerBridgeKeyring extends EventEmitter {
         .then(_ => {
           let hdPath
           if (this._isBIP44()) {
-            hdPath = this._getPathForIndex(this.unlockedAccount)
+            const checksummedAddress = ethUtil.toChecksumAddress(withAccount)
+            if (!this.accountIndexes[checksummedAddress]) {
+              reject(new Error(`Ledger: Index for address '${checksummedAddress}' not found`))
+            }
+            hdPath = this._getPathForIndex(this.accountIndexes[checksummedAddress])
           } else {
             hdPath = this._toLedgerPath(this._pathFromAddress(withAccount))
           }
path: index.js