From 4fa2cb033266b69520c0761cb2e9068fc0ffdf86 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 13 Dec 2017 11:21:05 +0100 Subject: [PATCH] cmd/signer: fix bugs, improve abi detection, abi argument display --- cmd/signer/abihelper.go | 69 ++++++++++++++++++++++++++++++------ cmd/signer/abihelper_test.go | 36 +++++++++++++++++++ cmd/signer/api.go | 36 ++++++++++++------- cmd/signer/api_test.go | 18 ++++++++-- cmd/signer/cliui.go | 25 +++++++++---- cmd/signer/main.go | 10 +++--- cmd/signer/stdioui.go | 2 +- 7 files changed, 157 insertions(+), 39 deletions(-) diff --git a/cmd/signer/abihelper.go b/cmd/signer/abihelper.go index 4e48055dc0..720a482147 100644 --- a/cmd/signer/abihelper.go +++ b/cmd/signer/abihelper.go @@ -24,10 +24,12 @@ import ( "github.com/ethereum/go-ethereum/common" "io/ioutil" "strings" + + "regexp" ) type decodedArgument struct { - soltype string + soltype abi.Argument value interface{} } type decodedCallData struct { @@ -36,9 +38,19 @@ type decodedCallData struct { inputs []decodedArgument } +// String implements stringer interface, tries to use the underlying value-type func (arg decodedArgument) String() string { - return fmt.Sprintf("%v: %v", arg.soltype, arg.value) + var value string + switch arg.value.(type) { + case fmt.Stringer: + value = arg.value.(fmt.Stringer).String() + default: + value = fmt.Sprintf("%v", arg.value) + } + return fmt.Sprintf("%v: %v", arg.soltype.Type.String(), value) } + +// String implements stringer interface for decodedCallData func (cd decodedCallData) String() string { args := make([]string, len(cd.inputs)) for i, arg := range cd.inputs { @@ -55,16 +67,16 @@ func parseCallData(calldata []byte, abidata string) (*decodedCallData, error) { return nil, fmt.Errorf("Invalid ABI-data, incomplete method signature of (%d bytes)", len(calldata)) } - abispec, err := abi.JSON(strings.NewReader(abidata)) - if err != nil { - return nil, fmt.Errorf("Failed parsing JSON ABI: %v", err) - } - sigdata, argdata := calldata[:4], calldata[4:] if len(argdata)%32 != 0 { return nil, fmt.Errorf("Not ABI-encoded data; length should be a multiple of 32 (was %d)", len(argdata)) } + abispec, err := abi.JSON(strings.NewReader(abidata)) + if err != nil { + return nil, fmt.Errorf("Failed parsing JSON ABI: %v, abidata: %v", err, abidata) + } + method := abispec.MethodById(sigdata) if method == nil { return nil, fmt.Errorf("Supplied ABI spec does not contain method signature in data: 0x%x", sigdata) @@ -79,7 +91,7 @@ func parseCallData(calldata []byte, abidata string) (*decodedCallData, error) { return nil, fmt.Errorf("Failed to decode argument %d (signature %v): %v", n, method.Sig(), err) } else { decodedArg := decodedArgument{ - soltype: argument.Type.String(), + soltype: argument, value: value, } decoded.inputs = append(decoded.inputs, decodedArg) @@ -102,15 +114,50 @@ func parseCallData(calldata []byte, abidata string) (*decodedCallData, error) { if !bytes.Equal(encoded, calldata) { exp := common.Bytes2Hex(encoded) was := common.Bytes2Hex(calldata) - return nil, fmt.Errorf("WARNING: Supplied data is stuffed with extra data. %v \nWant %s\nHave %s", decoded, was, exp) + return nil, fmt.Errorf("WARNING: Supplied data is stuffed with extra data. \nWant %s\nHave %s\nfor method %v", exp, was, method.Sig()) } return &decoded, nil } +// MethodSelectorToAbi converts a method selector into an ABI struct. The returned data is a valid json string +// which can be consumed by the standard abi package. +func MethodSelectorToAbi(selector string) ([]byte, error) { + + re := regexp.MustCompile("^([^\\)]+)\\(([a-z0-9,\\[\\]]*)\\)") + + type fakeArg struct { + Type string `json:"type"` + } + type fakeABI struct { + Name string `json:"name"` + Type string `json:"type"` + Inputs []fakeArg `json:"inputs"` + } + groups := re.FindStringSubmatch(selector) + if len(groups) != 3 { + return nil, fmt.Errorf("Did not match: %v (%v matches)", selector, len(groups)) + } + name := groups[1] + args := groups[2] + arguments := make([]fakeArg, 0) + if len(args) > 0 { + for _, arg := range strings.Split(args, ",") { + arguments = append(arguments, fakeArg{arg}) + } + } + abicheat := fakeABI{ + name, "function", arguments, + } + return json.Marshal([]fakeABI{abicheat}) + +} + type abiDb struct { db map[string]string } +// 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) { raw, err := ioutil.ReadFile(path) if err != nil { @@ -121,9 +168,9 @@ func NewAbiDBFromFile(path string) (*abiDb, error) { return db, nil } -// LookupABI checks the given 4byte-sequence against the known ABI methods. +// LookupMethodSelector checks the given 4byte-sequence against the known ABI methods. // OBS: This method does not validate the match, it's assumed the caller will do so -func (db *abiDb) LookupABI(id []byte) (string, error) { +func (db *abiDb) LookupMethodSelector(id []byte) (string, error) { if len(id) != 4 { return "", fmt.Errorf("Expected 4-byte id, got %d", len(id)) } diff --git a/cmd/signer/abihelper_test.go b/cmd/signer/abihelper_test.go index fa5c56c3e7..cc6158f7df 100644 --- a/cmd/signer/abihelper_test.go +++ b/cmd/signer/abihelper_test.go @@ -17,7 +17,10 @@ package main import ( + "fmt" + "github.com/ethereum/go-ethereum/accounts/abi" "github.com/ethereum/go-ethereum/common" + "strings" "testing" ) @@ -84,3 +87,36 @@ func TestCalldataDecoding(t *testing.T) { } } } + +func TestSelectorUnmarshalling(t *testing.T) { + var ( + db *abiDb + err error + abistring []byte + abistruct abi.ABI + ) + + db, err = NewAbiDBFromFile("4byte.json") + if err != nil { + t.Fatal(err) + } + fmt.Printf("DB size %v\n", db.Size()) + for id, selector := range db.db { + + abistring, err = MethodSelectorToAbi(selector) + if err != nil { + t.Error(err) + return + } + abistruct, err = abi.JSON(strings.NewReader(string(abistring))) + if err != nil { + t.Error(err) + return + } + m := abistruct.MethodById(common.Hex2Bytes(id[2:])) + if m.Sig() != selector { + t.Errorf("Expected equality: %v != %v", m.Sig(), selector) + } + } + +} diff --git a/cmd/signer/api.go b/cmd/signer/api.go index 628b7a1590..f2d5c287f9 100644 --- a/cmd/signer/api.go +++ b/cmd/signer/api.go @@ -56,7 +56,7 @@ type ( SignTxRequest struct { Transaction TransactionArg `json:"transaction"` From common.Address `json:"fromaccount"` - Callinfo fmt.Stringer `json:"call_info"` + Callinfo string `json:"call_info"` Meta Metadata `json:"meta"` } // SignTxResponse result from SignTxRequest @@ -299,7 +299,7 @@ func logDiff(original *SignTxRequest, new *SignTxResponse) bool { // SignTransaction signs the given Transaction and returns it in an RLP encoded form // that can be posted to `eth_sendRawTransaction`. -func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address, args TransactionArg, methodSig *string) (hexutil.Bytes, error) { +func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address, args TransactionArg, methodSelector *string) (hexutil.Bytes, error) { var ( err error @@ -311,19 +311,27 @@ func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address, data := args.Data if len(data) > 3 { // Try to make sense of the data - var abidata string - if methodSig == nil { - abidata, err = api.abidb.LookupABI(data[:4]) + var selector string + if methodSelector == nil { + selector, err = api.abidb.LookupMethodSelector(data[:4]) if err != nil { - req.Callinfo = errorWrapper{"Warning! Could not locate ABI", err} + req.Callinfo = errorWrapper{"Warning! Could not locate ABI", err}.String() } } else { - abidata = *methodSig + selector = *methodSelector } - if abidata != "" { - req.Callinfo, err = parseCallData(data, abidata) + if selector != "" { + abidata, err := MethodSelectorToAbi(selector) if err != nil { - req.Callinfo = errorWrapper{"Warning! Could not validate ABI-data against calldata", err} + req.Callinfo = errorWrapper{"Warning! Could not validate ABI-data against calldata", err}.String() + } else { + var info *decodedCallData + info, err = parseCallData(data, string(abidata)) + if err != nil { + req.Callinfo = errorWrapper{"Warning! Could not validate ABI-data against calldata", err}.String() + } else { + req.Callinfo = info.String() + } } } } @@ -338,8 +346,12 @@ func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address, // Log changes made by the UI to the signing-request logDiff(&req, &result) - acc := accounts.Account{Address: result.From} - wallet, err := api.am.Find(acc) + var ( + acc accounts.Account + wallet accounts.Wallet + ) + acc = accounts.Account{Address: result.From} + wallet, err = api.am.Find(acc) if err != nil { return nil, err } diff --git a/cmd/signer/api_test.go b/cmd/signer/api_test.go index 412bc355d2..d448ebce38 100644 --- a/cmd/signer/api_test.go +++ b/cmd/signer/api_test.go @@ -8,6 +8,8 @@ import ( "github.com/ethereum/go-ethereum/cmd/utils" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/hexutil" + "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/rlp" "io/ioutil" "math/big" "os" @@ -291,10 +293,12 @@ func TestSignTx(t *testing.T) { if err != nil { t.Fatal(err) } - if h == nil || len(h) != 118 { - t.Errorf("Expected 181 byte rlp-data (got %d bytes)", len(h)) + parsedTx := &types.Transaction{} + rlp.Decode(bytes.NewReader(h), parsedTx) + //The tx should NOT be modified by the UI + if parsedTx.Value().Cmp(tx.Value.ToInt()) != 0 { + t.Errorf("Expected value to be unchanged, expected %v got %v", tx.Value, parsedTx.Value()) } - //The tx is NOT modified by the UI control <- "Y" control <- "apassword" @@ -314,6 +318,14 @@ func TestSignTx(t *testing.T) { if err != nil { t.Fatal(err) } + + parsedTx2 := &types.Transaction{} + rlp.Decode(bytes.NewReader(h), parsedTx2) + //The tx should NOT be modified by the UI + if parsedTx2.Value().Cmp(tx.Value.ToInt()) != 0 { + t.Errorf("Expected value to be changed, got %v", parsedTx.Value()) + } + if bytes.Equal(h, h2) { t.Error("Expected tx to be modified by UI") } diff --git a/cmd/signer/cliui.go b/cmd/signer/cliui.go index 881349ee40..28b9dd083c 100644 --- a/cmd/signer/cliui.go +++ b/cmd/signer/cliui.go @@ -97,24 +97,29 @@ func showMetadata(metadata Metadata) { func (ui *CommandlineUI) ApproveTx(request *SignTxRequest) (SignTxResponse, error) { ui.mu.Lock() defer ui.mu.Unlock() - weival := request.Transaction.Value - + weival := request.Transaction.Value.ToInt() + toval := "" + if request.Transaction.To != nil { + toval = request.Transaction.To.Hex() + } fmt.Printf("--------- Transaction request-------------\n") - fmt.Printf("to: %v\n", request.Transaction.To) + fmt.Printf("to: %v\n", toval) fmt.Printf("from: %v\n", request.From.Hex()) fmt.Printf("value: %v wei\n", weival) if len(request.Transaction.Data) > 0 { fmt.Printf("data: %v\n", common.Bytes2Hex(request.Transaction.Data)) } - if request.Callinfo != nil { + if request.Callinfo != "" { fmt.Printf("\nNote: This Transaction contains data. Review abi-decoding info below:") - fmt.Printf("\nCall info:\n\t%v\n", request.Callinfo.String()) + fmt.Printf("\nCall info:\n\t%v\n", request.Callinfo) } fmt.Printf("\n") showMetadata(request.Meta) fmt.Printf("-------------------------------------------\n") - + if !ui.confirm() { + return SignTxResponse{request.Transaction, request.From, false, ""}, nil + } return SignTxResponse{request.Transaction, request.From, true, ui.readPassword()}, nil } @@ -130,6 +135,9 @@ func (ui *CommandlineUI) ApproveSignData(request *SignDataRequest) (SignDataResp fmt.Printf("message hash: %v\n", request.Hash) fmt.Printf("-------------------------------------------\n") showMetadata(request.Meta) + if !ui.confirm() { + return SignDataResponse{false, ""}, nil + } return SignDataResponse{true, ui.readPassword()}, nil } @@ -196,7 +204,10 @@ func (ui *CommandlineUI) ApproveNewAccount(request *NewAccountRequest) (NewAccou fmt.Printf("Approving this operation means that a new Account is created,\n") fmt.Printf("and the address show to the caller\n") showMetadata(request.Meta) - return NewAccountResponse{ui.confirm(), ui.readPassword()}, nil + if !ui.confirm() { + return NewAccountResponse{false, ""}, nil + } + return NewAccountResponse{true, ui.readPassword()}, nil } // ShowError displays error message to user diff --git a/cmd/signer/main.go b/cmd/signer/main.go index 3fcf97e4a9..7b0d11cbef 100644 --- a/cmd/signer/main.go +++ b/cmd/signer/main.go @@ -183,18 +183,18 @@ func testExternalUI(api *SignerAPI) { api.ui.ShowInfo("Testing 'ShowInfo'") api.ui.ShowError("Testing 'ShowError'") - checkErr:= func(method string, err error){ - if err != nil && err != ErrRequestDenied{ + checkErr := func(method string, err error) { + if err != nil && err != ErrRequestDenied { errs = append(errs, fmt.Sprintf("%v: %v", method, err.Error())) } } var err error - _, err = api.SignTransaction(ctx, common.Address{}, TransactionArg{}, nil); + _, err = api.SignTransaction(ctx, common.Address{}, TransactionArg{}, nil) checkErr("SignTransaction", err) _, err = api.Sign(ctx, common.Address{}, common.Hex2Bytes("01020304")) checkErr("Sign", err) - _, err =api.List(ctx) + _, err = api.List(ctx) checkErr("List", err) _, err = api.New(ctx) checkErr("New", err) @@ -210,7 +210,7 @@ func testExternalUI(api *SignerAPI) { for _, e := range errs { log.Error(e) } - }else{ + } else { log.Info("No errors") } diff --git a/cmd/signer/stdioui.go b/cmd/signer/stdioui.go index 876342ff82..28aa7a6e35 100644 --- a/cmd/signer/stdioui.go +++ b/cmd/signer/stdioui.go @@ -111,7 +111,7 @@ func (ui StdIOUI) ApproveNewAccount(request *NewAccountRequest) (NewAccountRespo func (ui StdIOUI) ShowError(message string) { err := ui.dispatch("ShowError", &Message{message}, nil) if err != nil { - log.Info("Error calling 'ShowError'", "exc", err.Error(),"msg", message) + log.Info("Error calling 'ShowError'", "exc", err.Error(), "msg", message) } }