signer: implement validation rules, change signature of call_info

cmd/signer/README.md

@@ -506,29 +506,137 @@ Invoked when there's a transaction for approval.
 
 #### Sample call
 
+Here's a method invocation:
+```bash
+
+curl -i -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","method":"account_signTransaction","params":[{"from":"0x694267f14675d7e1b9494fd8d72fefe1755710fa","gas":"0x333","gasPrice":"0x1","nonce":"0x0","to":"0x07a565b7ed7d7a678680a4c162885bedbb695fe0", "value":"0x0", "data":"0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"},"safeSend(address)"],"id":67}' http://localhost:8550/
+```
+
 ```json
 
 {
   "jsonrpc": "2.0",
+  "id": 1,
   "method": "ApproveTx",
-	"params": [{
+  "params": [
+    {
       "transaction": {
-			"to": "0xae967917c465db8578ca9024c205720b1a3651A9",
+        "from": "0x0x694267f14675d7e1b9494fd8d72fefe1755710fa",
+        "to": "0x0x07a565b7ed7d7a678680a4c162885bedbb695fe0",
         "gas": "0x333",
-			"gasPrice": "0x123",
+        "gasPrice": "0x1",
-			"value": "0x10",
+        "value": "0x0",
-			"data": "0xd7a5865800000000000000000000000000000000000000000000000000000000000000ff",
+        "nonce": "0x0",
-			"nonce": "0x0"
+        "data": "0x4401a6e40000000000000000000000000000000000000000000000000000000000000012",
+        "input": null
+      },
+      "call_info": {
+        "Messages": [
+          {
+            "type": "WARNING",
+            "message": "Invalid checksum on to-address"
+          },
+          {
+            "type": "Info",
+            "message": "safeSend(address: 0x0000000000000000000000000000000000000012)"
+          }
+        ]
       },
-		"fromaccount": "0xAe967917c465db8578ca9024c205720b1a3651A9",
-		"call_info": "Warning! Could not validate ABI-data against calldata\nSupplied ABI spec does not contain method signature in data: 0xd7a58658",
       "meta": {
-			"remote": "127.0.0.1:34572",
+        "remote": "127.0.0.1:48486",
         "local": "localhost:8550",
         "scheme": "HTTP/1.1"
       }
-	}],
+    }
-	"id": 1
+  ]
+}
+
+```
+
+The same method invocation, but with invalid data:
+```bash
+
+curl -i -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","method":"account_signTransaction","params":[{"from":"0x694267f14675d7e1b9494fd8d72fefe1755710fa","gas":"0x333","gasPrice":"0x1","nonce":"0x0","to":"0x07a565b7ed7d7a678680a4c162885bedbb695fe0", "value":"0x0", "data":"0x4401a6e40000000000000002000000000000000000000000000000000000000000000012"},"safeSend(address)"],"id":67}' http://localhost:8550/
+```
+
+```json
+
+{
+  "jsonrpc": "2.0",
+  "id": 1,
+  "method": "ApproveTx",
+  "params": [
+    {
+      "transaction": {
+        "from": "0x0x694267f14675d7e1b9494fd8d72fefe1755710fa",
+        "to": "0x0x07a565b7ed7d7a678680a4c162885bedbb695fe0",
+        "gas": "0x333",
+        "gasPrice": "0x1",
+        "value": "0x0",
+        "nonce": "0x0",
+        "data": "0x4401a6e40000000000000002000000000000000000000000000000000000000000000012",
+        "input": null
+      },
+      "call_info": {
+        "Messages": [
+          {
+            "type": "WARNING",
+            "message": "Invalid checksum on to-address"
+          },
+          {
+            "type": "WARNING",
+            "message": "Transaction data did not match ABI-interface: WARNING: Supplied data is stuffed with extra data. \nWant 0000000000000002000000000000000000000000000000000000000000000012\nHave 0000000000000000000000000000000000000000000000000000000000000012\nfor method safeSend(address)"
+          }
+        ]
+      },
+      "meta": {
+        "remote": "127.0.0.1:48492",
+        "local": "localhost:8550",
+        "scheme": "HTTP/1.1"
+      }
+    }
+  ]
+}
+
+
+```
+
+One which has missing `to`, but with no `data`:
+
+
+```json
+
+{
+  "jsonrpc": "2.0",
+  "id": 3,
+  "method": "ApproveTx",
+  "params": [
+    {
+      "transaction": {
+        "from": "",
+        "to": null,
+        "gas": "0x0",
+        "gasPrice": "0x0",
+        "value": "0x0",
+        "nonce": "0x0",
+        "data": null,
+        "input": null
+      },
+      "call_info": {
+        "Messages": [
+          {
+            "type": "CRITICAL",
+            "message": "Tx will create contract with empty code!"
+          }
+        ]
+      },
+      "meta": {
+        "remote": "signer binary",
+        "local": "main",
+        "scheme": "in-proc"
+      }
+    }
+  ]
 }
 ```
 

