From 2154607a5f0e3e9535d24bf1414e61db40da320a Mon Sep 17 00:00:00 2001 From: Daniel Liu Date: Tue, 14 Jan 2025 10:56:07 +0800 Subject: [PATCH] accounts/keystore: double-check keystore file after creation (#17348) --- accounts/keystore/key.go | 18 +++++++++++----- accounts/keystore/keystore.go | 2 +- accounts/keystore/keystore_passphrase.go | 26 ++++++++++++++++++++++-- accounts/keystore/keystore_plain_test.go | 4 ++-- 4 files changed, 40 insertions(+), 10 deletions(-) diff --git a/accounts/keystore/key.go b/accounts/keystore/key.go index 7bb300fb82..c840d8926e 100644 --- a/accounts/keystore/key.go +++ b/accounts/keystore/key.go @@ -177,26 +177,34 @@ func storeNewKey(ks keyStore, rand io.Reader, auth string) (*Key, accounts.Accou return key, a, err } -func writeKeyFile(file string, content []byte) error { +func writeTemporaryKeyFile(file string, content []byte) (string, error) { // Create the keystore directory with appropriate permissions // in case it is not present yet. const dirPerm = 0700 if err := os.MkdirAll(filepath.Dir(file), dirPerm); err != nil { - return err + return "", err } // Atomic write: create a temporary hidden file first // then move it into place. TempFile assigns mode 0600. f, err := os.CreateTemp(filepath.Dir(file), "."+filepath.Base(file)+".tmp") if err != nil { - return err + return "", err } if _, err := f.Write(content); err != nil { f.Close() os.Remove(f.Name()) - return err + return "", err } f.Close() - return os.Rename(f.Name(), file) + return f.Name(), nil +} + +func writeKeyFile(file string, content []byte) error { + name, err := writeTemporaryKeyFile(file, content) + if err != nil { + return err + } + return os.Rename(name, file) } // keyFileName implements the naming convention for keyfiles: diff --git a/accounts/keystore/keystore.go b/accounts/keystore/keystore.go index d4ea66f2e4..617e8aa2fc 100644 --- a/accounts/keystore/keystore.go +++ b/accounts/keystore/keystore.go @@ -77,7 +77,7 @@ type unlocked struct { // NewKeyStore creates a keystore for the given directory. func NewKeyStore(keydir string, scryptN, scryptP int) *KeyStore { keydir, _ = filepath.Abs(keydir) - ks := &KeyStore{storage: &keyStorePassphrase{keydir, scryptN, scryptP}} + ks := &KeyStore{storage: &keyStorePassphrase{keydir, scryptN, scryptP, false}} ks.init(keydir) return ks } diff --git a/accounts/keystore/keystore_passphrase.go b/accounts/keystore/keystore_passphrase.go index 9d29b7fa97..93d68c9721 100644 --- a/accounts/keystore/keystore_passphrase.go +++ b/accounts/keystore/keystore_passphrase.go @@ -72,6 +72,10 @@ type keyStorePassphrase struct { keysDirPath string scryptN int scryptP int + // skipKeyFileVerification disables the security-feature which does + // reads and decrypts any newly created keyfiles. This should be 'false' in all + // cases except tests -- setting this to 'true' is not recommended. + skipKeyFileVerification bool } func (ks keyStorePassphrase) GetKey(addr common.Address, filename, auth string) (*Key, error) { @@ -93,7 +97,7 @@ func (ks keyStorePassphrase) GetKey(addr common.Address, filename, auth string) // StoreKey generates a key, encrypts with 'auth' and stores in the given directory func StoreKey(dir, auth string, scryptN, scryptP int) (common.Address, error) { - _, a, err := storeNewKey(&keyStorePassphrase{dir, scryptN, scryptP}, rand.Reader, auth) + _, a, err := storeNewKey(&keyStorePassphrase{dir, scryptN, scryptP, false}, rand.Reader, auth) return a.Address, err } @@ -102,7 +106,25 @@ func (ks keyStorePassphrase) StoreKey(filename string, key *Key, auth string) er if err != nil { return err } - return writeKeyFile(filename, keyjson) + // Write into temporary file + tmpName, err := writeTemporaryKeyFile(filename, keyjson) + if err != nil { + return err + } + if !ks.skipKeyFileVerification { + // Verify that we can decrypt the file with the given password. + _, err = ks.GetKey(key.Address, tmpName, auth) + if err != nil { + msg := "an error was encountered when saving and verifying the keystore file. \n" + + "This indicates that the keystore is corrupted. \n" + + "The corrupted file is stored at \n%v\n" + + "Please file a ticket at:\n\n" + + "https://github.com/ethereum/go-ethereum/issues." + + "The error was : %s" + return fmt.Errorf(msg, tmpName, err) + } + } + return os.Rename(tmpName, filename) } func (ks keyStorePassphrase) JoinPath(filename string) string { diff --git a/accounts/keystore/keystore_plain_test.go b/accounts/keystore/keystore_plain_test.go index 69dee3e3dd..a42c75c4ad 100644 --- a/accounts/keystore/keystore_plain_test.go +++ b/accounts/keystore/keystore_plain_test.go @@ -36,7 +36,7 @@ func tmpKeyStoreIface(t *testing.T, encrypted bool) (dir string, ks keyStore) { t.Fatal(err) } if encrypted { - ks = &keyStorePassphrase{d, veryLightScryptN, veryLightScryptP} + ks = &keyStorePassphrase{d, veryLightScryptN, veryLightScryptP, true} } else { ks = &keyStorePlain{d} } @@ -190,7 +190,7 @@ func TestV1_1(t *testing.T) { func TestV1_2(t *testing.T) { t.Parallel() - ks := &keyStorePassphrase{"testdata/v1", LightScryptN, LightScryptP} + ks := &keyStorePassphrase{"testdata/v1", LightScryptN, LightScryptP, true} addr := common.HexToAddress("cb61d5a9c4896fb9658090b597ef0e7be6f7b67e") file := "testdata/v1/cb61d5a9c4896fb9658090b597ef0e7be6f7b67e/cb61d5a9c4896fb9658090b597ef0e7be6f7b67e" k, err := ks.GetKey(addr, file, "g")