From e99c788155ddd754c73d2c81b6051dcbd42e6575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Sat, 11 Feb 2017 17:02:00 +0200 Subject: [PATCH] accounts: ledger and HD review fixes - Handle a data race where a Ledger drops between list and open - Prolong Ledger tx confirmation window to 30 days from 1 minute - Simplify Ledger chainid-signature calculation and validation - Simplify Ledger USB APDU request chunking algorithm - Silence keystore account cache notifications for manual actions - Only enable self derivations if wallet open succeeds --- accounts/keystore/account_cache.go | 9 ---- accounts/keystore/account_cache_test.go | 17 +------ accounts/keystore/keystore_test.go | 2 +- accounts/usbwallet/ledger_wallet.go | 66 +++++++++++++------------ cmd/geth/main.go | 5 +- 5 files changed, 39 insertions(+), 60 deletions(-) diff --git a/accounts/keystore/account_cache.go b/accounts/keystore/account_cache.go index e2f826250..3fae3ef5b 100644 --- a/accounts/keystore/account_cache.go +++ b/accounts/keystore/account_cache.go @@ -113,11 +113,6 @@ func (ac *accountCache) add(newAccount accounts.Account) { copy(ac.all[i+1:], ac.all[i:]) ac.all[i] = newAccount ac.byAddr[newAccount.Address] = append(ac.byAddr[newAccount.Address], newAccount) - - select { - case ac.notify <- struct{}{}: - default: - } } // note: removed needs to be unique here (i.e. both File and Address must be set). @@ -131,10 +126,6 @@ func (ac *accountCache) delete(removed accounts.Account) { } else { ac.byAddr[removed.Address] = ba } - select { - case ac.notify <- struct{}{}: - default: - } } func removeAccount(slice []accounts.Account, elem accounts.Account) []accounts.Account { diff --git a/accounts/keystore/account_cache_test.go b/accounts/keystore/account_cache_test.go index 3e68351ea..554196321 100644 --- a/accounts/keystore/account_cache_test.go +++ b/accounts/keystore/account_cache_test.go @@ -140,7 +140,7 @@ func TestCacheInitialReload(t *testing.T) { } func TestCacheAddDeleteOrder(t *testing.T) { - cache, notify := newAccountCache("testdata/no-such-dir") + cache, _ := newAccountCache("testdata/no-such-dir") cache.watcher.running = true // prevent unexpected reloads accs := []accounts.Account{ @@ -176,20 +176,10 @@ func TestCacheAddDeleteOrder(t *testing.T) { for _, a := range accs { cache.add(a) } - select { - case <-notify: - default: - t.Fatalf("notifications didn't fire for adding new accounts") - } // Add some of them twice to check that they don't get reinserted. cache.add(accs[0]) cache.add(accs[2]) - select { - case <-notify: - t.Fatalf("notifications fired for adding existing accounts") - default: - } // Check that the account list is sorted by filename. wantAccounts := make([]accounts.Account, len(accs)) copy(wantAccounts, accs) @@ -213,11 +203,6 @@ func TestCacheAddDeleteOrder(t *testing.T) { } cache.delete(accounts.Account{Address: common.HexToAddress("fd9bd350f08ee3c0c19b85a8e16114a11a60aa4e"), URL: accounts.URL{Scheme: KeyStoreScheme, Path: "something"}}) - select { - case <-notify: - default: - t.Fatalf("notifications didn't fire for deleting accounts") - } // Check content again after deletion. wantAccountsAfterDelete := []accounts.Account{ wantAccounts[1], diff --git a/accounts/keystore/keystore_test.go b/accounts/keystore/keystore_test.go index 1f1935be6..60f2606ee 100644 --- a/accounts/keystore/keystore_test.go +++ b/accounts/keystore/keystore_test.go @@ -286,7 +286,7 @@ func TestWalletNotifications(t *testing.T) { // Randomly add and remove account and make sure events and wallets are in sync live := make(map[common.Address]accounts.Account) - for i := 0; i < 256; i++ { + for i := 0; i < 1024; i++ { // Execute a creation or deletion and ensure event arrival if create := len(live) == 0 || rand.Int()%4 > 0; create { // Add a new account and ensure wallet notifications arrives diff --git a/accounts/usbwallet/ledger_wallet.go b/accounts/usbwallet/ledger_wallet.go index 6044b9caf..a667f580a 100644 --- a/accounts/usbwallet/ledger_wallet.go +++ b/accounts/usbwallet/ledger_wallet.go @@ -192,6 +192,9 @@ func (w *ledgerWallet) Open(passphrase string) error { if err != nil { return err } + if len(devices) == 0 { + return accounts.ErrUnknownWallet + } // Device opened, attach to the input and output endpoints device := devices[0] @@ -767,7 +770,7 @@ func (w *ledgerWallet) ledgerDerive(derivationPath []uint32) (common.Address, er func (w *ledgerWallet) ledgerSign(derivationPath []uint32, address common.Address, tx *types.Transaction, chainID *big.Int) (*types.Transaction, error) { // We need to modify the timeouts to account for user feedback defer func(old time.Duration) { w.device.ReadTimeout = old }(w.device.ReadTimeout) - w.device.ReadTimeout = time.Minute + w.device.ReadTimeout = time.Hour * 24 * 30 // Timeout requires a Ledger power cycle, only if you must // Flatten the derivation path into the Ledger request path := make([]byte, 1+4*len(derivationPath)) @@ -823,7 +826,7 @@ func (w *ledgerWallet) ledgerSign(derivationPath []uint32, address common.Addres signer = new(types.HomesteadSigner) } else { signer = types.NewEIP155Signer(chainID) - signature[64] = (signature[64]-34)/2 - byte(chainID.Uint64()) + signature[64] = signature[64] - byte(chainID.Uint64()*2+35) } // Inject the final signature into the transaction and sanity check the sender signed, err := tx.WithSignature(signer, signature) @@ -875,45 +878,42 @@ func (w *ledgerWallet) ledgerSign(derivationPath []uint32, address common.Addres // Optional APDU data | arbitrary func (w *ledgerWallet) ledgerExchange(opcode ledgerOpcode, p1 ledgerParam1, p2 ledgerParam2, data []byte) ([]byte, error) { // Construct the message payload, possibly split into multiple chunks - var chunks [][]byte - for left := data; len(left) > 0 || len(chunks) == 0; { - // Create the chunk header - var chunk []byte + apdu := make([]byte, 2, 7+len(data)) + + binary.BigEndian.PutUint16(apdu, uint16(5+len(data))) + apdu = append(apdu, []byte{0xe0, byte(opcode), byte(p1), byte(p2), byte(len(data))}...) + apdu = append(apdu, data...) - if len(chunks) == 0 { - // The first chunk encodes the length and all the opcodes - chunk = []byte{0x00, 0x00, 0xe0, byte(opcode), byte(p1), byte(p2), byte(len(data))} - binary.BigEndian.PutUint16(chunk, uint16(5+len(data))) - } - // Append the data blob to the end of the chunk - space := 64 - len(chunk) - 5 // 5 == header size - if len(left) > space { - chunks, left = append(chunks, append(chunk, left[:space]...)), left[space:] - continue - } - chunks, left = append(chunks, append(chunk, left...)), nil - } // Stream all the chunks to the device - for i, chunk := range chunks { + header := []byte{0x01, 0x01, 0x05, 0x00, 0x00} // Channel ID and command tag appended + chunk := make([]byte, 64) + space := len(chunk) - len(header) + + for i := 0; len(apdu) > 0; i++ { // Construct the new message to stream - header := []byte{0x01, 0x01, 0x05, 0x00, 0x00} // Channel ID and command tag appended - binary.BigEndian.PutUint16(header[3:], uint16(i)) - - msg := append(header, chunk...) + chunk = append(chunk[:0], header...) + binary.BigEndian.PutUint16(chunk[3:], uint16(i)) + if len(apdu) > space { + chunk = append(chunk, apdu[:space]...) + apdu = apdu[space:] + } else { + chunk = append(chunk, apdu...) + apdu = nil + } // Send over to the device if glog.V(logger.Detail) { - glog.Infof("-> %03d.%03d: %x", w.device.Bus, w.device.Address, msg) + glog.Infof("-> %03d.%03d: %x", w.device.Bus, w.device.Address, chunk) } - if _, err := w.input.Write(msg); err != nil { + if _, err := w.input.Write(chunk); err != nil { return nil, err } } // Stream the reply back from the wallet in 64 byte chunks var reply []byte + chunk = chunk[:64] // Yeah, we surely have enough space for { // Read the next chunk from the Ledger wallet - chunk := make([]byte, 64) if _, err := io.ReadFull(w.output, chunk); err != nil { return nil, err } @@ -925,17 +925,19 @@ func (w *ledgerWallet) ledgerExchange(opcode ledgerOpcode, p1 ledgerParam1, p2 l return nil, errReplyInvalidHeader } // If it's the first chunk, retrieve the total message length + var payload []byte + if chunk[3] == 0x00 && chunk[4] == 0x00 { reply = make([]byte, 0, int(binary.BigEndian.Uint16(chunk[5:7]))) - chunk = chunk[7:] + payload = chunk[7:] } else { - chunk = chunk[5:] + payload = chunk[5:] } // Append to the reply and stop when filled up - if left := cap(reply) - len(reply); left > len(chunk) { - reply = append(reply, chunk...) + if left := cap(reply) - len(reply); left > len(payload) { + reply = append(reply, payload...) } else { - reply = append(reply, chunk[:left]...) + reply = append(reply, payload[:left]...) break } } diff --git a/cmd/geth/main.go b/cmd/geth/main.go index cc597717e..c19770bfa 100644 --- a/cmd/geth/main.go +++ b/cmd/geth/main.go @@ -273,8 +273,9 @@ func startNode(ctx *cli.Context, stack *node.Node) { for _, wallet := range stack.AccountManager().Wallets() { if err := wallet.Open(""); err != nil { glog.V(logger.Warn).Infof("Failed to open wallet %s: %v", wallet.URL(), err) + } else { + wallet.SelfDerive(accounts.DefaultBaseDerivationPath, stateReader) } - wallet.SelfDerive(accounts.DefaultBaseDerivationPath, stateReader) } // Listen for wallet event till termination for event := range events { @@ -283,8 +284,8 @@ func startNode(ctx *cli.Context, stack *node.Node) { glog.V(logger.Info).Infof("New wallet appeared: %s, failed to open: %s", event.Wallet.URL(), err) } else { glog.V(logger.Info).Infof("New wallet appeared: %s, %s", event.Wallet.URL(), event.Wallet.Status()) + event.Wallet.SelfDerive(accounts.DefaultBaseDerivationPath, stateReader) } - event.Wallet.SelfDerive(accounts.DefaultBaseDerivationPath, stateReader) } else { glog.V(logger.Info).Infof("Old wallet dropped: %s", event.Wallet.URL()) event.Wallet.Close()