cmd/signer/core/abihelper.go

@@ -161,6 +161,11 @@ type AbiDb struct {
 	db map[string]string
 }
 
+// NewEmptyAbiDB exists for test purposes
+func NewEmptyAbiDB() (*AbiDb, error) {
+	return &AbiDb{make(map[string]string)}, nil
+}
+
 // NewAbiDBFromFile loads signature database from file, and
 // errors if the file is not valid json. Does no other validation of contents
 func NewAbiDBFromFile(path string) (*AbiDb, error) {

cmd/signer/core/api.go

@@ -23,9 +23,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"math/big"
-
-	"bytes"
-
 	"reflect"
 
 	"github.com/ethereum/go-ethereum/accounts"
@@ -87,7 +84,7 @@ type SignerAPI struct {
 	chainID   *big.Int
 	am        *accounts.Manager
 	UI        SignerUI
-	abidb   AbiDb
+	validator *Validator
 }
 
 // Metadata about a request
@@ -127,7 +124,7 @@ type (
 	// SignTxRequest contains info about a Transaction to sign
 	SignTxRequest struct {
 		Transaction SendTxArgs         `json:"transaction"`
-		Callinfo    string     `json:"call_info"`
+		Callinfo    *ValidationMessages `json:"call_info"`
 		Meta        Metadata           `json:"meta"`
 	}
 	// SignTxResponse result from SignTxRequest
@@ -229,7 +226,7 @@ func NewSignerAPI(chainID int64, ksLocation string, noUSB bool, ui SignerUI, abi
 			log.Debug("Trezor support enabled")
 		}
 	}
-	return &SignerAPI{big.NewInt(chainID), accounts.NewManager(backends...), ui, *abidb}
+	return &SignerAPI{big.NewInt(chainID), accounts.NewManager(backends...), ui, NewValidator(abidb)}
 }
 
 // List returns the set of wallet this signer manages. Each wallet can contain
@@ -318,66 +315,21 @@ func logDiff(original *SignTxRequest, new *SignTxResponse) bool {
 	return modified
 }
 
