Skip to content

fix(waddrmgr): fix data race between Manager lock/unlock and ScopedKeyManager operations #1183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Aharonee wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(waddrmgr): fix data race between Manager lock/unlock and ScopedKeyManager operations #1183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94ca3cb
waddrmgr: fix data race between Manager lock/unlock and ScopedKeyManager
Aharonee Feb 22, 2026
fb1e8c2
fix(waddrmgr): resolve CI lint errors
Aharonee Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions waddrmgr/address.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -577,23 +584,23 @@ func newManagedAddressWithoutPrivKey(m *ScopedKeyManager,
// witnessProgram as the sigScript, then present the proper
// <sig, pubkey> pair as the witness.
address, err = btcutil.NewAddressScriptHash(
witnessProgram, m.rootManager.chainParams,
witnessProgram, m.rootManager.ChainParams(),
)
if err != nil {
return nil, err
}

case PubKeyHash:
address, err = btcutil.NewAddressPubKeyHash(
pubKeyHash, m.rootManager.chainParams,
pubKeyHash, m.rootManager.ChainParams(),
)
if err != nil {
return nil, err
}

case WitnessPubKey:
address, err = btcutil.NewAddressWitnessPubKeyHash(
pubKeyHash, m.rootManager.chainParams,
pubKeyHash, m.rootManager.ChainParams(),
)
if err != nil {
return nil, err
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
161 changes: 55 additions & 106 deletions waddrmgr/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
}

Expand Down
Loading