Skip to content

Commit

Permalink
signer,clef: default reject instead of warn + valideate new passwords. …
Browse files Browse the repository at this point in the history 
…fixes ethereum#17632 and ethereum#17631
  • Loading branch information
@holiman
holiman committed Sep 13, 2018
1 parent cf3bf17 commit 193f704
Show file tree
Hide file tree
Showing 7 changed files with 173 additions and 59 deletions.
8 changes: 7 additions & 1 deletion cmd/clef/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -191,6 +195,7 @@ func init() {
ruleFlag,
stdiouiFlag,
testFlag,
advancedMode,
}
app.Action = signer
app.Commands = []cli.Command{initCommand, attestCommand, addCredentialCommand}
Expand Down Expand Up @@ -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

Expand Down
55 changes: 39 additions & 16 deletions signer/core/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
if !resp.Approved {
return accounts.Account{}, ErrRequestDenied
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)
}
}
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.
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down
74 changes: 48 additions & 26 deletions signer/core/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -137,21 +140,40 @@ 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)
}
// 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())
Expand All @@ -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())
Expand All @@ -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 {
Expand All @@ -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:
Expand All @@ -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)
Expand All @@ -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 {
Expand All @@ -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)
}
Expand All @@ -273,7 +298,6 @@ func mkTestTx(from common.MixedcaseAddress) SendTxArgs {
}

func TestSignTx(t *testing.T) {

var (
list []common.Address
res, res2 *ethapi.SignTransactionResult
Expand Down Expand Up @@ -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 {
Expand All @@ -310,22 +333,22 @@ 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 {
t.Fatal(err)
}
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 {
Expand All @@ -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")
}
Expand All @@ -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
Expand Down
Loading

0 comments on commit 193f704

Please sign in to comment.