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 <fjl@twurst.com>

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()

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 {
+
+	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
 	}
 
-	key, err := hex.DecodeString(string(buf))
-	if 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)
 }

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",
+		},
+	}
+
+	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)
 		}
 	}
+}
 
-	ioutil.WriteFile(fileName0, []byte(testPrivHex), 0600)
-	defer os.Remove(fileName0)
-
-	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)
+	key, _ := HexToECDSA(testPrivHex)
+	if err := SaveECDSA(file, key); err != nil {
+		t.Fatal(err)
+	}
+	loaded, err := LoadECDSA(file)
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer os.Remove(fileName1)
-
-	key1, err := LoadECDSA(fileName1)
-	if err != nil {
-		t.Fatal(err)
+	if !reflect.DeepEqual(key, loaded) {
+		t.Fatal("loaded key not equal to saved key")
 	}
-	checkKey(key1)
 }
 
 func TestValidateSignatureValues(t *testing.T) {