From 94583497b0470520e27da70df1d64a7b62b8a2f5 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 15 Feb 2018 14:38:29 +0100 Subject: [PATCH] signer/rules: hide json-conversion from users, ensure context is cleaned --- cmd/signer/rules.md | 10 +- cmd/signer/rules/rules.go | 63 ++++++++--- cmd/signer/rules/rules_test.go | 193 ++++++++++++++++++++++++++++++--- 3 files changed, 230 insertions(+), 36 deletions(-) diff --git a/cmd/signer/rules.md b/cmd/signer/rules.md index 3062fc0403..c0e3196f89 100644 --- a/cmd/signer/rules.md +++ b/cmd/signer/rules.md @@ -151,8 +151,7 @@ This is now implemented (with ephemeral non-encrypted storage for now, so not ye return sum.plus(value).lt(limit) } - function ApproveTx(jsonstr){ - var r = JSON.parse(jsonstr) + function ApproveTx(r){ if (isLimitOk(r.transaction)){ return "Approve" } @@ -171,9 +170,7 @@ This is now implemented (with ephemeral non-encrypted storage for now, so not ye * * TLDR; Use this method to keep track of signed transactions, instead of using the data in ApproveTx. */ - function OnApprovedTx(response_str){ - console.log("OnApprovedTx > called with data\n\t "+response_str) - var resp = JSON.parse(response_str) + function OnApprovedTx(resp){ var value = big(resp.tx.value) var txs = [] // Load stored transactions @@ -192,8 +189,7 @@ This is now implemented (with ephemeral non-encrypted storage for now, so not ye ```javascript - function ApproveTx(jsonstr){ - r = JSON.parse(jsonstr) + function ApproveTx(r){ if(r.transaction.from.toLowerCase()=="0x0000000000000000000000000000000000001337"){ return "Approve"} if(r.transaction.from.toLowerCase()=="0x000000000000000000000000000000000000dead"){ return "Reject"} // Otherwise goes to manual processing diff --git a/cmd/signer/rules/rules.go b/cmd/signer/rules/rules.go index 8eb1a0fbb7..2cac0b721f 100644 --- a/cmd/signer/rules/rules.go +++ b/cmd/signer/rules/rules.go @@ -47,46 +47,78 @@ func consoleOutput(call otto.FunctionCall) otto.Value { // rulesetUi provides an implementation of SignerUI that evaluates a javascript // file for each defined UI-method type rulesetUi struct { - vm *otto.Otto // The JS vm + // vm *otto.Otto // The JS vm next core.SignerUI // The next handler, for manual processing storage storage.Storage + jsRules string // The rules to use } func NewRuleEvaluator(next core.SignerUI) (*rulesetUi, error) { c := &rulesetUi{ - vm: otto.New(), + // vm: otto.New(), next: next, storage: storage.NewEphemeralStorage(), + jsRules: "", } - consoleObj, _ := c.vm.Get("console") - consoleObj.Object().Set("log", consoleOutput) - consoleObj.Object().Set("error", consoleOutput) - - c.vm.Set("storage", c.storage) return c, nil } func (r *rulesetUi) Init(javascriptRules string) error { - script, err := r.vm.Compile("bignumber.js", BigNumber_JS) + r.jsRules = javascriptRules + return nil +} +func (r *rulesetUi) execute(jsfunc string, jsarg interface{}) (otto.Value, error) { + + // Instantiate a fresh vm engine every time + vm := otto.New() + // Set the native callbacks + consoleObj, _ := vm.Get("console") + consoleObj.Object().Set("log", consoleOutput) + consoleObj.Object().Set("error", consoleOutput) + vm.Set("storage", r.storage) + + // Load bootstrap libraries + script, err := vm.Compile("bignumber.js", BigNumber_JS) if err != nil { log.Warn("Failed loading libraries", "err", err) - return err + return otto.UndefinedValue(), err } - r.vm.Run(script) + vm.Run(script) - _, err = r.vm.Run(javascriptRules) + // Run the actual rule implementation + _, err = vm.Run(r.jsRules) if err != nil { log.Warn("Execution failed", "err", err) + return otto.UndefinedValue(), err } - return err + + // And the actual call + // All calls are objects with the parameters being keys in that object. + // To provide additional insulation between js and go, we serialize it into JSON on the Go-side, + // and deserialize it on the JS side. + //argdata := "" + + jsonbytes, err := json.Marshal(jsarg) + if err != nil { + log.Warn("failed marshalling data", "data", jsarg) + return otto.UndefinedValue(), err + } + // Now, we call foobar(JSON.parse()). + var call string + if(len(jsonbytes) > 0){ + call = fmt.Sprintf("%v(JSON.parse(%v))", jsfunc, string(jsonbytes)) + }else{ + call = fmt.Sprintf("%v()", jsfunc) + } + return vm.Run(call) } func (r *rulesetUi) checkApproval(jsfunc string, jsarg []byte, err error) (bool, error) { if err != nil { return false, err } - v, err := r.vm.Call(jsfunc, nil, string(jsarg)) + v, err := r.execute(jsfunc, string(jsarg)) if err != nil { log.Info("error occurred during execution", "error", err) return false, err @@ -186,9 +218,8 @@ func (r *rulesetUi) OnApprovedTx(tx ethapi.SignTransactionResult) { log.Warn("failed marshalling transaction", "tx", tx) return } - _, err = r.vm.Call("OnApprovedTx", nil, string(jsonTx)) + _, err = r.execute("OnApprovedTx", string(jsonTx)) if err != nil { - fmt.Printf("Error in onapprove %v", err) - log.Warn("error occurred during execution", "error", err) + log.Info("error occurred during execution", "error", err) } } diff --git a/cmd/signer/rules/rules_test.go b/cmd/signer/rules/rules_test.go index ae303e2187..a789923644 100644 --- a/cmd/signer/rules/rules_test.go +++ b/cmd/signer/rules/rules_test.go @@ -3,13 +3,14 @@ package rules import ( "fmt" "github.com/ethereum/go-ethereum/accounts" + "github.com/ethereum/go-ethereum/cmd/signer/core" "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/internal/ethapi" "math/big" "testing" - "github.com/ethereum/go-ethereum/cmd/signer/core" + "strings" ) const JS = ` @@ -53,6 +54,7 @@ func hexAddr(a string) common.Address { return common.BytesToAddress(common.Hex2 func mixAddr(a string) (*common.MixedcaseAddress, error) { return common.NewMixedcaseAddressFromString(a) } + type alwaysDenyUi struct{} func (alwaysDenyUi) ApproveTx(request *core.SignTxRequest) (core.SignTxResponse, error) { @@ -135,9 +137,7 @@ func TestListRequest(t *testing.T) { func TestSignTxRequest(t *testing.T) { js := ` - function ApproveTx(jsonstr){ - console.log(jsonstr) - r = JSON.parse(jsonstr) + function ApproveTx(r){ console.log("transaction.from", r.transaction.from); console.log("transaction.to", r.transaction.to); console.log("transaction.value", r.transaction.value); @@ -178,6 +178,85 @@ func TestSignTxRequest(t *testing.T) { } } +type dummyUi struct { + calls []string +} + +func (d *dummyUi) ApproveTx(request *core.SignTxRequest) (core.SignTxResponse, error) { + d.calls = append(d.calls, "ApproveTx") + return core.SignTxResponse{}, core.ErrRequestDenied +} + +func (d *dummyUi) ApproveSignData(request *core.SignDataRequest) (core.SignDataResponse, error) { + d.calls = append(d.calls, "ApproveSignData") + return core.SignDataResponse{}, core.ErrRequestDenied +} + +func (d *dummyUi) ApproveExport(request *core.ExportRequest) (core.ExportResponse, error) { + d.calls = append(d.calls, "ApproveExport") + return core.ExportResponse{}, core.ErrRequestDenied +} + +func (d *dummyUi) ApproveImport(request *core.ImportRequest) (core.ImportResponse, error) { + d.calls = append(d.calls, "ApproveImport") + return core.ImportResponse{}, core.ErrRequestDenied +} + +func (d *dummyUi) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { + d.calls = append(d.calls, "ApproveListing") + return core.ListResponse{}, core.ErrRequestDenied +} + +func (d *dummyUi) ApproveNewAccount(request *core.NewAccountRequest) (core.NewAccountResponse, error) { + d.calls = append(d.calls, "ApproveNewAccount") + return core.NewAccountResponse{}, core.ErrRequestDenied +} + +func (d *dummyUi) ShowError(message string) { + d.calls = append(d.calls, "ShowError") +} + +func (d *dummyUi) ShowInfo(message string) { + d.calls = append(d.calls, "ShowInfo") +} + +func (d *dummyUi) OnApprovedTx(tx ethapi.SignTransactionResult) { + d.calls = append(d.calls, "OnApprovedTx") +} + +//TestForwarding tests that the rule-engine correctly dispatches requests to the next caller +func TestForwarding(t *testing.T) { + + js := "" + ui := &dummyUi{make([]string, 0)} + r, err := NewRuleEvaluator(ui) + if err != nil { + t.Fatalf("Failed to create js engine: %v", err) + } + if err = r.Init(js); err != nil { + t.Fatalf("Failed to load bootstrap js: %v", err) + } + r.ApproveSignData(nil) + r.ApproveTx(nil) + r.ApproveImport(nil) + r.ApproveNewAccount(nil) + r.ApproveListing(nil) + r.ApproveExport(nil) + r.ShowError("test") + r.ShowInfo("test") + + //This one is not forwarded + r.OnApprovedTx(ethapi.SignTransactionResult{}) + + exp_calls := 8 + if len(ui.calls) != exp_calls { + + t.Errorf("Expected %d forwarded calls, got %d: %s", exp_calls, len(ui.calls), strings.Join(ui.calls,",")) + + } + +} + func TestMissingFunc(t *testing.T) { r, err := initRuleEngine(JS) if err != nil { @@ -185,17 +264,17 @@ func TestMissingFunc(t *testing.T) { return } - _, err = r.vm.Call("MissingMethod", nil, "test") + _, err = r.execute("MissingMethod", "test") if err == nil { t.Error("Expected error") } - approved, err := r.checkApproval("MissingMethod", nil, nil); + approved, err := r.checkApproval("MissingMethod", nil, nil) if err == nil { t.Errorf("Expected missing method to yield error'") } - if approved{ + if approved { t.Errorf("Expected missing method to cause non-approval") } fmt.Printf("Err %v", err) @@ -236,7 +315,7 @@ func TestStorage(t *testing.T) { return } - v, err := r.vm.Call("testStorage", nil, nil) + v, err := r.execute("testStorage", nil) if err != nil { t.Errorf("Unexpected error %v", err) @@ -293,8 +372,9 @@ const ExampleTxWindow = ` return sum.plus(value).lt(limit) } - function ApproveTx(jsonstr){ - var r = JSON.parse(jsonstr) + function ApproveTx(r){ + console.log(r) + console.log(typeof(r)) if (isLimitOk(r.transaction)){ return "Approve" } @@ -313,9 +393,7 @@ const ExampleTxWindow = ` * * TLDR; Use this method to keep track of signed transactions, instead of using the data in ApproveTx. */ - function OnApprovedTx(response_str){ - console.log("OnApprovedTx > called with data\n\t "+response_str) - var resp = JSON.parse(response_str) + function OnApprovedTx(resp){ var value = big(resp.tx.value) var txs = [] // Load stored transactions @@ -351,6 +429,12 @@ func dummyTx(value hexutil.Big) *core.SignTxRequest { Meta: core.Metadata{"remoteip", "localip", "inproc"}, } } +func dummyTxWithV(value uint64) *core.SignTxRequest { + + v := big.NewInt(0).SetUint64(value) + h := hexutil.Big(*v) + return dummyTx(h) +} func dummySigned(value *big.Int) *types.Transaction { to := common.HexToAddress("000000000000000000000000000000000000dead") gas := big.NewInt(21000) @@ -398,3 +482,86 @@ func TestLimitWindow(t *testing.T) { } } + +// dontCallMe is used as a next-handler that does not want to be called - it invokes test failure +type dontCallMe struct{ + t *testing.T +} + +func (d *dontCallMe) ApproveTx(request *core.SignTxRequest) (core.SignTxResponse, error) { + d.t.Fatalf("Did not expect next-handler to be called") + return core.SignTxResponse{}, core.ErrRequestDenied +} + +func (d *dontCallMe) ApproveSignData(request *core.SignDataRequest) (core.SignDataResponse, error) { + d.t.Fatalf("Did not expect next-handler to be called") + return core.SignDataResponse{}, core.ErrRequestDenied +} + +func (d *dontCallMe) ApproveExport(request *core.ExportRequest) (core.ExportResponse, error) { + d.t.Fatalf("Did not expect next-handler to be called") + return core.ExportResponse{}, core.ErrRequestDenied +} + +func (d *dontCallMe) ApproveImport(request *core.ImportRequest) (core.ImportResponse, error) { + d.t.Fatalf("Did not expect next-handler to be called") + return core.ImportResponse{}, core.ErrRequestDenied +} + +func (d *dontCallMe) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { + d.t.Fatalf("Did not expect next-handler to be called") + return core.ListResponse{}, core.ErrRequestDenied +} + +func (d *dontCallMe) ApproveNewAccount(request *core.NewAccountRequest) (core.NewAccountResponse, error) { + d.t.Fatalf("Did not expect next-handler to be called") + return core.NewAccountResponse{}, core.ErrRequestDenied +} + +func (d *dontCallMe) ShowError(message string) { + d.t.Fatalf("Did not expect next-handler to be called") +} + +func (d *dontCallMe) ShowInfo(message string) { + d.t.Fatalf("Did not expect next-handler to be called") +} + +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 +func TestContextIsCleared(t *testing.T) { + + js := ` + function ApproveTx(){ + if (typeof foobar == 'undefined') { + foobar = "Approve" + } + console.log(foobar) + if (foobar == "Approve"){ + foobar = "Reject" + }else{ + foobar = "Approve" + } + return foobar + } + ` + ui := &dontCallMe{t} + r, err := NewRuleEvaluator(ui) + if err != nil { + t.Fatalf("Failed to create js engine: %v", err) + } + if err = r.Init(js); err != nil { + t.Fatalf("Failed to load bootstrap js: %v", err) + } + tx := dummyTxWithV(0) + r1, err := r.ApproveTx(tx) + r2, err := r.ApproveTx(tx) + if r1.Approved != r2.Approved{ + t.Errorf("Expected execution context to be cleared between executions") + } +}