signer: address review concerns, check sign in integer parsing

cmd/clef/main.go

@@ -671,7 +671,7 @@ func testExternalUI(api *core.SignerAPI) {
 		}
 	}
 	var delay = 1 * time.Second
-	//Test display of info and error
+	// Test display of info and error
 	{
 		api.UI.ShowInfo("If you see this message, enter 'yes' to next question")
 		time.Sleep(delay)

signer/core/signed_data.go

@@ -327,7 +327,10 @@ func (api *SignerAPI) SignTypedData(ctx context.Context, addr common.MixedcaseAd
 	}
 	rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash)))
 	sighash := crypto.Keccak256(rawData)
-	message := typedData.Format()
+	message, err := typedData.Format()
+	if err != nil {
+		return nil, err
+	}
 	req := &SignDataRequest{ContentType: DataTyped.Mime, Rawdata: rawData, Message: message, Hash: sighash}
 	signature, err := api.sign(addr, req, true)
 	if err != nil {
@@ -482,8 +485,12 @@ func (typedData *TypedData) EncodeData(primaryType string, data map[string]inter
 	return buffer.Bytes(), nil
 }
 
-func parseIntegerType(encType string, encValue interface{}) (*big.Int, error) {
-	length := 0
+func parseInteger(encType string, encValue interface{}) (*big.Int, error) {
+	var (
+		length = 0
+		signed = strings.HasPrefix(encType, "int")
+		b      *big.Int
+	)
 	if encType == "int" || encType == "uint" {
 		length = 256
 	} else {
@@ -499,7 +506,6 @@ func parseIntegerType(encType string, encValue interface{}) (*big.Int, error) {
 		}
 		length = atoiSize
 	}
-	var b *big.Int
 	switch v := encValue.(type) {
 	case *math.HexOrDecimal256:
 		b = (*big.Int)(v)
@@ -524,6 +530,9 @@ func parseIntegerType(encType string, encValue interface{}) (*big.Int, error) {
 	if b.BitLen() > length {
 		return nil, fmt.Errorf("integer larger than '%v'", encType)
 	}
+	if !signed && b.Sign() == -1 {
+		return nil, fmt.Errorf("invalid negative value for unsigned type %v", encType)
+	}
 	return b, nil
 }
 
@@ -577,7 +586,7 @@ func (typedData *TypedData) EncodePrimitiveValue(encType string, encValue interf
 		}
 	}
 	if strings.HasPrefix(encType, "int") || strings.HasPrefix(encType, "uint") {
-		b, err := parseIntegerType(encType, encValue)
+		b, err := parseInteger(encType, encValue)
 		if err != nil {
 			return nil, err
 		}
@@ -680,35 +689,32 @@ func (typedData *TypedData) Map() map[string]interface{} {
 	return dataMap
 }
 
-// PrettyPrint generates a nice output to help the users
-// of clef present data in their apps
-func (typedData *TypedData) PrettyPrint() string {
-	output := bytes.Buffer{}
-	formatted := typedData.Format()
-	for _, item := range formatted {
-		output.WriteString(fmt.Sprintf("%v\n", item.Pprint(0)))
-	}
-	return output.String()
-}
-
 // Format returns a representation of typedData, which can be easily displayed by a user-interface
 // without in-depth knowledge about 712 rules
-func (typedData *TypedData) Format() []*NameValueType {
+func (typedData *TypedData) Format() ([]*NameValueType, error) {
+	domain, err := typedData.formatData("EIP712Domain", typedData.Domain.Map())
+	if err != nil {
+		return nil, err
+	}
+	ptype, err := typedData.formatData(typedData.PrimaryType, typedData.Message)
+	if err != nil {
+		return nil, err
+	}
 	var nvts []*NameValueType
 	nvts = append(nvts, &NameValueType{
 		Name:  "EIP712Domain",
-		Value: typedData.formatData("EIP712Domain", typedData.Domain.Map()),
+		Value: domain,
 		Typ:   "domain",
 	})
 	nvts = append(nvts, &NameValueType{
 		Name:  typedData.PrimaryType,
-		Value: typedData.formatData(typedData.PrimaryType, typedData.Message),
+		Value: ptype,
 		Typ:   "primary type",
 	})
-	return nvts
+	return nvts, nil
 }
 
-func (typedData *TypedData) formatData(primaryType string, data map[string]interface{}) []*NameValueType {
+func (typedData *TypedData) formatData(primaryType string, data map[string]interface{}) ([]*NameValueType, error) {
 	var output []*NameValueType
 
 	// Add field contents. Structs and arrays have special handlers.
@@ -725,50 +731,70 @@ func (typedData *TypedData) formatData(primaryType string, data map[string]inter
 			for _, v := range arrayValue {
 				if typedData.Types[parsedType] != nil {
 					mapValue, _ := v.(map[string]interface{})
-					mapOutput := typedData.formatData(parsedType, mapValue)
+					mapOutput, err := typedData.formatData(parsedType, mapValue)
+					if err != nil {
+						return nil, err
+					}
 					item.Value = mapOutput
 				} else {
-					primitiveOutput := formatPrimitiveValue(field.Type, encValue)
+					primitiveOutput, err := formatPrimitiveValue(field.Type, encValue)
+					if err != nil {
+						return nil, err
+					}
 					item.Value = primitiveOutput
 				}
 			}
 		} else if typedData.Types[field.Type] != nil {
 			if mapValue, ok := encValue.(map[string]interface{}); ok {
-				mapOutput := typedData.formatData(field.Type, mapValue)
+				mapOutput, err := typedData.formatData(field.Type, mapValue)
+				if err != nil {
+					return nil, err
+				}
 				item.Value = mapOutput
 			} else {
 				item.Value = "<nil>"
 			}
 		} else {
-			primitiveOutput := formatPrimitiveValue(field.Type, encValue)
+			primitiveOutput, err := formatPrimitiveValue(field.Type, encValue)
+			if err != nil {
+				return nil, err
+			}
 			item.Value = primitiveOutput
 		}
 		output = append(output, item)
 	}
-	return output
+	return output, nil
 }
 
-func formatPrimitiveValue(encType string, encValue interface{}) string {
+func formatPrimitiveValue(encType string, encValue interface{}) (string, error) {
 	switch encType {
 	case "address":
-		stringValue, _ := encValue.(string)
-		return common.HexToAddress(stringValue).String()
+		if stringValue, ok := encValue.(string); !ok {
+			return "", fmt.Errorf("could not format value %v as address", encValue)
+		} else {
+			return common.HexToAddress(stringValue).String(), nil
+		}
 	case "bool":
-		boolValue, _ := encValue.(bool)
-		return fmt.Sprintf("%t", boolValue)
+		if boolValue, ok := encValue.(bool); !ok {
+			return "", fmt.Errorf("could not format value %v as bool", encValue)
+		} else {
+			return fmt.Sprintf("%t", boolValue), nil
+		}
 	case "bytes", "string":
-		return fmt.Sprintf("%s", encValue)
+		return fmt.Sprintf("%s", encValue), nil
 	}
 	if strings.HasPrefix(encType, "bytes") {
-		return fmt.Sprintf("%s", encValue)
-	} else if strings.HasPrefix(encType, "uint") || strings.HasPrefix(encType, "int") {
-		b, err := parseIntegerType(encType, encValue)
-		if err != nil {
-			return fmt.Sprintf("ERROR: %v", err)
-		}
-		return fmt.Sprintf("%d (0x%x)", b, b)
+		return fmt.Sprintf("%s", encValue), nil
+
 	}
-	return "NA"
+	if strings.HasPrefix(encType, "uint") || strings.HasPrefix(encType, "int") {
+		if b, err := parseInteger(encType, encValue); err != nil {
+			return "", err
+		} else {
+			return fmt.Sprintf("%d (0x%x)", b, b), nil
+		}
+	}
+	return "", fmt.Errorf("unhandled type %v", encType)
 }
 
 // NameValueType is a very simple struct with Name, Value and Type. It's meant for simple

signer/core/signed_data_internal_test.go

@@ -0,0 +1,51 @@
+// Copyright 2019 The go-ethereum Authors
+// This file is part of go-ethereum.
+//
+// go-ethereum is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// go-ethereum is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with go-ethereum. If not, see <http://www.gnu.org/licenses/>.
+//
+package core
+
+import (
+	"math/big"
+	"testing"
+)
+
+func TestParseInteger(t *testing.T) {
+	for i, tt := range []struct {
+		t   string
+		v   interface{}
+		exp *big.Int
+	}{
+		{"uint32", "-123", nil},
+		{"int32", "-123", big.NewInt(-123)},
+		{"uint32", "0xff", big.NewInt(0xff)},
+		{"int8", "0xffff", nil},
+	} {
+		res, err := parseInteger(tt.t, tt.v)
+		if tt.exp == nil && res == nil {
+			continue
+		}
+		if tt.exp == nil && res != nil {
+			t.Errorf("test %d, got %v, expected nil", i, res)
+			continue
+		}
+		if tt.exp != nil && res == nil {
+			t.Errorf("test %d, got '%v', expected %v", i, err, tt.exp)
+			continue
+		}
+		if tt.exp.Cmp(res) != 0 {
+			t.Errorf("test %d, got %v expected %v", i, res, tt.exp)
+		}
+	}
+}

signer/core/signed_data_test.go

@@ -319,7 +319,7 @@ func TestFormatter(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unmarshalling failed '%v'", err)
 	}
-	formatted := d.Format()
+	formatted, _ := d.Format()
 	for _, item := range formatted {
 		fmt.Printf("'%v'\n", item.Pprint(0))
 	}
@@ -403,7 +403,6 @@ func TestFuzzerFiles(t *testing.T) {
 		if verbose && err != nil {
 			fmt.Printf("%d, EncodeData[2] err: %v\n", i, err)
 		}
-		typedData.PrettyPrint()
 		typedData.Format()
 	}
 }