diff --git a/waddrmgr/address.go b/waddrmgr/address.go index 49abea7659..3ff07a33c9 100644 --- a/waddrmgr/address.go +++ b/waddrmgr/address.go @@ -401,7 +401,7 @@ func (a *managedAddress) PrivKey() (*btcec.PrivateKey, error) { // Decrypt the key as needed. Also, make sure it's a copy since the // private key stored in memory can be cleared at any time. Otherwise // the returned private key could be invalidated from under the caller. - privKeyCopy, err := a.unlock(a.manager.rootManager.cryptoKeyPriv) + privKeyCopy, err := a.unlock(a.manager.rootManager.CryptoKeyPriv()) if err != nil { return nil, err } @@ -421,7 +421,14 @@ func (a *managedAddress) ExportPrivKey() (*btcutil.WIF, error) { return nil, err } - return btcutil.NewWIF(pk, a.manager.rootManager.chainParams, a.compressed) + wif, err := btcutil.NewWIF( + pk, a.manager.rootManager.ChainParams(), a.compressed, + ) + if err != nil { + return nil, fmt.Errorf("create WIF: %w", err) + } + + return wif, nil } // DerivationInfo contains the information required to derive the key that @@ -559,7 +566,7 @@ func newManagedAddressWithoutPrivKey(m *ScopedKeyManager, // First, we'll generate a normal p2wkh address from the pubkey hash. witAddr, err := btcutil.NewAddressWitnessPubKeyHash( - pubKeyHash, m.rootManager.chainParams, + pubKeyHash, m.rootManager.ChainParams(), ) if err != nil { return nil, err @@ -577,7 +584,7 @@ func newManagedAddressWithoutPrivKey(m *ScopedKeyManager, // witnessProgram as the sigScript, then present the proper // pair as the witness. address, err = btcutil.NewAddressScriptHash( - witnessProgram, m.rootManager.chainParams, + witnessProgram, m.rootManager.ChainParams(), ) if err != nil { return nil, err @@ -585,7 +592,7 @@ func newManagedAddressWithoutPrivKey(m *ScopedKeyManager, case PubKeyHash: address, err = btcutil.NewAddressPubKeyHash( - pubKeyHash, m.rootManager.chainParams, + pubKeyHash, m.rootManager.ChainParams(), ) if err != nil { return nil, err @@ -593,7 +600,7 @@ func newManagedAddressWithoutPrivKey(m *ScopedKeyManager, case WitnessPubKey: address, err = btcutil.NewAddressWitnessPubKeyHash( - pubKeyHash, m.rootManager.chainParams, + pubKeyHash, m.rootManager.ChainParams(), ) if err != nil { return nil, err @@ -602,7 +609,7 @@ func newManagedAddressWithoutPrivKey(m *ScopedKeyManager, case TaprootPubKey: tapKey := txscript.ComputeTaprootKeyNoScript(pubKey) address, err = btcutil.NewAddressTaproot( - schnorr.SerializePubKey(tapKey), m.rootManager.chainParams, + schnorr.SerializePubKey(tapKey), m.rootManager.ChainParams(), ) if err != nil { return nil, err @@ -635,7 +642,10 @@ func newManagedAddress(s *ScopedKeyManager, derivationPath DerivationPath, // NOTE: The privKeyBytes here are set into the managed address which // are cleared when locked, so they aren't cleared here. privKeyBytes := privKey.Serialize() - privKeyEncrypted, err := s.rootManager.cryptoKeyPriv.Encrypt(privKeyBytes) + + privKeyEncrypted, err := s.rootManager.CryptoKeyPriv().Encrypt( + privKeyBytes, + ) if err != nil { str := "failed to encrypt private key" return nil, managerError(ErrCrypto, str, err) @@ -894,7 +904,7 @@ func (a *scriptAddress) Script() ([]byte, error) { // Decrypt the script as needed. Also, make sure it's a copy since the // script stored in memory can be cleared at any time. Otherwise, // the returned script could be invalidated from under the caller. - return a.unlock(a.manager.rootManager.cryptoKeyScript) + return a.unlock(a.manager.rootManager.CryptoKeyScript()) } // Enforce scriptAddress satisfies the ManagedScriptAddress interface. @@ -905,7 +915,7 @@ func newScriptAddress(m *ScopedKeyManager, account uint32, scriptHash, scriptEncrypted []byte) (*scriptAddress, error) { address, err := btcutil.NewAddressScriptHashFromHash( - scriptHash, m.rootManager.chainParams, + scriptHash, m.rootManager.ChainParams(), ) if err != nil { return nil, err @@ -989,9 +999,9 @@ func (a *witnessScriptAddress) Script() ([]byte, error) { return nil, managerError(ErrLocked, errLocked, nil) } - cryptoKey := a.manager.rootManager.cryptoKeyScript + cryptoKey := a.manager.rootManager.CryptoKeyScript() if !a.isSecretScript { - cryptoKey = a.manager.rootManager.cryptoKeyPub + cryptoKey = a.manager.rootManager.CryptoKeyPub() } // Decrypt the script as needed. Also, make sure it's a copy since the @@ -1012,7 +1022,7 @@ func newWitnessScriptAddress(m *ScopedKeyManager, account uint32, scriptIdent, switch witnessVersion { case witnessVersionV0: address, err := btcutil.NewAddressWitnessScriptHash( - scriptIdent, m.rootManager.chainParams, + scriptIdent, m.rootManager.ChainParams(), ) if err != nil { return nil, err @@ -1031,7 +1041,7 @@ func newWitnessScriptAddress(m *ScopedKeyManager, account uint32, scriptIdent, case witnessVersionV1: address, err := btcutil.NewAddressTaproot( - scriptIdent, m.rootManager.chainParams, + scriptIdent, m.rootManager.ChainParams(), ) if err != nil { return nil, err diff --git a/waddrmgr/manager.go b/waddrmgr/manager.go index 36a6f4ad62..e5981f1d95 100644 --- a/waddrmgr/manager.go +++ b/waddrmgr/manager.go @@ -345,6 +345,7 @@ type Manager struct { externalAddrSchemas map[AddressType][]KeyScope internalAddrSchemas map[AddressType][]KeyScope + syncStateMtx sync.RWMutex syncState syncState watchingOnly atomic.Bool birthday time.Time @@ -426,42 +427,25 @@ func (m *Manager) IsWatchOnlyAccount(ns walletdb.ReadBucket, keyScope KeyScope, // // TODO(yy): Rename this to wipe memory for priv keys. func (m *Manager) lock() { - for _, manager := range m.scopedManagers { - // Clear all of the account private keys. - for _, acctInfo := range manager.acctInfo { - if acctInfo.acctKeyPriv != nil { - acctInfo.acctKeyPriv.Zero() - } - acctInfo.acctKeyPriv = nil - } - } + // Mark locked first so callers checking IsLocked() see the locked + // state before we begin zeroing keys. + m.locked.Store(true) - // Remove clear text private keys and scripts from all address entries. - for _, manager := range m.scopedManagers { - for _, ma := range manager.addrs { - switch addr := ma.(type) { - case *managedAddress: - addr.lock() - case *scriptAddress: - addr.lock() - } - } + // Zero private key material in each scoped manager under s.mtx. + for _, sm := range m.scopedManagers { + sm.Lock() } - // Remove clear text private master and crypto keys from memory. + // Zero manager-level crypto keys. m.cryptoKeyScript.Zero() m.cryptoKeyPriv.Zero() m.masterKeyPriv.Zero() - - // Zero the hashed passphrase. zero.Bytea64(&m.hashedPrivPassphrase) // NOTE: m.cryptoKeyPub is intentionally not cleared here as the address // manager needs to be able to continue to read and decrypt public data // which uses a separate derived key from the database even when it is // locked. - - m.locked.Store(true) } // Close cleanly shuts down the manager. It makes a best try effort to remove @@ -878,6 +862,47 @@ func (m *Manager) ChainParams() *chaincfg.Params { return m.chainParams } +// CryptoKeyPub returns the public crypto key used to encrypt/decrypt +// public extended keys and addresses. +func (m *Manager) CryptoKeyPub() EncryptorDecryptor { //nolint:ireturn + return m.cryptoKeyPub +} + +// CryptoKeyPriv returns the private crypto key used to +// encrypt/decrypt private key material. The underlying key is zeroed +// when the manager is locked. +func (m *Manager) CryptoKeyPriv() EncryptorDecryptor { //nolint:ireturn + return m.cryptoKeyPriv +} + +// CryptoKeyScript returns the script crypto key used to +// encrypt/decrypt script data. The underlying key is zeroed when the +// manager is locked. +func (m *Manager) CryptoKeyScript() EncryptorDecryptor { //nolint:ireturn + return m.cryptoKeyScript +} + +// SetStartBlock updates the start block if the given block is earlier +// than the current start block. It persists the change to the database +// and updates the in-memory state atomically. +func (m *Manager) SetStartBlock(ns walletdb.ReadWriteBucket, + bs *BlockStamp) error { + + m.syncStateMtx.Lock() + defer m.syncStateMtx.Unlock() + + if bs.Height < m.syncState.startBlock.Height { + err := putStartBlock(ns, bs) + if err != nil { + return err + } + + m.syncState.startBlock = *bs + } + + return nil +} + // ChangePassphrase changes either the public or private passphrase to the // provided value depending on the private flag. In order to change the // private password, the address manager must not be watching-only. The new @@ -1076,32 +1101,9 @@ func (m *Manager) ConvertToWatchingOnly(ns walletdb.ReadWriteBucket) error { m.lock() } - // This section clears and removes the encrypted private key material - // that is ordinarily used to unlock the manager. Since the the manager - // is being converted to watching-only, the encrypted private key - // material is no longer needed. - - // Clear and remove all of the encrypted acount private keys. + // Clear encrypted private key material from each scoped manager. for _, manager := range m.scopedManagers { - for _, acctInfo := range manager.acctInfo { - zero.Bytes(acctInfo.acctKeyEncrypted) - acctInfo.acctKeyEncrypted = nil - } - } - - // Clear and remove encrypted private keys and encrypted scripts from - // all address entries. - for _, manager := range m.scopedManagers { - for _, ma := range manager.addrs { - switch addr := ma.(type) { - case *managedAddress: - zero.Bytes(addr.privKeyEncrypted) - addr.privKeyEncrypted = nil - case *scriptAddress: - zero.Bytes(addr.scriptEncrypted) - addr.scriptEncrypted = nil - } - } + manager.ConvertToWatchingOnly() } // Clear and remove encrypted private and script crypto keys. @@ -1210,64 +1212,11 @@ func (m *Manager) Unlock(ns walletdb.ReadBucket, passphrase []byte) error { // Use the crypto private key to decrypt all of the account private // extended keys. for _, manager := range m.scopedManagers { - for account, acctInfo := range manager.acctInfo { - decrypted, err := m.cryptoKeyPriv.Decrypt(acctInfo.acctKeyEncrypted) - if err != nil { - m.lock() - str := fmt.Sprintf("failed to decrypt account %d "+ - "private key", account) - return managerError(ErrCrypto, str, err) - } - - acctKeyPriv, err := hdkeychain.NewKeyFromString(string(decrypted)) - zero.Bytes(decrypted) - if err != nil { - m.lock() - str := fmt.Sprintf("failed to regenerate account %d "+ - "extended key", account) - return managerError(ErrKeyChain, str, err) - } - acctInfo.acctKeyPriv = acctKeyPriv - } - - // We'll also derive any private keys that are pending due to - // them being created while the address manager was locked. - for _, info := range manager.deriveOnUnlock { - addressKey, _, _, err := manager.deriveKeyFromPath( - ns, info.managedAddr.InternalAccount(), - info.branch, info.index, true, - ) - if err != nil { - m.lock() - return err - } - - // It's ok to ignore the error here since it can only - // fail if the extended key is not private, however it - // was just derived as a private key. - privKey, _ := addressKey.ECPrivKey() - addressKey.Zero() - - privKeyBytes := privKey.Serialize() - privKeyEncrypted, err := m.cryptoKeyPriv.Encrypt(privKeyBytes) - privKey.Zero() - if err != nil { - m.lock() - str := fmt.Sprintf("failed to encrypt private key for "+ - "address %s", info.managedAddr.Address()) - return managerError(ErrCrypto, str, err) - } - - switch a := info.managedAddr.(type) { - case *managedAddress: - a.privKeyEncrypted = privKeyEncrypted - a.privKeyCT = privKeyBytes - case *scriptAddress: - } + err := manager.Unlock(m.cryptoKeyPriv, ns) + if err != nil { + m.lock() - // Avoid re-deriving this key on subsequent unlocks. - manager.deriveOnUnlock[0] = nil - manager.deriveOnUnlock = manager.deriveOnUnlock[1:] + return err } } diff --git a/waddrmgr/manager_test.go b/waddrmgr/manager_test.go index f0a5d8af93..563698d05b 100644 --- a/waddrmgr/manager_test.go +++ b/waddrmgr/manager_test.go @@ -12,6 +12,7 @@ import ( "fmt" "os" "reflect" + "sync" "testing" "time" @@ -3294,3 +3295,58 @@ func TestManagedAddressValidation(t *testing.T) { } } + +// TestLockUnlockRace verifies there is no data race between Manager.Lock() +// and concurrent ScopedKeyManager.NextExternalAddresses calls. +func TestLockUnlockRace(t *testing.T) { + t.Parallel() + + teardown, db, mgr := setupManager(t) + defer teardown() + + err := walletdb.View(db, func(tx walletdb.ReadTx) error { + ns := tx.ReadBucket(waddrmgrNamespaceKey) + + return mgr.Unlock(ns, privPassphrase) + }) + require.NoError(t, err) + + scopedMgr, err := mgr.FetchScopedKeyManager(KeyScopeBIP0086) + require.NoError(t, err) + + const iterations = 50 + + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + + for range iterations { + _ = walletdb.Update(db, func(tx walletdb.ReadWriteTx) error { + ns := tx.ReadWriteBucket(waddrmgrNamespaceKey) + _, err := scopedMgr.NextExternalAddresses( + ns, 0, 1, + ) + + return err + }) + } + }() + + go func() { + defer wg.Done() + + for range iterations { + _ = mgr.Lock() + + _ = walletdb.View(db, func(tx walletdb.ReadTx) error { + ns := tx.ReadBucket(waddrmgrNamespaceKey) + + return mgr.Unlock(ns, privPassphrase) + }) + } + }() + + wg.Wait() +} diff --git a/waddrmgr/scoped_manager.go b/waddrmgr/scoped_manager.go index 0f12d1d779..5139ba6ac9 100644 --- a/waddrmgr/scoped_manager.go +++ b/waddrmgr/scoped_manager.go @@ -258,6 +258,21 @@ func IsDefaultScope(scope KeyScope) bool { return false } +// keyManagerRoot is the interface through which ScopedKeyManager accesses +// the root Manager. It exposes only what is needed: lock state queries, +// crypto keys for encryption/decryption, chain parameters, and sync state +// management. This prevents ScopedKeyManager from directly accessing +// Manager's mutex or internal fields. +type keyManagerRoot interface { + IsLocked() bool + WatchOnly() bool + ChainParams() *chaincfg.Params + CryptoKeyPub() EncryptorDecryptor + CryptoKeyPriv() EncryptorDecryptor + CryptoKeyScript() EncryptorDecryptor + SetStartBlock(ns walletdb.ReadWriteBucket, bs *BlockStamp) error +} + // ScopedKeyManager is a sub key manager under the main root key manager. The // root key manager will handle the root HD key (m/), while each sub scoped key // manager will handle the cointype key for a particular key scope @@ -274,10 +289,9 @@ type ScopedKeyManager struct { // consulted when encoding addresses from derived keys. addrSchema ScopeAddrSchema - // rootManager is a pointer to the root key manager. We'll maintain - // this as we need access to the crypto encryption keys before we can - // derive any new accounts of child keys of accounts. - rootManager *Manager + // rootManager provides access to the root key manager through the + // keyManagerRoot interface. + rootManager keyManagerRoot // addrs is a cached map of all the addresses that we currently // manage. @@ -323,6 +337,115 @@ func (s *ScopedKeyManager) zeroSensitivePublicData() { } } +// Lock removes all private key material from memory. It is the thread-safe +// counterpart to the private key restoration done by Unlock. +func (s *ScopedKeyManager) Lock() { + s.mtx.Lock() + defer s.mtx.Unlock() + + s.zeroPrivKeys() +} + +// Unlock decrypts and restores all account private keys, and derives any +// private keys that were pending from when the manager was locked. It is the +// inverse of Lock. +func (s *ScopedKeyManager) Unlock(cryptoKeyPriv EncryptorDecryptor, + ns walletdb.ReadBucket) error { + + s.mtx.Lock() + defer s.mtx.Unlock() + + for account, acctInfo := range s.acctInfo { + decrypted, err := cryptoKeyPriv.Decrypt( + acctInfo.acctKeyEncrypted, + ) + if err != nil { + str := fmt.Sprintf("failed to decrypt account %d "+ + "private key", account) + + return managerError(ErrCrypto, str, err) + } + + acctKeyPriv, err := hdkeychain.NewKeyFromString( + string(decrypted), + ) + + zero.Bytes(decrypted) + + if err != nil { + str := fmt.Sprintf("failed to regenerate account "+ + "%d extended key", account) + + return managerError(ErrKeyChain, str, err) + } + + acctInfo.acctKeyPriv = acctKeyPriv + } + + for _, info := range s.deriveOnUnlock { + addressKey, _, _, err := s.deriveKeyFromPath( + ns, info.managedAddr.InternalAccount(), + info.branch, info.index, true, + ) + if err != nil { + return err + } + + privKey, _ := addressKey.ECPrivKey() + addressKey.Zero() + + privKeyBytes := privKey.Serialize() + privKeyEncrypted, err := cryptoKeyPriv.Encrypt( + privKeyBytes, + ) + + privKey.Zero() + + if err != nil { + str := fmt.Sprintf("failed to encrypt private "+ + "key for address %s", + info.managedAddr.Address()) + + return managerError(ErrCrypto, str, err) + } + + switch a := info.managedAddr.(type) { + case *managedAddress: + a.privKeyEncrypted = privKeyEncrypted + a.privKeyCT = privKeyBytes + case *scriptAddress: + } + + s.deriveOnUnlock[0] = nil + s.deriveOnUnlock = s.deriveOnUnlock[1:] + } + + return nil +} + +// ConvertToWatchingOnly removes all encrypted private key material from this +// scoped manager's in-memory state. +func (s *ScopedKeyManager) ConvertToWatchingOnly() { + s.mtx.Lock() + defer s.mtx.Unlock() + + for _, acctInfo := range s.acctInfo { + zero.Bytes(acctInfo.acctKeyEncrypted) + acctInfo.acctKeyEncrypted = nil + } + + for _, ma := range s.addrs { + switch addr := ma.(type) { + case *managedAddress: + zero.Bytes(addr.privKeyEncrypted) + addr.privKeyEncrypted = nil + case *scriptAddress: + zero.Bytes(addr.scriptEncrypted) + addr.scriptEncrypted = nil + } + } +} + // Close cleanly shuts down the manager. It makes a best try effort to remove // and zero all private key and sensitive public key material associated with // the address manager from memory. @@ -330,7 +453,7 @@ func (s *ScopedKeyManager) Close() { s.mtx.Lock() defer s.mtx.Unlock() - // Attempt to clear sensitive public key material from memory too. + s.zeroPrivKeys() s.zeroSensitivePublicData() } @@ -466,7 +589,7 @@ func (s *ScopedKeyManager) loadAccountInfo(ns walletdb.ReadBucket, // Use the crypto public key to decrypt the account public // extended key. acctInfo.acctKeyPub, err = decryptKey( - s.rootManager.cryptoKeyPub, row.pubKeyEncrypted, + s.rootManager.CryptoKeyPub(), row.pubKeyEncrypted, ) if err != nil { str := fmt.Sprintf("failed to decrypt public key for "+ @@ -478,7 +601,7 @@ func (s *ScopedKeyManager) loadAccountInfo(ns walletdb.ReadBucket, // Use the crypto private key to decrypt the account // private extended keys. acctInfo.acctKeyPriv, err = decryptKey( - s.rootManager.cryptoKeyPriv, row.privKeyEncrypted, + s.rootManager.CryptoKeyPriv(), row.privKeyEncrypted, ) if err != nil { str := fmt.Sprintf("failed to decrypt private "+ @@ -500,7 +623,7 @@ func (s *ScopedKeyManager) loadAccountInfo(ns walletdb.ReadBucket, // Use the crypto public key to decrypt the account public // extended key. acctInfo.acctKeyPub, err = decryptKey( - s.rootManager.cryptoKeyPub, row.pubKeyEncrypted, + s.rootManager.CryptoKeyPub(), row.pubKeyEncrypted, ) if err != nil { str := fmt.Sprintf("failed to decrypt public key for "+ @@ -804,7 +927,7 @@ func (s *ScopedKeyManager) chainAddressRowToManaged(ns walletdb.ReadBucket, func (s *ScopedKeyManager) importedAddressRowToManaged(row *dbImportedAddressRow) (ManagedAddress, error) { // Use the crypto public key to decrypt the imported public key. - pubBytes, err := s.rootManager.cryptoKeyPub.Decrypt(row.encryptedPubKey) + pubBytes, err := s.rootManager.CryptoKeyPub().Decrypt(row.encryptedPubKey) if err != nil { str := "failed to decrypt public key for imported address" return nil, managerError(ErrCrypto, str, err) @@ -837,7 +960,7 @@ func (s *ScopedKeyManager) scriptAddressRowToManaged( row *dbScriptAddressRow) (ManagedAddress, error) { // Use the crypto public key to decrypt the imported script hash. - scriptHash, err := s.rootManager.cryptoKeyPub.Decrypt(row.encryptedHash) + scriptHash, err := s.rootManager.CryptoKeyPub().Decrypt(row.encryptedHash) if err != nil { str := "failed to decrypt imported script hash" return nil, managerError(ErrCrypto, str, err) @@ -852,7 +975,7 @@ func (s *ScopedKeyManager) witnessScriptAddressRowToManaged( row *dbWitnessScriptAddressRow) (ManagedAddress, error) { // Use the crypto public key to decrypt the imported script hash. - scriptHash, err := s.rootManager.cryptoKeyPub.Decrypt(row.encryptedHash) + scriptHash, err := s.rootManager.CryptoKeyPub().Decrypt(row.encryptedHash) if err != nil { str := "failed to decrypt imported witness script hash" return nil, managerError(ErrCrypto, str, err) @@ -1077,7 +1200,8 @@ func (s *ScopedKeyManager) nextAddresses(ns walletdb.ReadWriteBucket, nextIndex) return nil, managerError(ErrKeyChain, str, err) } - key.SetNet(s.rootManager.chainParams) + + key.SetNet(s.rootManager.ChainParams()) nextIndex++ nextKey = key @@ -1136,7 +1260,7 @@ func (s *ScopedKeyManager) nextAddresses(ns walletdb.ReadWriteBucket, } case *scriptAddress: - encryptedHash, err := s.rootManager.cryptoKeyPub.Encrypt( + encryptedHash, err := s.rootManager.CryptoKeyPub().Encrypt( a.AddrHash(), ) if err != nil { @@ -1307,7 +1431,8 @@ func (s *ScopedKeyManager) extendAddresses(ns walletdb.ReadWriteBucket, nextIndex) return managerError(ErrKeyChain, str, err) } - key.SetNet(s.rootManager.chainParams) + + key.SetNet(s.rootManager.ChainParams()) nextIndex++ nextKey = key @@ -1364,10 +1489,13 @@ func (s *ScopedKeyManager) extendAddresses(ns walletdb.ReadWriteBucket, return maybeConvertDbError(err) } case *scriptAddress: - encryptedHash, err := s.rootManager.cryptoKeyPub.Encrypt(a.AddrHash()) + encryptedHash, err := s.rootManager.CryptoKeyPub().Encrypt( + a.AddrHash(), + ) if err != nil { str := fmt.Sprintf("failed to encrypt script hash %x", a.AddrHash()) + return managerError(ErrCrypto, str, err) } @@ -1663,7 +1791,9 @@ func (s *ScopedKeyManager) newAccount(ns walletdb.ReadWriteBucket, } // Decrypt the cointype key. - serializedKeyPriv, err := s.rootManager.cryptoKeyPriv.Decrypt(coinTypePrivEnc) + serializedKeyPriv, err := s.rootManager.CryptoKeyPriv().Decrypt( + coinTypePrivEnc, + ) if err != nil { str := "failed to decrypt cointype serialized private key" return managerError(ErrLocked, str, err) @@ -1689,14 +1819,15 @@ func (s *ScopedKeyManager) newAccount(ns walletdb.ReadWriteBucket, } // Encrypt the default account keys with the associated crypto keys. - acctPubEnc, err := s.rootManager.cryptoKeyPub.Encrypt( + acctPubEnc, err := s.rootManager.CryptoKeyPub().Encrypt( []byte(acctKeyPub.String()), ) if err != nil { str := "failed to encrypt public key for account" return managerError(ErrCrypto, str, err) } - acctPrivEnc, err := s.rootManager.cryptoKeyPriv.Encrypt( + + acctPrivEnc, err := s.rootManager.CryptoKeyPriv().Encrypt( []byte(acctKeyPriv.String()), ) if err != nil { @@ -1784,7 +1915,7 @@ func (s *ScopedKeyManager) newAccountWatchingOnly(ns walletdb.ReadWriteBucket, } // Encrypt the default account keys with the associated crypto keys. - acctPubEnc, err := s.rootManager.cryptoKeyPub.Encrypt( + acctPubEnc, err := s.rootManager.CryptoKeyPub().Encrypt( []byte(pubKey.String()), ) if err != nil { @@ -1912,10 +2043,10 @@ func (s *ScopedKeyManager) ImportPrivateKey(ns walletdb.ReadWriteBucket, // Ensure the address is intended for network the address manager is // associated with. - if !wif.IsForNet(s.rootManager.chainParams) { + if !wif.IsForNet(s.rootManager.ChainParams()) { str := fmt.Sprintf("private key is not for the same network the "+ "address manager is configured for (%s)", - s.rootManager.chainParams.Name) + s.rootManager.ChainParams().Name) return nil, managerError(ErrWrongNet, str, nil) } @@ -1932,8 +2063,13 @@ func (s *ScopedKeyManager) ImportPrivateKey(ns walletdb.ReadWriteBucket, if !s.rootManager.WatchOnly() { privKeyBytes := wif.PrivKey.Serialize() var err error - encryptedPrivKey, err = s.rootManager.cryptoKeyPriv.Encrypt(privKeyBytes) + + encryptedPrivKey, err = s.rootManager.CryptoKeyPriv().Encrypt( + privKeyBytes, + ) + zero.Bytes(privKeyBytes) + if err != nil { str := fmt.Sprintf("failed to encrypt private key for %x", wif.PrivKey.PubKey().SerializeCompressed()) @@ -1994,7 +2130,7 @@ func (s *ScopedKeyManager) importPublicKey(ns walletdb.ReadWriteBucket, case NestedWitnessPubKey: pubKeyHash := btcutil.Hash160(serializedPubKey) p2wkhAddr, err := btcutil.NewAddressWitnessPubKeyHash( - pubKeyHash, s.rootManager.chainParams, + pubKeyHash, s.rootManager.ChainParams(), ) if err != nil { return err @@ -2028,7 +2164,7 @@ func (s *ScopedKeyManager) importPublicKey(ns walletdb.ReadWriteBucket, } // Encrypt public key. - encryptedPubKey, err := s.rootManager.cryptoKeyPub.Encrypt( + encryptedPubKey, err := s.rootManager.CryptoKeyPub().Encrypt( serializedPubKey, ) if err != nil { @@ -2037,15 +2173,7 @@ func (s *ScopedKeyManager) importPublicKey(ns walletdb.ReadWriteBucket, return managerError(ErrCrypto, str, err) } - // The start block needs to be updated when the newly imported address - // is before the current one. - s.rootManager.mtx.Lock() - updateStartBlock := bs != nil && - bs.Height < s.rootManager.syncState.startBlock.Height - s.rootManager.mtx.Unlock() - - // Save the new imported address to the db and update start block (if - // needed) in a single transaction. + // Save the new imported address to the db. err = putImportedAddress( ns, &s.scope, addressID, ImportedAddrAccount, ssNone, encryptedPubKey, encryptedPrivKey, @@ -2054,21 +2182,14 @@ func (s *ScopedKeyManager) importPublicKey(ns walletdb.ReadWriteBucket, return err } - if updateStartBlock { - err := putStartBlock(ns, bs) + // Update the start block if the imported address is earlier. + if bs != nil { + err = s.rootManager.SetStartBlock(ns, bs) if err != nil { return err } } - // Now that the database has been updated, update the start block in - // memory too if needed. - if updateStartBlock { - s.rootManager.mtx.Lock() - s.rootManager.syncState.startBlock = *bs - s.rootManager.mtx.Unlock() - } - return nil } @@ -2198,12 +2319,7 @@ func (s *ScopedKeyManager) importScriptAddress(ns walletdb.ReadWriteBucket, error) { s.mtx.Lock() - unlockNeeded := true - defer func() { - if unlockNeeded { - s.mtx.Unlock() - } - }() + defer s.mtx.Unlock() // The manager must be unlocked to encrypt the imported script. if isSecretScript && s.rootManager.IsLocked() { @@ -2228,7 +2344,7 @@ func (s *ScopedKeyManager) importScriptAddress(ns walletdb.ReadWriteBucket, // Encrypt the script hash/key using the crypto public key, so it is // accessible when the address manager is locked or watching-only. - encryptedHash, err := s.rootManager.cryptoKeyPub.Encrypt(scriptIdent) + encryptedHash, err := s.rootManager.CryptoKeyPub().Encrypt(scriptIdent) if err != nil { str := fmt.Sprintf("failed to encrypt script hash/key %x", scriptIdent) @@ -2238,9 +2354,9 @@ func (s *ScopedKeyManager) importScriptAddress(ns walletdb.ReadWriteBucket, // If a key isn't considered to be "secret", we encrypt it with the // public key, so we can create script addresses that also work in // watch-only mode. - cryptoKey := s.rootManager.cryptoKeyScript + cryptoKey := s.rootManager.CryptoKeyScript() if !isSecretScript { - cryptoKey = s.rootManager.cryptoKeyPub + cryptoKey = s.rootManager.CryptoKeyPub() } // Encrypt the script for storage in database using the selected crypto @@ -2252,17 +2368,7 @@ func (s *ScopedKeyManager) importScriptAddress(ns walletdb.ReadWriteBucket, return nil, managerError(ErrCrypto, str, err) } - // The start block needs to be updated when the newly imported address - // is before the current one. - updateStartBlock := false - s.rootManager.mtx.RLock() - if bs.Height < s.rootManager.syncState.startBlock.Height { - updateStartBlock = true - } - s.rootManager.mtx.RUnlock() - - // Save the new imported address to the db and update start block (if - // needed) in a single transaction. + // Save the new imported address to the db. switch addrType { case WitnessScript, TaprootScript: err = putWitnessScriptAddress( @@ -2302,13 +2408,6 @@ func (s *ScopedKeyManager) importScriptAddress(ns walletdb.ReadWriteBucket, return nil, err } - if updateStartBlock { - err := putStartBlock(ns, bs) - if err != nil { - return nil, maybeConvertDbError(err) - } - } - // Even if the script is secret, we are currently unlocked, so we keep a // clear text copy of the script around to avoid decrypting it on each // access. @@ -2320,17 +2419,10 @@ func (s *ScopedKeyManager) importScriptAddress(ns walletdb.ReadWriteBucket, // return it. s.addrs[addrKey(scriptIdent)] = managedAddr - if updateStartBlock { - // Now that the database has been updated, update the start block in - // memory too if needed. Ensure the manager lock goes outside the scoped - // manager lock. - s.mtx.Unlock() - unlockNeeded = false - s.rootManager.mtx.Lock() - if bs.Height < s.rootManager.syncState.startBlock.Height { - s.rootManager.syncState.startBlock = *bs - } - s.rootManager.mtx.Unlock() + // Update the start block if the imported address is earlier. + err = s.rootManager.SetStartBlock(ns, bs) + if err != nil { + return nil, fmt.Errorf("set start block: %w", err) } return managedAddr, nil @@ -2382,7 +2474,7 @@ func (s *ScopedKeyManager) ChainParams() *chaincfg.Params { // NOTE: No need for mutex here since the net field does not change // after the manager instance is created. - return s.rootManager.chainParams + return s.rootManager.ChainParams() } // AccountName returns the account name for the given account number stored in @@ -2575,3 +2667,26 @@ func (s *ScopedKeyManager) InvalidateAccountCache(account uint32) { defer s.mtx.Unlock() delete(s.acctInfo, account) } + +// zeroPrivKeys zeroes all private key material: account private keys +// and clear-text private keys in managed addresses. +// +// This function MUST be called with s.mtx held for writes. +func (s *ScopedKeyManager) zeroPrivKeys() { + for _, acctInfo := range s.acctInfo { + if acctInfo.acctKeyPriv != nil { + acctInfo.acctKeyPriv.Zero() + } + + acctInfo.acctKeyPriv = nil + } + + for _, ma := range s.addrs { + switch addr := ma.(type) { + case *managedAddress: + addr.lock() + case *scriptAddress: + addr.lock() + } + } +} diff --git a/waddrmgr/sync.go b/waddrmgr/sync.go index 3472ddbd9d..c1731c854f 100644 --- a/waddrmgr/sync.go +++ b/waddrmgr/sync.go @@ -49,8 +49,8 @@ func newSyncState(startBlock, syncedTo *BlockStamp) *syncState { // marked as unsynced back to the oldest known point any of the addresses have // appeared in the block chain. func (m *Manager) SetSyncedTo(ns walletdb.ReadWriteBucket, bs *BlockStamp) error { - m.mtx.Lock() - defer m.mtx.Unlock() + m.syncStateMtx.Lock() + defer m.syncStateMtx.Unlock() // Use the stored start blockstamp and reset recent hashes and height // when the provided blockstamp is nil. @@ -74,8 +74,8 @@ func (m *Manager) SetSyncedTo(ns walletdb.ReadWriteBucket, bs *BlockStamp) error // can use this information for intelligently initiating rescans to sync back to // the best chain from the last known good block. func (m *Manager) SyncedTo() BlockStamp { - m.mtx.RLock() - defer m.mtx.RUnlock() + m.syncStateMtx.RLock() + defer m.syncStateMtx.RUnlock() return m.syncState.syncedTo } diff --git a/wallet/wallet_test.go b/wallet/wallet_test.go index 527cd83ad2..06d3caa583 100644 --- a/wallet/wallet_test.go +++ b/wallet/wallet_test.go @@ -661,3 +661,37 @@ func TestEndRecovery(t *testing.T) { t.Fatal("wrong error") } } + +// TestWalletLockerAddressRace verifies there is no data race between the +// walletLocker goroutine locking the wallet and concurrent address derivation +// via NewAddress. +func TestWalletLockerAddressRace(t *testing.T) { + t.Parallel() + + w, cleanup := testWallet(t) + defer cleanup() + + require.NoError(t, w.Unlock([]byte("world"), nil)) + + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + + for range 20 { + _, _ = w.NewAddress(0, waddrmgr.KeyScopeBIP0086) + } + }() + + go func() { + defer wg.Done() + + for range 20 { + w.Lock() + _ = w.Unlock([]byte("world"), nil) + } + }() + + wg.Wait() +}