From 4d9c43593ddd3162364adc8a4f57cc471708742c Mon Sep 17 00:00:00 2001 From: Josh Rickmar Date: Tue, 3 Feb 2015 20:42:40 -0500 Subject: [PATCH] Consolidate and optimize zero functions. This introduce a new internal package to deal with the explicit clearing of data (such as private keys) in byte slices, byte arrays (32 and 64-bytes long), and multi-precision "big" integers. Benchmarks from a xeon e3 (Xor is the zeroing funcion which Bytes replaces): BenchmarkXor32 30000000 52.1 ns/op BenchmarkXor64 20000000 91.5 ns/op BenchmarkRange32 50000000 31.8 ns/op BenchmarkRange64 30000000 49.5 ns/op BenchmarkBytes32 200000000 10.1 ns/op BenchmarkBytes64 100000000 15.4 ns/op BenchmarkBytea32 1000000000 2.24 ns/op BenchmarkBytea64 300000000 4.46 ns/op Removes an XXX from the votingpool package. --- internal/zero/benchmark_test.go | 83 ++++++++++++++++++ internal/zero/zero.go | 49 +++++++++++ internal/zero/zero_test.go | 143 ++++++++++++++++++++++++++++++++ snacl/snacl.go | 12 +-- votingpool/pool.go | 15 +--- waddrmgr/address.go | 35 ++------ waddrmgr/manager.go | 33 ++++---- 7 files changed, 302 insertions(+), 68 deletions(-) create mode 100644 internal/zero/benchmark_test.go create mode 100644 internal/zero/zero.go create mode 100644 internal/zero/zero_test.go diff --git a/internal/zero/benchmark_test.go b/internal/zero/benchmark_test.go new file mode 100644 index 0000000..802bbc1 --- /dev/null +++ b/internal/zero/benchmark_test.go @@ -0,0 +1,83 @@ +package zero_test + +import ( + "testing" + + . "github.com/btcsuite/btcwallet/internal/zero" +) + +var ( + bytes32 = make([]byte, 32) // typical key size + bytes64 = make([]byte, 64) // passphrase hash size + bytea32 = new([32]byte) + bytea64 = new([64]byte) +) + +// xor is the "slow" byte zeroing implementation which this package +// originally replaced. If this function benchmarks faster than the +// functions exported by this package in a future Go version (perhaps +// by calling runtime.memclr), replace the "optimized" versions with +// this. +func xor(b []byte) { + for i := range b { + b[i] ^= b[i] + } +} + +// zrange is an alternative zero implementation that, while currently +// slower than the functions provided by this package, may be faster +// in a future Go release. Switch to this or the xor implementation +// if they ever become faster. +func zrange(b []byte) { + for i := range b { + b[i] = 0 + } +} + +func BenchmarkXor32(b *testing.B) { + for i := 0; i < b.N; i++ { + xor(bytes32) + } +} + +func BenchmarkXor64(b *testing.B) { + for i := 0; i < b.N; i++ { + xor(bytes64) + } +} + +func BenchmarkRange32(b *testing.B) { + for i := 0; i < b.N; i++ { + zrange(bytes32) + } +} + +func BenchmarkRange64(b *testing.B) { + for i := 0; i < b.N; i++ { + zrange(bytes64) + } +} + +func BenchmarkBytes32(b *testing.B) { + for i := 0; i < b.N; i++ { + Bytes(bytes32) + } +} + +func BenchmarkBytes64(b *testing.B) { + for i := 0; i < b.N; i++ { + Bytes(bytes64) + } +} + +func BenchmarkBytea32(b *testing.B) { + for i := 0; i < b.N; i++ { + Bytea32(bytea32) + } +} + +func BenchmarkBytea64(b *testing.B) { + for i := 0; i < b.N; i++ { + Bytea64(bytea64) + } +} diff --git a/internal/zero/zero.go b/internal/zero/zero.go new file mode 100644 index 0000000..51d4505 --- /dev/null +++ b/internal/zero/zero.go @@ -0,0 +1,49 @@ +// Package zero contains functions to clear data from byte slices and +// multi-precision integers. +package zero + +import ( + "math/big" +) + +// Bytes sets all bytes in the passed slice to zero. This is used to +// explicitly clear private key material from memory. +// +// In general, prefer to use the fixed-sized zeroing functions (Bytea*) +// when zeroing bytes as they are much more efficient than the variable +// sized zeroing func Bytes. +func Bytes(b []byte) { + z := [32]byte{} + n := uint(copy(b, z[:])) + for n < uint(len(b)) { + copy(b[n:], b[:n]) + n <<= 1 + } +} + +// Bytea32 clears the 32-byte array by filling it with the zero value. +// This is used to explicitly clear private key material from memory. +func Bytea32(b *[32]byte) { + *b = [32]byte{} +} + +// Bytea64 clears the 64-byte array by filling it with the zero value. +// This is used to explicitly clear sensitive material from memory. +func Bytea64(b *[64]byte) { + *b = [64]byte{} +} + +// BigInt sets all bytes in the passed big int to zero and then sets the +// value to 0. This differs from simply setting the value in that it +// specifically clears the underlying bytes whereas simply setting the value +// does not. This is mostly useful to forcefully clear private keys. +func BigInt(x *big.Int) { + b := x.Bits() + z := [16]big.Word{} + n := uint(copy(b, z[:])) + for n < uint(len(b)) { + copy(b[n:], b[:n]) + n <<= 1 + } + x.SetInt64(0) +} diff --git a/internal/zero/zero_test.go b/internal/zero/zero_test.go new file mode 100644 index 0000000..e877e4c --- /dev/null +++ b/internal/zero/zero_test.go @@ -0,0 +1,143 @@ +package zero_test + +import ( + "fmt" + "math/big" + "strings" + "testing" + + . "github.com/btcsuite/btcwallet/internal/zero" +) + +func makeOneBytes(n int) []byte { + b := make([]byte, n) + for i := range b { + b[i] = 1 + } + return b +} + +func checkZeroBytes(b []byte) error { + for i, v := range b { + if v != 0 { + return fmt.Errorf("b[%d] = %d", i, v) + } + } + return nil +} + +func TestBytes(t *testing.T) { + tests := []int{ + 0, + 31, + 32, + 33, + 127, + 128, + 129, + 255, + 256, + 256, + 257, + 383, + 384, + 385, + 511, + 512, + 513, + } + + for i, n := range tests { + b := makeOneBytes(n) + Bytes(b) + err := checkZeroBytes(b) + if err != nil { + t.Errorf("Test %d (n=%d) failed: %v", i, n, err) + continue + } + } +} + +func checkZeroWords(b []big.Word) error { + for i, v := range b { + if v != 0 { + return fmt.Errorf("b[%d] = %d", i, v) + } + } + return nil +} + +var bigZero = new(big.Int) + +func TestBigInt(t *testing.T) { + tests := []string{ + // 16 0xFFFFFFFF 32-bit uintptrs + strings.Repeat("FFFFFFFF", 16), + + // 17 32-bit uintptrs, minimum value which enters loop on 32-bit + "01" + strings.Repeat("00000000", 16), + + // 32 0xFFFFFFFF 32-bit uintptrs, maximum value which enters loop exactly once on 32-bit + strings.Repeat("FFFFFFFF", 32), + + // 33 32-bit uintptrs, minimum value which enters loop twice on 32-bit + "01" + strings.Repeat("00000000", 32), + + // 16 0xFFFFFFFFFFFFFFFF 64-bit uintptrs + strings.Repeat("FFFFFFFFFFFFFFFF", 16), + + // 17 64-bit uintptrs, minimum value which enters loop on 64-bit + "01" + strings.Repeat("0000000000000000", 16), + + // 32 0xFFFFFFFFFFFFFFFF 64-bit uintptrs, maximum value which enters loop exactly once on 64-bit + strings.Repeat("FFFFFFFFFFFFFFFF", 32), + + // 33 64-bit uintptrs, minimum value which enters loop twice on 64-bit + "01" + strings.Repeat("0000000000000000", 32), + } + + for i, s := range tests { + v, ok := new(big.Int).SetString(s, 16) + if !ok { + t.Errorf("Test %d includes invalid hex number %s", i, s) + continue + } + + BigInt(v) + err := checkZeroWords(v.Bits()) + if err != nil { + t.Errorf("Test %d (s=%s) failed: %v", i, s, err) + continue + } + if v.Cmp(bigZero) != 0 { + t.Errorf("Test %d (s=%s) zeroed big.Int represents non-zero number %v", i, s, v) + continue + } + } +} + +func TestBytea32(t *testing.T) { + const sz = 32 + var b [sz]byte + copy(b[:], makeOneBytes(sz)) + + Bytea32(&b) + + err := checkZeroBytes(b[:]) + if err != nil { + t.Error(err) + } +} + +func TestBytea64(t *testing.T) { + const sz = 64 + var b [sz]byte + copy(b[:], makeOneBytes(sz)) + + Bytea64(&b) + + err := checkZeroBytes(b[:]) + if err != nil { + t.Error(err) + } +} diff --git a/snacl/snacl.go b/snacl/snacl.go index 8e8725f..f939889 100644 --- a/snacl/snacl.go +++ b/snacl/snacl.go @@ -8,6 +8,7 @@ import ( "io" "runtime/debug" + "github.com/btcsuite/btcwallet/internal/zero" "github.com/btcsuite/fastsha256" "github.com/btcsuite/golangcrypto/nacl/secretbox" "github.com/btcsuite/golangcrypto/scrypt" @@ -23,13 +24,6 @@ var ( ErrDecryptFailed = errors.New("unable to decrypt") ) -// Zero out a byte slice. -func zero(b []byte) { - for i := range b { - b[i] ^= b[i] - } -} - const ( // Expose secretbox's Overhead const here for convenience. Overhead = secretbox.Overhead @@ -79,7 +73,7 @@ func (ck *CryptoKey) Decrypt(in []byte) ([]byte, error) { // rather than waiting until it's reclaimed by the garbage collector. The // key is no longer usable after this call. func (ck *CryptoKey) Zero() { - zero(ck[:]) + zero.Bytea32((*[KeySize]byte)(ck)) } // GenerateCryptoKey generates a new crypotgraphically random key. @@ -120,7 +114,7 @@ func (sk *SecretKey) deriveKey(password *[]byte) error { return err } copy(sk.Key[:], key) - zero(key) + zero.Bytes(key) // I'm not a fan of forced garbage collections, but scrypt allocates a // ton of memory and calling it back to back without a GC cycle in diff --git a/votingpool/pool.go b/votingpool/pool.go index 38833f0..e1d483b 100644 --- a/votingpool/pool.go +++ b/votingpool/pool.go @@ -23,6 +23,7 @@ import ( "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcutil" "github.com/btcsuite/btcutil/hdkeychain" + "github.com/btcsuite/btcwallet/internal/zero" "github.com/btcsuite/btcwallet/waddrmgr" "github.com/btcsuite/btcwallet/walletdb" ) @@ -326,7 +327,7 @@ func (vp *Pool) decryptExtendedKey(keyType waddrmgr.CryptoKeyType, encrypted []b return nil, managerError(waddrmgr.ErrCrypto, str, err) } result, err := hdkeychain.NewKeyFromString(string(decrypted)) - zero(decrypted) + zero.Bytes(decrypted) if err != nil { str := fmt.Sprintf("cannot get key from string %v", decrypted) return nil, managerError(waddrmgr.ErrKeyChain, str, err) @@ -608,15 +609,3 @@ func (s *SeriesData) IsEmpowered() bool { func managerError(c waddrmgr.ErrorCode, desc string, err error) waddrmgr.ManagerError { return waddrmgr.ManagerError{ErrorCode: c, Description: desc, Err: err} } - -// zero sets all bytes in the passed slice to zero. This is used to -// explicitly clear private key material from memory. -// -// XXX(lars) there exists currently around 4-5 other zero functions -// with at least 3 different implementations. We should try to -// consolidate these. -func zero(b []byte) { - for i := range b { - b[i] ^= b[i] - } -} diff --git a/waddrmgr/address.go b/waddrmgr/address.go index 0727e68..64bcb19 100644 --- a/waddrmgr/address.go +++ b/waddrmgr/address.go @@ -19,39 +19,14 @@ package waddrmgr import ( "encoding/hex" "fmt" - "math/big" "sync" "github.com/btcsuite/btcd/btcec" "github.com/btcsuite/btcutil" "github.com/btcsuite/btcutil/hdkeychain" + "github.com/btcsuite/btcwallet/internal/zero" ) -// zero sets all bytes in the passed slice to zero. This is used to -// explicitly clear private key material from memory. -func zero(b []byte) { - for i := range b { - b[i] ^= b[i] - } -} - -// zeroBigInt sets all bytes in the passed big int to zero and then sets the -// value to 0. This differs from simply setting the value in that it -// specifically clears the underlying bytes whereas simply setting the value -// does not. This is mostly useful to forcefully clear private keys. -func zeroBigInt(x *big.Int) { - // NOTE: This could make use of .Xor, however this is safer since the - // specific implementation of Xor could technically change in such a way - // as the original bits aren't cleared. This function would silenty - // fail in that case and it's best to avoid that possibility. - bits := x.Bits() - numBits := len(bits) - for i := 0; i < numBits; i++ { - bits[i] ^= bits[i] - } - x.SetInt64(0) -} - // ManagedAddress is an interface that provides acces to information regarding // an address managed by an address manager. Concrete implementations of this // type may provide further fields to provide information specific to that type @@ -159,7 +134,7 @@ func (a *managedAddress) lock() { // Zero and nil the clear text private key associated with this // address. a.privKeyMutex.Lock() - zero(a.privKeyCT) + zero.Bytes(a.privKeyCT) a.privKeyCT = nil a.privKeyMutex.Unlock() } @@ -260,7 +235,7 @@ func (a *managedAddress) PrivKey() (*btcec.PrivateKey, error) { } privKey, _ := btcec.PrivKeyFromBytes(btcec.S256(), privKeyCopy) - zero(privKeyCopy) + zero.Bytes(privKeyCopy) return privKey, nil } @@ -351,7 +326,7 @@ func newManagedAddressFromExtKey(m *Manager, account uint32, key *hdkeychain.Ext // Ensure the temp private key big integer is cleared after use. managedAddr, err = newManagedAddress(m, account, privKey, true) - zeroBigInt(privKey.D) + zero.BigInt(privKey.D) if err != nil { return nil, err } @@ -413,7 +388,7 @@ func (a *scriptAddress) unlock(key EncryptorDecryptor) ([]byte, error) { func (a *scriptAddress) lock() { // Zero and nil the clear text script associated with this address. a.scriptMutex.Lock() - zero(a.scriptCT) + zero.Bytes(a.scriptCT) a.scriptCT = nil a.scriptMutex.Unlock() } diff --git a/waddrmgr/manager.go b/waddrmgr/manager.go index 38828f8..bced6a3 100644 --- a/waddrmgr/manager.go +++ b/waddrmgr/manager.go @@ -27,6 +27,7 @@ import ( "github.com/btcsuite/btcd/wire" "github.com/btcsuite/btcutil" "github.com/btcsuite/btcutil/hdkeychain" + "github.com/btcsuite/btcwallet/internal/zero" "github.com/btcsuite/btcwallet/snacl" "github.com/btcsuite/btcwallet/walletdb" ) @@ -298,7 +299,7 @@ func (m *Manager) lock() { m.masterKeyPriv.Zero() // Zero the hashed passphrase. - zero(m.hashedPrivPassphrase[:]) + 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 @@ -723,7 +724,7 @@ func (m *Manager) ChangePassphrase(oldPassphrase, newPassphrase []byte, private return managerError(ErrCrypto, str, err) } encPriv, err := newMasterKey.Encrypt(decPriv) - zero(decPriv) + zero.Bytes(decPriv) if err != nil { str := "failed to encrypt crypto private key" return managerError(ErrCrypto, str, err) @@ -737,7 +738,7 @@ func (m *Manager) ChangePassphrase(oldPassphrase, newPassphrase []byte, private return managerError(ErrCrypto, str, err) } encScript, err := newMasterKey.Encrypt(decScript) - zero(decScript) + zero.Bytes(decScript) if err != nil { str := "failed to encrypt crypto script key" return managerError(ErrCrypto, str, err) @@ -754,7 +755,7 @@ func (m *Manager) ChangePassphrase(oldPassphrase, newPassphrase []byte, private saltedPassphrase := append(passphraseSalt[:], newPassphrase...) hashedPassphrase = sha512.Sum512(saltedPassphrase) - zero(saltedPassphrase) + zero.Bytes(saltedPassphrase) } // Save the new keys and params to the the db in a single @@ -856,7 +857,7 @@ func (m *Manager) ConvertToWatchingOnly() error { // Clear and remove all of the encrypted acount private keys. for _, acctInfo := range m.acctInfo { - zero(acctInfo.acctKeyEncrypted) + zero.Bytes(acctInfo.acctKeyEncrypted) acctInfo.acctKeyEncrypted = nil } @@ -865,19 +866,19 @@ func (m *Manager) ConvertToWatchingOnly() error { for _, ma := range m.addrs { switch addr := ma.(type) { case *managedAddress: - zero(addr.privKeyEncrypted) + zero.Bytes(addr.privKeyEncrypted) addr.privKeyEncrypted = nil case *scriptAddress: - zero(addr.scriptEncrypted) + zero.Bytes(addr.scriptEncrypted) addr.scriptEncrypted = nil } } // Clear and remove encrypted private and script crypto keys. - zero(m.cryptoKeyScriptEncrypted) + zero.Bytes(m.cryptoKeyScriptEncrypted) m.cryptoKeyScriptEncrypted = nil m.cryptoKeyScript = nil - zero(m.cryptoKeyPrivEncrypted) + zero.Bytes(m.cryptoKeyPrivEncrypted) m.cryptoKeyPrivEncrypted = nil m.cryptoKeyPriv = nil @@ -976,7 +977,7 @@ func (m *Manager) ImportPrivateKey(wif *btcutil.WIF, bs *BlockStamp) (ManagedPub if !m.watchingOnly { privKeyBytes := wif.PrivKey.Serialize() encryptedPrivKey, err = m.cryptoKeyPriv.Encrypt(privKeyBytes) - zero(privKeyBytes) + zero.Bytes(privKeyBytes) if err != nil { str := fmt.Sprintf("failed to encrypt private key for %x", serializedPubKey) @@ -1196,7 +1197,7 @@ func (m *Manager) Unlock(passphrase []byte) error { saltedPassphrase := append(m.privPassphraseSalt[:], passphrase...) hashedPassphrase := sha512.Sum512(saltedPassphrase) - zero(saltedPassphrase) + zero.Bytes(saltedPassphrase) if hashedPassphrase != m.hashedPrivPassphrase { m.lock() str := "invalid passphrase for master private key" @@ -1225,7 +1226,7 @@ func (m *Manager) Unlock(passphrase []byte) error { return managerError(ErrCrypto, str, err) } m.cryptoKeyPriv.CopyBytes(decryptedKey) - zero(decryptedKey) + zero.Bytes(decryptedKey) // Use the crypto private key to decrypt all of the account private // extended keys. @@ -1239,7 +1240,7 @@ func (m *Manager) Unlock(passphrase []byte) error { } acctKeyPriv, err := hdkeychain.NewKeyFromString(string(decrypted)) - zero(decrypted) + zero.Bytes(decrypted) if err != nil { m.lock() str := fmt.Sprintf("failed to regenerate account %d "+ @@ -1267,7 +1268,7 @@ func (m *Manager) Unlock(passphrase []byte) error { privKeyBytes := privKey.Serialize() privKeyEncrypted, err := m.cryptoKeyPriv.Encrypt(privKeyBytes) - zeroBigInt(privKey.D) + zero.BigInt(privKey.D) if err != nil { m.lock() str := fmt.Sprintf("failed to encrypt private key for "+ @@ -1285,7 +1286,7 @@ func (m *Manager) Unlock(passphrase []byte) error { m.locked = false saltedPassphrase := append(m.privPassphraseSalt[:], passphrase...) m.hashedPrivPassphrase = sha512.Sum512(saltedPassphrase) - zero(saltedPassphrase) + zero.Bytes(saltedPassphrase) return nil } @@ -1796,7 +1797,7 @@ func loadManager(namespace walletdb.Namespace, pubPassphrase []byte, chainParams return nil, managerError(ErrCrypto, str, err) } cryptoKeyPub.CopyBytes(cryptoKeyPubCT) - zero(cryptoKeyPubCT) + zero.Bytes(cryptoKeyPubCT) // Create the sync state struct. syncInfo := newSyncState(startBlock, syncedTo, recentHeight, recentHashes)