signer,clef: default reject instead of warn + valideate new passwords. fixes #17632 and #17631

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
 

signer/core/api.go

@@ -85,6 +85,7 @@ type SignerAPI struct {
 	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)})
+	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)
+		}
+	}
+	// 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,

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
 

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("-------- New Account request--------------\n\n")
-	fmt.Printf("A request has been made to create a new. \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("Approving this operation means that a new account is created,\n")
-	fmt.Printf("and the address show to the caller\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 message from Clef-----------\n")
-	fmt.Printf("ERROR: %v\n", message)
+	fmt.Println(message)
+	fmt.Printf("-------------------------------------------\n")
 }
 
 // ShowInfo displays info message to user

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"`

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
+}

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)
+		}
+	}
+}