cmd/clef, signer: security fixes (#17554)

* signer: remove local path disclosure from extapi * signer: show more data in cli ui * rpc: make http server forward UA and Origin via Context * signer, clef/core: ui changes + display UA and Origin * signer: cliui - indicate less trust in remote headers, see https://github.com/ethereum/go-ethereum/issues/17637 * signer: prevent possibility swap KV-entries in aes_gcm storage, fixes #17635 * signer: remove ecrecover from external API * signer,clef: default reject instead of warn + valideate new passwords. fixes #17632 and #17631 * signer: check calldata length even if no ABI signature is present * signer: fix failing testcase * clef: remove account import from external api * signer: allow space in passwords, improve error messsage * signer/storage: fix typos
2018-09-25 15:54:58 +02:00
parent a95a601f35
commit d3441ebb56
12 changed files with 307 additions and 133 deletions
--- a/signer/core/api.go
+++ b/signer/core/api.go
@@ -39,19 +39,19 @@ import (
 // ExternalAPI defines the external API through which signing requests are made.
 type ExternalAPI interface {
 	// List available accounts
-	List(ctx context.Context) (Accounts, error)
+	List(ctx context.Context) ([]common.Address, error)
 	// New request to create a new account
 	New(ctx context.Context) (accounts.Account, error)
 	// SignTransaction request to sign the specified transaction
 	SignTransaction(ctx context.Context, args SendTxArgs, methodSelector *string) (*ethapi.SignTransactionResult, error)
 	// Sign - request to sign the given data (plus prefix)
 	Sign(ctx context.Context, addr common.MixedcaseAddress, data hexutil.Bytes) (hexutil.Bytes, error)
-	// EcRecover - request to perform ecrecover
-	EcRecover(ctx context.Context, data, sig hexutil.Bytes) (common.Address, error)
 	// Export - request to export an account
 	Export(ctx context.Context, addr common.Address) (json.RawMessage, error)
 	// Import - request to import an account
-	Import(ctx context.Context, keyJSON json.RawMessage) (Account, error)
+	// Should be moved to Internal API, in next phase when we have
+	// bi-directional communication
+	//Import(ctx context.Context, keyJSON json.RawMessage) (Account, error)
 }

 // SignerUI specifies what method a UI needs to implement to be able to be used as a UI for the signer
@@ -83,22 +83,25 @@ 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
 type Metadata struct {
-	Remote string `json:"remote"`
-	Local  string `json:"local"`
-	Scheme string `json:"scheme"`
+	Remote    string `json:"remote"`
+	Local     string `json:"local"`
+	Scheme    string `json:"scheme"`
+	UserAgent string `json:"User-Agent"`
+	Origin    string `json:"Origin"`
 }

 // MetadataFromContext extracts Metadata from a given context.Context
 func MetadataFromContext(ctx context.Context) Metadata {
-	m := Metadata{"NA", "NA", "NA"} // batman
+	m := Metadata{"NA", "NA", "NA", "", ""} // batman

 	if v := ctx.Value("remote"); v != nil {
 		m.Remote = v.(string)
@@ -109,6 +112,12 @@ func MetadataFromContext(ctx context.Context) Metadata {
 	if v := ctx.Value("local"); v != nil {
 		m.Local = v.(string)
 	}
+	if v := ctx.Value("Origin"); v != nil {
+		m.Origin = v.(string)
+	}
+	if v := ctx.Value("User-Agent"); v != nil {
+		m.UserAgent = v.(string)
+	}
 	return m
 }

@@ -194,7 +203,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
@@ -222,12 +231,15 @@ 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
 // multiple accounts.
-func (api *SignerAPI) List(ctx context.Context) (Accounts, error) {
+func (api *SignerAPI) List(ctx context.Context) ([]common.Address, error) {
 	var accs []Account
 	for _, wallet := range api.am.Wallets() {
 		for _, acc := range wallet.Accounts() {
@@ -243,7 +255,13 @@ func (api *SignerAPI) List(ctx context.Context) (Accounts, error) {
 		return nil, ErrRequestDenied

 	}
-	return result.Accounts, nil
+
+	addresses := make([]common.Address, 0)
+	for _, acc := range result.Accounts {
+		addresses = append(addresses, acc.Address)
+	}
+
+	return addresses, nil
 }

 // New creates a new password protected Account. The private key is protected with
@@ -254,15 +272,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.
@@ -294,10 +325,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
@@ -321,6 +352,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,
@@ -404,32 +441,6 @@ func (api *SignerAPI) Sign(ctx context.Context, addr common.MixedcaseAddress, da
 	return signature, nil
 }

-// EcRecover returns the address for the Account that was used to create the signature.
-// Note, this function is compatible with eth_sign and personal_sign. As such it recovers
-// the address of:
-// hash = keccak256("\x19Ethereum Signed Message:\n"${message length}${message})
-// addr = ecrecover(hash, signature)
-//
-// Note, the signature must conform to the secp256k1 curve R, S and V values, where
-// the V value must be 27 or 28 for legacy reasons.
-//
-// https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_ecRecover
-func (api *SignerAPI) EcRecover(ctx context.Context, data, sig hexutil.Bytes) (common.Address, error) {
-	if len(sig) != 65 {
-		return common.Address{}, fmt.Errorf("signature must be 65 bytes long")
-	}
-	if sig[64] != 27 && sig[64] != 28 {
-		return common.Address{}, fmt.Errorf("invalid Ethereum signature (V is not 27 or 28)")
-	}
-	sig[64] -= 27 // Transform yellow paper V from 27/28 to 0/1
-	hash, _ := SignHash(data)
-	rpk, err := crypto.SigToPub(hash, sig)
-	if err != nil {
-		return common.Address{}, err
-	}
-	return crypto.PubkeyToAddress(*rpk), nil
-}
-
 // SignHash is a helper function that calculates a hash for the given message that can be
 // safely used to calculate a signature from.
 //
@@ -466,6 +477,11 @@ func (api *SignerAPI) Export(ctx context.Context, addr common.Address) (json.Raw
 // Import tries to import the given keyJSON in the local keystore. The keyJSON data is expected to be
 // in web3 keystore format. It will decrypt the keyJSON with the given passphrase and on successful
 // decryption it will encrypt the key with the given newPassphrase and store it in the keystore.
+// OBS! This method is removed from the public API. It should not be exposed on the external API
+// for a couple of reasons:
+// 1. Even though it is encrypted, it should still be seen as sensitive data
+// 2. It can be used to DoS clef, by using malicious data with e.g. extreme large
+// values for the kdfparams.
 func (api *SignerAPI) Import(ctx context.Context, keyJSON json.RawMessage) (Account, error) {
 	be := api.am.Backends(keystore.KeyStoreType)

--- 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, true)
 	)
 	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,7 +184,8 @@ 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) []Account {
+
+func list(control chan string, api *SignerAPI, t *testing.T) []common.Address {
 	control <- "A"
 	list, err := api.List(context.Background())
 	if err != nil {
@@ -172,7 +195,6 @@ func list(control chan string, api *SignerAPI, t *testing.T) []Account {
 }

 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)
@@ -222,7 +250,7 @@ func TestSignData(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	a := common.NewMixedcaseAddress(list[0].Address)
+	a := common.NewMixedcaseAddress(list[0])

 	control <- "Y"
 	control <- "wrongpassword"
@@ -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,9 +298,8 @@ func mkTestTx(from common.MixedcaseAddress) SendTxArgs {
 }

 func TestSignTx(t *testing.T) {
-
 	var (
-		list      Accounts
+		list      []common.Address
 		res, res2 *ethapi.SignTransactionResult
 		err       error
 	)
@@ -287,7 +311,7 @@ func TestSignTx(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	a := common.NewMixedcaseAddress(list[0].Address)
+	a := common.NewMixedcaseAddress(list[0])

 	methodSig := "test(uint)"
 	tx := mkTestTx(a)
@@ -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

--- a/signer/core/auditlog.go
+++ b/signer/core/auditlog.go
@@ -33,11 +33,10 @@ type AuditLogger struct {
 	api ExternalAPI
 }

-func (l *AuditLogger) List(ctx context.Context) (Accounts, error) {
+func (l *AuditLogger) List(ctx context.Context) ([]common.Address, error) {
 	l.log.Info("List", "type", "request", "metadata", MetadataFromContext(ctx).String())
 	res, e := l.api.List(ctx)
-
-	l.log.Info("List", "type", "response", "data", res.String())
+	l.log.Info("List", "type", "response", "data", res)

 	return res, e
 }
@@ -72,14 +71,6 @@ func (l *AuditLogger) Sign(ctx context.Context, addr common.MixedcaseAddress, da
 	return b, e
 }

-func (l *AuditLogger) EcRecover(ctx context.Context, data, sig hexutil.Bytes) (common.Address, error) {
-	l.log.Info("EcRecover", "type", "request", "metadata", MetadataFromContext(ctx).String(),
-		"data", common.Bytes2Hex(data))
-	a, e := l.api.EcRecover(ctx, data, sig)
-	l.log.Info("EcRecover", "type", "response", "addr", a.String(), "error", e)
-	return a, e
-}
-
 func (l *AuditLogger) Export(ctx context.Context, addr common.Address) (json.RawMessage, error) {
 	l.log.Info("Export", "type", "request", "metadata", MetadataFromContext(ctx).String(),
 		"addr", addr.Hex())
@@ -89,14 +80,14 @@ func (l *AuditLogger) Export(ctx context.Context, addr common.Address) (json.Raw
 	return j, e
 }

-func (l *AuditLogger) Import(ctx context.Context, keyJSON json.RawMessage) (Account, error) {
-	// Don't actually log the json contents
-	l.log.Info("Import", "type", "request", "metadata", MetadataFromContext(ctx).String(),
-		"keyJSON size", len(keyJSON))
-	a, e := l.api.Import(ctx, keyJSON)
-	l.log.Info("Import", "type", "response", "addr", a.String(), "error", e)
-	return a, e
-}
+//func (l *AuditLogger) Import(ctx context.Context, keyJSON json.RawMessage) (Account, error) {
+//	// Don't actually log the json contents
+//	l.log.Info("Import", "type", "request", "metadata", MetadataFromContext(ctx).String(),
+//		"keyJSON size", len(keyJSON))
+//	a, e := l.api.Import(ctx, keyJSON)
+//	l.log.Info("Import", "type", "response", "addr", a.String(), "error", e)
+//	return a, e
+//}

 func NewAuditLogger(path string, api ExternalAPI) (*AuditLogger, error) {
 	l := log.New("api", "signer")
--- a/signer/core/cliui.go
+++ b/signer/core/cliui.go
@@ -25,7 +25,7 @@ import (
 	"sync"

 	"github.com/davecgh/go-spew/spew"
-	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/internal/ethapi"
 	"github.com/ethereum/go-ethereum/log"
 	"golang.org/x/crypto/ssh/terminal"
@@ -95,6 +95,8 @@ func (ui *CommandlineUI) confirm() bool {

 func showMetadata(metadata Metadata) {
 	fmt.Printf("Request context:\n\t%v -> %v -> %v\n", metadata.Remote, metadata.Scheme, metadata.Local)
+	fmt.Printf("\nAdditional HTTP header data, provided by the external caller:\n")
+	fmt.Printf("\tUser-Agent: %v\n\tOrigin: %v\n", metadata.UserAgent, metadata.Origin)
 }

 // ApproveTx prompt the user for confirmation to request to sign Transaction
@@ -111,18 +113,22 @@ func (ui *CommandlineUI) ApproveTx(request *SignTxRequest) (SignTxResponse, erro
 	} else {
 		fmt.Printf("to:    <contact creation>\n")
 	}
-	fmt.Printf("from:  %v\n", request.Transaction.From.String())
-	fmt.Printf("value: %v wei\n", weival)
+	fmt.Printf("from:     %v\n", request.Transaction.From.String())
+	fmt.Printf("value:    %v wei\n", weival)
+	fmt.Printf("gas:      %v (%v)\n", request.Transaction.Gas, uint64(request.Transaction.Gas))
+	fmt.Printf("gasprice: %v wei\n", request.Transaction.GasPrice.ToInt())
+	fmt.Printf("nonce:    %v (%v)\n", request.Transaction.Nonce, uint64(request.Transaction.Nonce))
 	if request.Transaction.Data != nil {
 		d := *request.Transaction.Data
 		if len(d) > 0 {
-			fmt.Printf("data:  %v\n", common.Bytes2Hex(d))
+
+			fmt.Printf("data:     %v\n", hexutil.Encode(d))
 		}
 	}
 	if request.Callinfo != nil {
 		fmt.Printf("\nTransaction validation:\n")
 		for _, m := range request.Callinfo {
-			fmt.Printf("  * %s : %s", m.Typ, m.Message)
+			fmt.Printf("  * %s : %s\n", m.Typ, m.Message)
 		}
 		fmt.Println()

@@ -196,7 +202,9 @@ func (ui *CommandlineUI) ApproveListing(request *ListRequest) (ListResponse, err
 	fmt.Printf("A request has been made to list all accounts. \n")
 	fmt.Printf("You can select which accounts the caller can see\n")
 	for _, account := range request.Accounts {
-		fmt.Printf("\t[x] %v\n", account.Address.Hex())
+		fmt.Printf("  [x] %v\n", account.Address.Hex())
+		fmt.Printf("    URL: %v\n", account.URL)
+		fmt.Printf("    Type: %v\n", account.Typ)
 	}
 	fmt.Printf("-------------------------------------------\n")
 	showMetadata(request.Meta)
@@ -212,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
@@ -225,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
--- 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"`
--- 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
 }
@@ -72,6 +63,9 @@ func (v *Validator) validateCallData(msgs *ValidationMessages, data []byte, meth
 		msgs.warn("Tx contains data which is not valid ABI")
 		return
 	}
+	if arglen := len(data) - 4; arglen%32 != 0 {
+		msgs.warn(fmt.Sprintf("Not ABI-encoded data; length should be a multiple of 32 (was %d)", arglen))
+	}
 	var (
 		info *decodedCallData
 		err  error
@@ -161,3 +155,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 - only 7bit printable ascii allowed")
+	}
+	return nil
+}
--- 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
+		{"password WhichIsOk", 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)
+		}
+	}
+}