-// determineCallInfo turns ABI-data + methodselector (if given) into a string suitable
-// to present to the user.
-func (api *SignerAPI) determineCallInfo(data []byte, methodSelector *string) string {
-
-	if len(data) < 4 {
-		return ""
-	}
-	var (
-		selector string
-		err      error
-	)
-	// Try to make sense of the data
-	if methodSelector == nil {
-		selector, err = api.abidb.LookupMethodSelector(data[:4])
-		if err != nil {
-			return errorWrapper{"Warning! Could not locate ABI", err}.String()
-		}
-	} else {
-		selector = *methodSelector
-	}
-	if selector != "" {
-		abiData, err := MethodSelectorToAbi(selector)
-		if err != nil {
-			return errorWrapper{"Warning! Could not validate ABI-data against calldata", err}.String()
-		} else {
-			var info *decodedCallData
-			info, err = parseCallData(data, string(abiData))
-			if err != nil {
-				return errorWrapper{"Warning! Could not validate ABI-data against calldata", err}.String()
-			} else {
-				return info.String()
-			}
-		}
-	}
-	return ""
-}
-
 // SignTransaction signs the given Transaction and returns it both as json and rlp-encoded form
 func (api *SignerAPI) SignTransaction(ctx context.Context, args SendTxArgs, methodSelector *string) (*ethapi.SignTransactionResult, error) {
 	var (
 		err    error
 		result SignTxResponse
-		data   []byte
 	)
-	// Prevent accidental erroneous usage of both 'input' and 'data'
+	msgs, err:= api.validator.ValidateTransaction(&args, methodSelector)
-	if args.Data != nil && args.Input != nil && !bytes.Equal(*args.Data, *args.Input) {
+	if err != nil {
-		return nil, errors.New(`Ambiguous request: moth "data" and "input" are set and are not identical`)
+		return nil, err
 	}
 
-	if args.Data != nil {
-		data = *args.Data
-	} else if args.Input != nil {
-		data = *args.Input
-		*args.Data = data
-		*args.Input = nil
-	}
 	req := SignTxRequest{
 		Transaction: args,
 		Meta:        MetadataFromContext(ctx),
-		Callinfo:    api.determineCallInfo(data, methodSelector),
+		Callinfo:    msgs,
 	}
 	// Process approval
 	result, err = api.UI.ApproveTx(&req)

cmd/signer/core/cliui.go

@@ -92,7 +92,7 @@ func (ui *CommandlineUI) confirm() bool {
 }
 
 func showMetadata(metadata Metadata) {
-	fmt.Printf("Request info:\n\t%v -> %v -> %v\n", metadata.Remote, metadata.Scheme, metadata.Local)
+	fmt.Printf("Request context:\n\t%v -> %v -> %v\n", metadata.Remote, metadata.Scheme, metadata.Local)
 }
 
 // ApproveTx prompt the user for confirmation to request to sign Transaction
@@ -117,9 +117,12 @@ func (ui *CommandlineUI) ApproveTx(request *SignTxRequest) (SignTxResponse, erro
 			fmt.Printf("data:  %v\n", common.Bytes2Hex(d))
 		}
 	}
-	if request.Callinfo != "" {
+	if request.Callinfo != nil {
-		fmt.Printf("\nNote: This Transaction contains data. Review abi-decoding info below:")
+		fmt.Printf("\nTransaction validation:\n")
-		fmt.Printf("\nCall info:\n\t%v\n", request.Callinfo)
+		for _,m := range request.Callinfo.Messages{
+			fmt.Printf("  * %s : %s", m.Typ, m.Message)
+		}
+		fmt.Println()
 
 	}
 	fmt.Printf("\n")

cmd/signer/core/stdioui.go

@@ -45,7 +45,12 @@ func NewStdIOUI() *StdIOUI {
 
 // dispatch sends a request over the stdio
 func (ui *StdIOUI) dispatch(serviceMethod string, args interface{}, reply interface{}) error{
-	err := ui.client.Call(&reply, serviceMethod, args)
+	var err error
+	if reply != nil{
+		err = ui.client.Call(nil, serviceMethod, args)
+	}else{
+		err = ui.client.Call(&reply, serviceMethod, args)
+	}
 	if err != nil {
 		log.Info("Error", "exc", err.Error())
 	}

cmd/signer/core/types.go

@@ -50,6 +50,14 @@ func (a Account) String() string {
 	}
 	return err.Error()
 }
+type ValidationInfo struct {
+	Typ     string `json:"type"`
+	Message string `json:"message"`
+}
+type ValidationMessages struct {
+	Messages []ValidationInfo
+}
+
 /*
 // TransactionArg represents a Transaction for the signer.
 type TransactionArg struct {

cmd/signer/core/validation.go

@@ -0,0 +1,152 @@
+// Copyright 2018 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 (
+	"bytes"
+	"errors"
+	"fmt"
+	"github.com/ethereum/go-ethereum/common"
+	"math/big"
+)
+
+// The validation package contains validation checks for transactions
+// - ABI-data validation
+// - Transaction semantics validation
+// The package provides warnings for typical pitfalls
+
+func (vs *ValidationMessages) crit(msg string) {
+	vs.Messages = append(vs.Messages, ValidationInfo{"CRITICAL", msg})
+}
+func (vs *ValidationMessages) warn(msg string) {
+	vs.Messages = append(vs.Messages, ValidationInfo{"WARNING", msg})
+}
+func (vs *ValidationMessages) info(msg string) {
+	vs.Messages = append(vs.Messages, ValidationInfo{"Info", msg})
+}
+
+type Validator struct {
+	db *AbiDb
+}
+
+func NewValidator(db *AbiDb) *Validator {
+	return &Validator{db}
+}
+
+// validateCallData checks if the ABI-data + methodselector (if given) can be parsed and seems to match
+func (v *Validator) validateCallData(msgs *ValidationMessages, data []byte, methodSelector *string) {
+	if len(data) == 0 {
+		return
+	}
+	if len(data) < 4 {
+		msgs.warn("Tx contains data which is not valid ABI")
+		return
+	}
+	var (
+		selector string
+		err      error
+	)
+	// Try to make sense of the data
+	if methodSelector != nil {
+		selector = *methodSelector
+	}
+
+	if selector == "" {
+		selector, err = v.db.LookupMethodSelector(data[:4])
+		if err != nil {
+			msgs.warn(fmt.Sprintf("Tx contains data, but the ABI signature could not be found: %v", err))
+			return
+		}
+	}
+	if selector == "" {
+		// No more to do that this stage
+		return
+	}
+	abiData, err := MethodSelectorToAbi(selector)
+	if err != nil {
+		msgs.warn(fmt.Sprintf("Transaction data did not match ABI-interface: %v", err))
+		return
+	}
+
+	info, err := parseCallData(data, string(abiData))
+	if err != nil {
+		msgs.warn(fmt.Sprintf("Transaction data did not match ABI-interface: %v", err))
+	} else {
+		msgs.info(info.String())
+	}
+	return
+}
+
+// validateSemantics checks if the transactions 'makes sense', and generate warnings for a couple of typical scenarios
+func (v *Validator) validate(msgs *ValidationMessages, txargs *SendTxArgs, methodSelector *string) error {
+	// Prevent accidental erroneous usage of both 'input' and 'data'
+	if txargs.Data != nil && txargs.Input != nil && !bytes.Equal(*txargs.Data, *txargs.Input) {
+		// This is a showstopper
+		return errors.New(`Ambiguous request: moth "data" and "input" are set and are not identical`)
+	}
+	var (
+		data []byte
+	)
+	// Place data on 'data', and nil 'input'
+	if txargs.Input != nil {
+		txargs.Data = txargs.Input
+		txargs.Input = nil
+	}
+	if txargs.Data != nil {
+		data = *txargs.Data
+	}
+
+	if txargs.To == nil {
+		//Contract creation should contain sufficient data to deploy a contract
+		// A typical error is omitting sender due to some quirk in the javascript call
+		// e.g. https://github.com/ethereum/go-ethereum/issues/16106
+		if len(data) == 0 {
+			if txargs.Value.ToInt().Cmp(big.NewInt(0)) > 0 {
+				// Sending ether into black hole
+				return errors.New(`Tx will create contract with value but empty code!`)
+			}
+			// No value submitted at least
+			msgs.crit("Tx will create contract with empty code!")
+		} else if len(data) < 40 { //Arbitrary limit
+			msgs.warn(fmt.Sprintf("Tx will will create contract, but payload is suspiciously small (%d b)", len(data)))
+		}
+		// methodSelector should be nil for contract creation
+		if methodSelector != nil {
+			msgs.warn("Tx will create contract, but method selector supplied; indicating intent to call a method.")
+		}
+
+	} else {
+		if !txargs.To.ValidChecksum() {
+			msgs.warn("Invalid checksum on to-address")
+		}
+		// Normal transaction
+		if bytes.Equal(txargs.To.Address().Bytes(), common.Address{}.Bytes()) {
+			// Sending to 0
+			msgs.crit("Tx destination is the zero address!")
+		}
+		// Validate calldata
+		v.validateCallData(msgs, data, methodSelector);
+	}
+	return nil
+}
+
+// ValidateTransaction does a number of checks on the supplied transaction, and returns either a list of warnings,
+// or an error, indicating that the transaction should be immediately rejected
+func (v *Validator) ValidateTransaction(txArgs *SendTxArgs, methodSelector *string) (*ValidationMessages, error) {
+	msgs := &ValidationMessages{}
+	return msgs, v.validate(msgs, txArgs, methodSelector)
+}

cmd/signer/core/validation_test.go

@@ -0,0 +1,140 @@
+// Copyright 2018 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 (
+	"fmt"
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/common/hexutil"
+	"math/big"
+	"testing"
+)
+
+func hexAddr(a string) common.Address { return common.BytesToAddress(common.FromHex(a)) }
+func mixAddr(a string) (*common.MixedcaseAddress, error) {
+	return common.NewMixedcaseAddressFromString(a)
+}
+func toHexBig(h string) hexutil.Big {
+	b := big.NewInt(0).SetBytes(common.FromHex(h))
+	return hexutil.Big(*b)
+}
+func toHexUint(h string) hexutil.Uint64 {
+	b := big.NewInt(0).SetBytes(common.FromHex(h))
+	return hexutil.Uint64(b.Uint64())
+}
+func dummyTxArgs(t txtestcase) *SendTxArgs {
+	to, _ := mixAddr(t.to)
+	from, _ := mixAddr(t.from)
+	n := toHexUint(t.n)
+	gas := toHexBig(t.g)
+	gasPrice := toHexBig(t.gp)
+	value := toHexBig(t.value)
+	var(
+		data, input *hexutil.Bytes
+	)
+	if t.d != ""{
+		a := hexutil.Bytes(common.FromHex(t.d))
+		data = &a
+	}
+	if t.i != ""{
+		a := hexutil.Bytes(common.FromHex(t.i))
+		input = &a
+
+	}
+	return &SendTxArgs{
+		From:     *from,
+		To:       to,
+		Value:    value,
+		Nonce:    n,
+		GasPrice: gas,
+		Gas:      gasPrice,
+		Data:     data,
+		Input:    input,
+	}
+}
+
+type txtestcase struct {
+	from, to, n, g, gp, value, d, i string
+	expectErr                       bool
+	numMessages                     int
+}
+
+func TestValidator(t *testing.T) {
+	var (
+		// use empty db, there are other tests for the abi-specific stuff
+		db, _ = NewEmptyAbiDB()
+		v     = NewValidator(db)
+	)
+	testcases := []txtestcase{
+		// Invalid to checksum
+		{from: "000000000000000000000000000000000000dead", to: "000000000000000000000000000000000000dead",
+			n: "0x01", g: "0x20", gp: "0x40", value: "0x01", numMessages: 1},
+			// valid 0x000000000000000000000000000000000000dEaD
+		{from: "000000000000000000000000000000000000dead", to: "0x000000000000000000000000000000000000dEaD",
+			n: "0x01", g: "0x20", gp: "0x40", value: "0x01", numMessages: 0},
+			// conflicting input and data
+		{from: "000000000000000000000000000000000000dead", to: "0x000000000000000000000000000000000000dEaD",
+			n: "0x01", g: "0x20", gp: "0x40", value: "0x01", d: "0x01", i: "0x02", expectErr: true, },
+			// Data can't be parsed
+		{from: "000000000000000000000000000000000000dead", to: "0x000000000000000000000000000000000000dEaD",
+			n: "0x01", g: "0x20", gp: "0x40", value: "0x01", d: "0x0102", numMessages:1 },
+		// Data (on Input) can't be parsed
+		{from: "000000000000000000000000000000000000dead", to: "0x000000000000000000000000000000000000dEaD",
+			n: "0x01", g: "0x20", gp: "0x40", value: "0x01", i: "0x0102", numMessages:1 },
+		// Send to 0
+		{from: "000000000000000000000000000000000000dead", to: "0x0000000000000000000000000000000000000000",
+			n: "0x01", g: "0x20", gp: "0x40", value: "0x01", numMessages: 1},
+		// Create empty contract (no value)
+		{from: "000000000000000000000000000000000000dead", to: "",
+			n: "0x01", g: "0x20", gp: "0x40", value: "0x00", numMessages: 1},
+		// Create empty contract (with value)
+		{from: "000000000000000000000000000000000000dead", to: "",
+			n: "0x01", g: "0x20", gp: "0x40", value: "0x01", expectErr: true},
+		// Small payload for create
+		{from: "000000000000000000000000000000000000dead", to: "",
+			n: "0x01", g: "0x20", gp: "0x40", value: "0x01",d:"0x01", numMessages: 1},
+
+
+	}
+	for i, test := range testcases {
+		msgs, err := v.ValidateTransaction(dummyTxArgs(test), nil)
+		if err == nil && test.expectErr {
+			t.Errorf("Test %d, expected error", i)
+			for _, msg := range msgs.Messages {
+				fmt.Printf("* %s: %s\n", msg.Typ, msg.Message)
+			}
+		}
+		if err != nil && !test.expectErr {
+			t.Errorf("Test %d, unexpected error: %v", i, err)
+		}
+		if err == nil {
+			got := len(msgs.Messages)
+			if  got != test.numMessages {
+				for _, msg := range msgs.Messages {
+					fmt.Printf("* %s: %s\n", msg.Typ, msg.Message)
+				}
+				t.Errorf("Test %d, expected %d messages, got %d", i,test.numMessages, got)
+			}else{
+				//Debug printout, remove later
+				for _, msg := range msgs.Messages {
+					fmt.Printf("* [%d] %s: %s\n", i, msg.Typ, msg.Message)
+				}
+				fmt.Println()
+			}
+		}
+	}
+}

cmd/signer/rules/rules_test.go

@@ -9,8 +9,8 @@ import (
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/internal/ethapi"
 	"math/big"
-	"testing"
 	"strings"
+	"testing"
 )
 
 const JS = `
@@ -167,7 +167,7 @@ func TestSignTxRequest(t *testing.T) {
 		Transaction: core.SendTxArgs{
 			From: *from,
 			To:   to},
-		Callinfo: "",
+		Callinfo: nil,
 		Meta:     core.Metadata{"remoteip", "localip", "inproc"},
 	})
 	if err != nil {
@@ -425,7 +425,11 @@ func dummyTx(value hexutil.Big) *core.SignTxRequest {
 			GasPrice: gas,
 			Gas:      gasPrice,
 		},
-		Callinfo: "Warning, all your base are bellong to us",
+		Callinfo: &core.ValidationMessages{
+			[]core.ValidationInfo{
+				{"Warning", "All your base are bellong to us"},
+			},
+		},
 		Meta: core.Metadata{"remoteip", "localip", "inproc"},
 	}
 }
@@ -530,7 +534,6 @@ func (d *dontCallMe) OnApprovedTx(tx ethapi.SignTransactionResult) {
 	d.t.Fatalf("Did not expect next-handler to be called")
 }
 
-
 //TestContextIsCleared tests that the rule-engine does not retain variables over several requests.
 // if it does, that would be bad since developers may rely on that to store data,
 // instead of using the disk-based data storage