From 8e41a5c0c644aac5bf25e9fc28c823a26559fbcd Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 13 Sep 2018 12:50:51 +0200 Subject: [PATCH] signer,clef: default reject instead of warn + valideate new passwords. fixes #17632 and #17631 --- cmd/clef/main.go | 8 +++- signer/core/api.go | 55 +++++++++++++++++-------- signer/core/api_test.go | 74 ++++++++++++++++++++++------------ signer/core/cliui.go | 13 +++--- signer/core/types.go | 31 ++++++++++++++ signer/core/validation.go | 25 +++++++----- signer/core/validation_test.go | 26 ++++++++++++ 7 files changed, 173 insertions(+), 59 deletions(-) diff --git a/cmd/clef/main.go b/cmd/clef/main.go index b337829b4c..c060285be6 100644 --- a/cmd/clef/main.go +++ b/cmd/clef/main.go @@ -70,6 +70,10 @@ var ( Value: 4, Usage: "log level to emit to the screen", } + advancedMode = cli.BoolFlag{ + Name: "advanced", + Usage: "If enabled, issues warnings instead of rejections for suspicious requests. Default off", + } keystoreFlag = cli.StringFlag{ Name: "keystore", Value: filepath.Join(node.DefaultDataDir(), "keystore"), @@ -191,6 +195,7 @@ func init() { ruleFlag, stdiouiFlag, testFlag, + advancedMode, } app.Action = signer app.Commands = []cli.Command{initCommand, attestCommand, addCredentialCommand} @@ -384,7 +389,8 @@ func signer(c *cli.Context) error { c.String(keystoreFlag.Name), c.Bool(utils.NoUSBFlag.Name), ui, db, - c.Bool(utils.LightKDFFlag.Name)) + c.Bool(utils.LightKDFFlag.Name), + c.Bool(advancedMode.Name)) api = apiImpl diff --git a/signer/core/api.go b/signer/core/api.go index 3e3ef3530d..b96ebf348e 100644 --- a/signer/core/api.go +++ b/signer/core/api.go @@ -81,10 +81,11 @@ type SignerUI interface { // SignerAPI defines the actual implementation of ExternalAPI type SignerAPI struct { - chainID *big.Int - am *accounts.Manager - UI SignerUI - validator *Validator + chainID *big.Int + am *accounts.Manager + UI SignerUI + validator *Validator + rejectMode bool } // Metadata about a request @@ -200,7 +201,7 @@ var ErrRequestDenied = errors.New("Request denied") // key that is generated when a new Account is created. // noUSB disables USB support that is required to support hardware devices such as // ledger and trezor. -func NewSignerAPI(chainID int64, ksLocation string, noUSB bool, ui SignerUI, abidb *AbiDb, lightKDF bool) *SignerAPI { +func NewSignerAPI(chainID int64, ksLocation string, noUSB bool, ui SignerUI, abidb *AbiDb, lightKDF bool, advancedMode bool) *SignerAPI { var ( backends []accounts.Backend n, p = keystore.StandardScryptN, keystore.StandardScryptP @@ -228,7 +229,10 @@ 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, NewValidator(abidb)} + if advancedMode { + log.Info("Clef is in advanced mode: will warn instead of reject") + } + return &SignerAPI{big.NewInt(chainID), accounts.NewManager(backends...), ui, NewValidator(abidb), !advancedMode} } // List returns the set of wallet this signer manages. Each wallet can contain @@ -266,15 +270,28 @@ func (api *SignerAPI) New(ctx context.Context) (accounts.Account, error) { if len(be) == 0 { return accounts.Account{}, errors.New("password based accounts not supported") } - resp, err := api.UI.ApproveNewAccount(&NewAccountRequest{MetadataFromContext(ctx)}) - - if err != nil { - return accounts.Account{}, err + var ( + resp NewAccountResponse + err error + ) + // Three retries to get a valid password + for i := 0; i < 3; i++ { + resp, err = api.UI.ApproveNewAccount(&NewAccountRequest{MetadataFromContext(ctx)}) + if err != nil { + return accounts.Account{}, err + } + if !resp.Approved { + return accounts.Account{}, ErrRequestDenied + } + if pwErr := ValidatePasswordFormat(resp.Password); pwErr != nil { + api.UI.ShowError(fmt.Sprintf("Account creation attempt #%d failed due to password requirements: %v", (i + 1), pwErr)) + } else { + // No error + return be[0].(*keystore.KeyStore).NewAccount(resp.Password) + } } - if !resp.Approved { - return accounts.Account{}, ErrRequestDenied - } - return be[0].(*keystore.KeyStore).NewAccount(resp.Password) + // Otherwise fail, with generic error message + return accounts.Account{}, errors.New("account creation failed") } // logDiff logs the difference between the incoming (original) transaction and the one returned from the signer. @@ -306,10 +323,10 @@ func logDiff(original *SignTxRequest, new *SignTxResponse) bool { d0s := "" d1s := "" if d0 != nil { - d0s = common.ToHex(*d0) + d0s = hexutil.Encode(*d0) } if d1 != nil { - d1s = common.ToHex(*d1) + d1s = hexutil.Encode(*d1) } if d1s != d0s { modified = true @@ -333,6 +350,12 @@ func (api *SignerAPI) SignTransaction(ctx context.Context, args SendTxArgs, meth if err != nil { return nil, err } + // If we are in 'rejectMode', then reject rather than show the user warnings + if api.rejectMode { + if err := msgs.getWarnings(); err != nil { + return nil, err + } + } req := SignTxRequest{ Transaction: args, diff --git a/signer/core/api_test.go b/signer/core/api_test.go index 3c2db9ae01..51478e56e7 100644 --- a/signer/core/api_test.go +++ b/signer/core/api_test.go @@ -45,7 +45,7 @@ func (ui *HeadlessUI) OnSignerStartup(info StartupInfo) { } func (ui *HeadlessUI) OnApprovedTx(tx ethapi.SignTransactionResult) { - fmt.Printf("OnApproved called") + fmt.Printf("OnApproved()\n") } func (ui *HeadlessUI) ApproveTx(request *SignTxRequest) (SignTxResponse, error) { @@ -62,26 +62,27 @@ func (ui *HeadlessUI) ApproveTx(request *SignTxRequest) (SignTxResponse, error) return SignTxResponse{request.Transaction, false, ""}, nil } } + func (ui *HeadlessUI) ApproveSignData(request *SignDataRequest) (SignDataResponse, error) { if "Y" == <-ui.controller { return SignDataResponse{true, <-ui.controller}, nil } return SignDataResponse{false, ""}, nil } -func (ui *HeadlessUI) ApproveExport(request *ExportRequest) (ExportResponse, error) { +func (ui *HeadlessUI) ApproveExport(request *ExportRequest) (ExportResponse, error) { return ExportResponse{<-ui.controller == "Y"}, nil } -func (ui *HeadlessUI) ApproveImport(request *ImportRequest) (ImportResponse, error) { +func (ui *HeadlessUI) ApproveImport(request *ImportRequest) (ImportResponse, error) { if "Y" == <-ui.controller { return ImportResponse{true, <-ui.controller, <-ui.controller}, nil } return ImportResponse{false, "", ""}, nil } -func (ui *HeadlessUI) ApproveListing(request *ListRequest) (ListResponse, error) { +func (ui *HeadlessUI) ApproveListing(request *ListRequest) (ListResponse, error) { switch <-ui.controller { case "A": return ListResponse{request.Accounts}, nil @@ -93,20 +94,22 @@ func (ui *HeadlessUI) ApproveListing(request *ListRequest) (ListResponse, error) return ListResponse{nil}, nil } } -func (ui *HeadlessUI) ApproveNewAccount(request *NewAccountRequest) (NewAccountResponse, error) { +func (ui *HeadlessUI) ApproveNewAccount(request *NewAccountRequest) (NewAccountResponse, error) { if "Y" == <-ui.controller { return NewAccountResponse{true, <-ui.controller}, nil } return NewAccountResponse{false, ""}, nil } + func (ui *HeadlessUI) ShowError(message string) { //stdout is used by communication - fmt.Fprint(os.Stderr, message) + fmt.Fprintln(os.Stderr, message) } + func (ui *HeadlessUI) ShowInfo(message string) { //stdout is used by communication - fmt.Fprint(os.Stderr, message) + fmt.Fprintln(os.Stderr, message) } func tmpDirName(t *testing.T) string { @@ -123,7 +126,7 @@ func tmpDirName(t *testing.T) string { func setup(t *testing.T) (*SignerAPI, chan string) { - controller := make(chan string, 10) + controller := make(chan string, 20) db, err := NewAbiDBFromFile("../../cmd/clef/4byte.json") if err != nil { @@ -137,14 +140,14 @@ func setup(t *testing.T) (*SignerAPI, chan string) { true, ui, db, - true) + true, false) ) return api, controller } func createAccount(control chan string, api *SignerAPI, t *testing.T) { control <- "Y" - control <- "apassword" + control <- "a_long_password" _, err := api.New(context.Background()) if err != nil { t.Fatal(err) @@ -152,6 +155,25 @@ func createAccount(control chan string, api *SignerAPI, t *testing.T) { // Some time to allow changes to propagate time.Sleep(250 * time.Millisecond) } + +func failCreateAccountWithPassword(control chan string, api *SignerAPI, password string, t *testing.T) { + + control <- "Y" + control <- password + control <- "Y" + control <- password + control <- "Y" + control <- password + + acc, err := api.New(context.Background()) + if err == nil { + t.Fatal("Should have returned an error") + } + if acc.Address != (common.Address{}) { + t.Fatal("Empty address should be returned") + } +} + func failCreateAccount(control chan string, api *SignerAPI, t *testing.T) { control <- "N" acc, err := api.New(context.Background()) @@ -162,6 +184,7 @@ func failCreateAccount(control chan string, api *SignerAPI, t *testing.T) { t.Fatal("Empty address should be returned") } } + func list(control chan string, api *SignerAPI, t *testing.T) []common.Address { control <- "A" list, err := api.List(context.Background()) @@ -172,7 +195,6 @@ func list(control chan string, api *SignerAPI, t *testing.T) []common.Address { } func TestNewAcc(t *testing.T) { - api, control := setup(t) verifyNum := func(num int) { if list := list(control, api, t); len(list) != num { @@ -188,6 +210,13 @@ func TestNewAcc(t *testing.T) { failCreateAccount(control, api, t) createAccount(control, api, t) failCreateAccount(control, api, t) + + verifyNum(4) + + // Fail to create this, due to bad password + failCreateAccountWithPassword(control, api, "short", t) + failCreateAccountWithPassword(control, api, "longerbutbad\rfoo", t) + verifyNum(4) // Testing listing: @@ -212,7 +241,6 @@ func TestNewAcc(t *testing.T) { } func TestSignData(t *testing.T) { - api, control := setup(t) //Create two accounts createAccount(control, api, t) @@ -233,7 +261,6 @@ func TestSignData(t *testing.T) { if err != keystore.ErrDecrypt { t.Errorf("Expected ErrLocked! %v", err) } - control <- "No way" h, err = api.Sign(context.Background(), a, []byte("EHLO world")) if h != nil { @@ -242,11 +269,9 @@ func TestSignData(t *testing.T) { if err != ErrRequestDenied { t.Errorf("Expected ErrRequestDenied! %v", err) } - control <- "Y" - control <- "apassword" + control <- "a_long_password" h, err = api.Sign(context.Background(), a, []byte("EHLO world")) - if err != nil { t.Fatal(err) } @@ -273,7 +298,6 @@ func mkTestTx(from common.MixedcaseAddress) SendTxArgs { } func TestSignTx(t *testing.T) { - var ( list []common.Address res, res2 *ethapi.SignTransactionResult @@ -301,7 +325,6 @@ func TestSignTx(t *testing.T) { if err != keystore.ErrDecrypt { t.Errorf("Expected ErrLocked! %v", err) } - control <- "No way" res, err = api.SignTransaction(context.Background(), tx, &methodSig) if res != nil { @@ -310,9 +333,8 @@ func TestSignTx(t *testing.T) { if err != ErrRequestDenied { t.Errorf("Expected ErrRequestDenied! %v", err) } - control <- "Y" - control <- "apassword" + control <- "a_long_password" res, err = api.SignTransaction(context.Background(), tx, &methodSig) if err != nil { @@ -320,12 +342,13 @@ func TestSignTx(t *testing.T) { } parsedTx := &types.Transaction{} rlp.Decode(bytes.NewReader(res.Raw), 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()) } control <- "Y" - control <- "apassword" + control <- "a_long_password" res2, err = api.SignTransaction(context.Background(), tx, &methodSig) if err != nil { @@ -337,20 +360,19 @@ func TestSignTx(t *testing.T) { //The tx is modified by the UI control <- "M" - control <- "apassword" + control <- "a_long_password" res2, err = api.SignTransaction(context.Background(), tx, &methodSig) if err != nil { t.Fatal(err) } - parsedTx2 := &types.Transaction{} rlp.Decode(bytes.NewReader(res.Raw), parsedTx2) + //The tx should be modified by the UI if parsedTx2.Value().Cmp(tx.Value.ToInt()) != 0 { t.Errorf("Expected value to be unchanged, got %v", parsedTx.Value()) } - if bytes.Equal(res.Raw, res2.Raw) { t.Error("Expected tx to be modified by UI") } @@ -372,9 +394,9 @@ func TestAsyncronousResponses(t *testing.T){ control <- "W" //wait control <- "Y" // - control <- "apassword" + control <- "a_long_password" control <- "Y" // - control <- "apassword" + control <- "a_long_password" var err error diff --git a/signer/core/cliui.go b/signer/core/cliui.go index 9469049667..cc237612eb 100644 --- a/signer/core/cliui.go +++ b/signer/core/cliui.go @@ -220,10 +220,10 @@ func (ui *CommandlineUI) ApproveNewAccount(request *NewAccountRequest) (NewAccou ui.mu.Lock() defer ui.mu.Unlock() - fmt.Printf("-------- New Account request--------------\n") - fmt.Printf("A request has been made to create a new. \n") - fmt.Printf("Approving this operation means that a new Account is created,\n") - fmt.Printf("and the address show to the caller\n") + fmt.Printf("-------- New Account request--------------\n\n") + fmt.Printf("A request has been made to create a new account. \n") + fmt.Printf("Approving this operation means that a new account is created,\n") + fmt.Printf("and the address is returned to the external caller\n\n") showMetadata(request.Meta) if !ui.confirm() { return NewAccountResponse{false, ""}, nil @@ -233,8 +233,9 @@ func (ui *CommandlineUI) ApproveNewAccount(request *NewAccountRequest) (NewAccou // ShowError displays error message to user func (ui *CommandlineUI) ShowError(message string) { - - fmt.Printf("ERROR: %v\n", message) + fmt.Printf("-------- Error message from Clef-----------\n") + fmt.Println(message) + fmt.Printf("-------------------------------------------\n") } // ShowInfo displays info message to user diff --git a/signer/core/types.go b/signer/core/types.go index 2acc0a4f4a..1280557748 100644 --- a/signer/core/types.go +++ b/signer/core/types.go @@ -18,6 +18,7 @@ package core import ( "encoding/json" + "fmt" "strings" "math/big" @@ -60,6 +61,36 @@ type ValidationMessages struct { Messages []ValidationInfo } +const ( + WARN = "WARNING" + CRIT = "CRITICAL" + INFO = "Info" +) + +func (vs *ValidationMessages) crit(msg string) { + vs.Messages = append(vs.Messages, ValidationInfo{CRIT, msg}) +} +func (vs *ValidationMessages) warn(msg string) { + vs.Messages = append(vs.Messages, ValidationInfo{WARN, msg}) +} +func (vs *ValidationMessages) info(msg string) { + vs.Messages = append(vs.Messages, ValidationInfo{INFO, msg}) +} + +/// getWarnings returns an error with all messages of type WARN of above, or nil if no warnings were present +func (v *ValidationMessages) getWarnings() error { + var messages []string + for _, msg := range v.Messages { + if msg.Typ == WARN || msg.Typ == CRIT { + messages = append(messages, msg.Message) + } + } + if len(messages) > 0 { + return fmt.Errorf("Validation failed: %s", strings.Join(messages, ",")) + } + return nil +} + // SendTxArgs represents the arguments to submit a transaction type SendTxArgs struct { From common.MixedcaseAddress `json:"from"` diff --git a/signer/core/validation.go b/signer/core/validation.go index 288456df87..969b948876 100644 --- a/signer/core/validation.go +++ b/signer/core/validation.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "math/big" + "regexp" "github.com/ethereum/go-ethereum/common" ) @@ -30,16 +31,6 @@ import ( // - 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 } @@ -161,3 +152,17 @@ func (v *Validator) ValidateTransaction(txArgs *SendTxArgs, methodSelector *stri msgs := &ValidationMessages{} return msgs, v.validate(msgs, txArgs, methodSelector) } + +var Printable7BitAscii = regexp.MustCompile("^[A-Za-z0-9!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~]+$") + +// ValidatePasswordFormat returns an error if the password is too short, or consists of characters +// outside the range of the printable 7bit ascii set +func ValidatePasswordFormat(password string) error { + if len(password) < 10 { + return errors.New("password too short (<10 characters)") + } + if !Printable7BitAscii.MatchString(password) { + return errors.New("password contains invalid characters - allowed set (7bit printable ascii) is A-Z, a-z, 0-9, and !\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~") + } + return nil +} diff --git a/signer/core/validation_test.go b/signer/core/validation_test.go index 2b33a8630b..1f5c1de9f0 100644 --- a/signer/core/validation_test.go +++ b/signer/core/validation_test.go @@ -137,3 +137,29 @@ func TestValidator(t *testing.T) { } } } + +func TestPasswordValidation(t *testing.T) { + testcases := []struct { + pw string + shouldFail bool + }{ + {"test", true}, + {"testtest\xbd\xb2\x3d\xbc\x20\xe2\x8c\x98", true}, + {"placeOfInterest⌘", true}, + {"password\nwith\nlinebreak", true}, + {"password\twith\vtabs", true}, + // Ok passwords + {"passwordWhichIsOk", false}, + {"passwordOk!@#$%^&*()", false}, + {"12301203123012301230123012", false}, + } + for _, test := range testcases { + err := ValidatePasswordFormat(test.pw) + if err == nil && test.shouldFail { + t.Errorf("password '%v' should fail validation", test.pw) + } else if err != nil && !test.shouldFail { + + t.Errorf("password '%v' shound not fail validation, but did: %v", test.pw, err) + } + } +}