From fe9ffa5953a28d570478ac61238218a195fe0ccb Mon Sep 17 00:00:00 2001 From: Adam Schmideg Date: Wed, 8 Apr 2020 16:01:11 +0200 Subject: [PATCH] crypto: improve error messages in LoadECDSA (#20718) This improves error messages when the file is too short or too long. Also rewrite the test for SaveECDSA because LoadECDSA has its own test now. Co-authored-by: Felix Lange --- cmd/geth/accountcmd_test.go | 31 ++++++++++++++ crypto/crypto.go | 56 ++++++++++++++++++++---- crypto/crypto_test.go | 85 ++++++++++++++++++++++++++++--------- 3 files changed, 143 insertions(+), 29 deletions(-) diff --git a/cmd/geth/accountcmd_test.go b/cmd/geth/accountcmd_test.go index 186db3c6b..3ee4278e5 100644 --- a/cmd/geth/accountcmd_test.go +++ b/cmd/geth/accountcmd_test.go @@ -88,6 +88,37 @@ Path of the secret key file: .*UTC--.+--[0-9a-f]{40} `) } +func TestAccountImport(t *testing.T) { + tests := []struct{ key, output string }{ + { + key: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + output: "Address: {fcad0b19bb29d4674531d6f115237e16afce377c}\n", + }, + { + key: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef1", + output: "Fatal: Failed to load the private key: invalid character '1' at end of key file\n", + }, + } + for _, test := range tests { + importAccountWithExpect(t, test.key, test.output) + } +} + +func importAccountWithExpect(t *testing.T, key string, expected string) { + dir := tmpdir(t) + keyfile := filepath.Join(dir, "key.prv") + if err := ioutil.WriteFile(keyfile, []byte(key), 0600); err != nil { + t.Error(err) + } + passwordFile := filepath.Join(dir, "password.txt") + if err := ioutil.WriteFile(passwordFile, []byte("foobar"), 0600); err != nil { + t.Error(err) + } + geth := runGeth(t, "account", "import", keyfile, "-password", passwordFile) + defer geth.ExpectExit() + geth.Expect(expected) +} + func TestAccountNewBadRepeat(t *testing.T) { geth := runGeth(t, "account", "new", "--lightkdf") defer geth.ExpectExit() diff --git a/crypto/crypto.go b/crypto/crypto.go index 2869b4c19..1f43ad15e 100644 --- a/crypto/crypto.go +++ b/crypto/crypto.go @@ -17,6 +17,7 @@ package crypto import ( + "bufio" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" @@ -158,29 +159,67 @@ func FromECDSAPub(pub *ecdsa.PublicKey) []byte { // HexToECDSA parses a secp256k1 private key. func HexToECDSA(hexkey string) (*ecdsa.PrivateKey, error) { b, err := hex.DecodeString(hexkey) - if err != nil { - return nil, errors.New("invalid hex string") + if byteErr, ok := err.(hex.InvalidByteError); ok { + return nil, fmt.Errorf("invalid hex character %q in private key", byte(byteErr)) + } else if err != nil { + return nil, errors.New("invalid hex data for private key") } return ToECDSA(b) } // LoadECDSA loads a secp256k1 private key from the given file. func LoadECDSA(file string) (*ecdsa.PrivateKey, error) { - buf := make([]byte, 64) fd, err := os.Open(file) if err != nil { return nil, err } defer fd.Close() - if _, err := io.ReadFull(fd, buf); err != nil { - return nil, err - } - key, err := hex.DecodeString(string(buf)) + r := bufio.NewReader(fd) + buf := make([]byte, 64) + n, err := readASCII(buf, r) if err != nil { return nil, err + } else if n != len(buf) { + return nil, fmt.Errorf("key file too short, want 64 hex characters") + } + if err := checkKeyFileEnd(r); err != nil { + return nil, err + } + + return HexToECDSA(string(buf)) +} + +// readASCII reads into 'buf', stopping when the buffer is full or +// when a non-printable control character is encountered. +func readASCII(buf []byte, r *bufio.Reader) (n int, err error) { + for ; n < len(buf); n++ { + buf[n], err = r.ReadByte() + switch { + case err == io.EOF || buf[n] < '!': + return n, nil + case err != nil: + return n, err + } + } + return n, nil +} + +// checkKeyFileEnd skips over additional newlines at the end of a key file. +func checkKeyFileEnd(r *bufio.Reader) error { + for i := 0; ; i++ { + b, err := r.ReadByte() + switch { + case err == io.EOF: + return nil + case err != nil: + return err + case b != '\n' && b != '\r': + return fmt.Errorf("invalid character %q at end of key file", b) + case i >= 2: + return errors.New("key file too long, want 64 hex characters") + } } - return ToECDSA(key) } // SaveECDSA saves a secp256k1 private key to the given file with @@ -190,6 +229,7 @@ func SaveECDSA(file string, key *ecdsa.PrivateKey) error { return ioutil.WriteFile(file, []byte(k), 0600) } +// GenerateKey generates a new private key. func GenerateKey() (*ecdsa.PrivateKey, error) { return ecdsa.GenerateKey(S256(), rand.Reader) } diff --git a/crypto/crypto_test.go b/crypto/crypto_test.go index 177c19c0c..f71ae8232 100644 --- a/crypto/crypto_test.go +++ b/crypto/crypto_test.go @@ -139,39 +139,82 @@ func TestNewContractAddress(t *testing.T) { checkAddr(t, common.HexToAddress("c9ddedf451bc62ce88bf9292afb13df35b670699"), caddr2) } -func TestLoadECDSAFile(t *testing.T) { - keyBytes := common.FromHex(testPrivHex) - fileName0 := "test_key0" - fileName1 := "test_key1" - checkKey := func(k *ecdsa.PrivateKey) { - checkAddr(t, PubkeyToAddress(k.PublicKey), common.HexToAddress(testAddrHex)) - loadedKeyBytes := FromECDSA(k) - if !bytes.Equal(loadedKeyBytes, keyBytes) { - t.Fatalf("private key mismatch: want: %x have: %x", keyBytes, loadedKeyBytes) - } +func TestLoadECDSA(t *testing.T) { + tests := []struct { + input string + err string + }{ + // good + {input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"}, + {input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef\n"}, + {input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef\n\r"}, + {input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef\r\n"}, + {input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef\n\n"}, + {input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef\n\r"}, + // bad + { + input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde", + err: "key file too short, want 64 hex characters", + }, + { + input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde\n", + err: "key file too short, want 64 hex characters", + }, + { + input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdeX", + err: "invalid hex character 'X' in private key", + }, + { + input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdefX", + err: "invalid character 'X' at end of key file", + }, + { + input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef\n\n\n", + err: "key file too long, want 64 hex characters", + }, } - ioutil.WriteFile(fileName0, []byte(testPrivHex), 0600) - defer os.Remove(fileName0) + for _, test := range tests { + f, err := ioutil.TempFile("", "loadecdsa_test.*.txt") + if err != nil { + t.Fatal(err) + } + filename := f.Name() + f.WriteString(test.input) + f.Close() + + _, err = LoadECDSA(filename) + switch { + case err != nil && test.err == "": + t.Fatalf("unexpected error for input %q:\n %v", test.input, err) + case err != nil && err.Error() != test.err: + t.Fatalf("wrong error for input %q:\n %v", test.input, err) + case err == nil && test.err != "": + t.Fatalf("LoadECDSA did not return error for input %q", test.input) + } + } +} - key0, err := LoadECDSA(fileName0) +func TestSaveECDSA(t *testing.T) { + f, err := ioutil.TempFile("", "saveecdsa_test.*.txt") if err != nil { t.Fatal(err) } - checkKey(key0) + file := f.Name() + f.Close() + defer os.Remove(file) - // again, this time with SaveECDSA instead of manual save: - err = SaveECDSA(fileName1, key0) - if err != nil { + key, _ := HexToECDSA(testPrivHex) + if err := SaveECDSA(file, key); err != nil { t.Fatal(err) } - defer os.Remove(fileName1) - - key1, err := LoadECDSA(fileName1) + loaded, err := LoadECDSA(file) if err != nil { t.Fatal(err) } - checkKey(key1) + if !reflect.DeepEqual(key, loaded) { + t.Fatal("loaded key not equal to saved key") + } } func TestValidateSignatureValues(t *testing.T) {