From d3441ebb563439bac0837d70591f92e2c6080303 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 25 Sep 2018 15:54:58 +0200 Subject: [PATCH] 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 --- cmd/clef/extapi_changelog.md | 7 ++ cmd/clef/main.go | 10 ++- rpc/http.go | 6 ++ signer/core/api.go | 120 ++++++++++++++----------- signer/core/api_test.go | 82 ++++++++++------- signer/core/auditlog.go | 29 +++--- signer/core/cliui.go | 33 ++++--- signer/core/types.go | 31 +++++++ signer/core/validation.go | 28 +++--- signer/core/validation_test.go | 26 ++++++ signer/storage/aes_gcm_storage.go | 15 ++-- signer/storage/aes_gcm_storage_test.go | 53 ++++++++++- 12 files changed, 307 insertions(+), 133 deletions(-) diff --git a/cmd/clef/extapi_changelog.md b/cmd/clef/extapi_changelog.md index 2014e90ae..6c2c3e819 100644 --- a/cmd/clef/extapi_changelog.md +++ b/cmd/clef/extapi_changelog.md @@ -1,6 +1,13 @@ ### Changelog for external API +#### 4.0.0 +* The external `account_Ecrecover`-method was removed. +* The external `account_Import`-method was removed. + +#### 3.0.0 + +* The external `account_List`-method was changed to not expose `url`, which contained info about the local filesystem. It now returns only a list of addresses. #### 2.0.0 diff --git a/cmd/clef/main.go b/cmd/clef/main.go index f363a86f2..c060285be 100644 --- a/cmd/clef/main.go +++ b/cmd/clef/main.go @@ -48,7 +48,7 @@ import ( ) // ExternalAPIVersion -- see extapi_changelog.md -const ExternalAPIVersion = "2.0.0" +const ExternalAPIVersion = "3.0.0" // InternalAPIVersion -- see intapi_changelog.md const InternalAPIVersion = "2.0.0" @@ -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/rpc/http.go b/rpc/http.go index 9e4f2b261..af79858e2 100644 --- a/rpc/http.go +++ b/rpc/http.go @@ -238,6 +238,12 @@ func (srv *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { ctx = context.WithValue(ctx, "remote", r.RemoteAddr) ctx = context.WithValue(ctx, "scheme", r.Proto) ctx = context.WithValue(ctx, "local", r.Host) + if ua := r.Header.Get("User-Agent"); ua != "" { + ctx = context.WithValue(ctx, "User-Agent", ua) + } + if origin := r.Header.Get("Origin"); origin != "" { + ctx = context.WithValue(ctx, "Origin", origin) + } body := io.LimitReader(r.Body, maxRequestContentLength) codec := NewJSONCodec(&httpReadWriteNopCloser{body, w}) diff --git a/signer/core/api.go b/signer/core/api.go index 4f390c1fd..1da86991e 100644 --- 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) diff --git a/signer/core/api_test.go b/signer/core/api_test.go index 50ad02198..70aa9aa94 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, 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 diff --git a/signer/core/auditlog.go b/signer/core/auditlog.go index d0ba733d2..1f9c90918 100644 --- 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") diff --git a/signer/core/cliui.go b/signer/core/cliui.go index 2f969669c..cc237612e 100644 --- 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: \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 diff --git a/signer/core/types.go b/signer/core/types.go index 2acc0a4f4..128055774 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 288456df8..7c3ec4274 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 } @@ -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 +} diff --git a/signer/core/validation_test.go b/signer/core/validation_test.go index 2b33a8630..155b25e92 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 + {"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) + } + } +} diff --git a/signer/storage/aes_gcm_storage.go b/signer/storage/aes_gcm_storage.go index 399637a44..900831867 100644 --- a/signer/storage/aes_gcm_storage.go +++ b/signer/storage/aes_gcm_storage.go @@ -63,7 +63,7 @@ func (s *AESEncryptedStorage) Put(key, value string) { log.Warn("Failed to read encrypted storage", "err", err, "file", s.filename) return } - ciphertext, iv, err := encrypt(s.key, []byte(value)) + ciphertext, iv, err := encrypt(s.key, []byte(value), []byte(key)) if err != nil { log.Warn("Failed to encrypt entry", "err", err) return @@ -90,7 +90,7 @@ func (s *AESEncryptedStorage) Get(key string) string { log.Warn("Key does not exist", "key", key) return "" } - entry, err := decrypt(s.key, encrypted.Iv, encrypted.CipherText) + entry, err := decrypt(s.key, encrypted.Iv, encrypted.CipherText, []byte(key)) if err != nil { log.Warn("Failed to decrypt key", "key", key) return "" @@ -129,7 +129,10 @@ func (s *AESEncryptedStorage) writeEncryptedStorage(creds map[string]storedCrede return nil } -func encrypt(key []byte, plaintext []byte) ([]byte, []byte, error) { +// encrypt encrypts plaintext with the given key, with additional data +// The 'additionalData' is used to place the (plaintext) KV-store key into the V, +// to prevent the possibility to alter a K, or swap two entries in the KV store with eachother. +func encrypt(key []byte, plaintext []byte, additionalData []byte) ([]byte, []byte, error) { block, err := aes.NewCipher(key) if err != nil { return nil, nil, err @@ -142,11 +145,11 @@ func encrypt(key []byte, plaintext []byte) ([]byte, []byte, error) { if err != nil { return nil, nil, err } - ciphertext := aesgcm.Seal(nil, nonce, plaintext, nil) + ciphertext := aesgcm.Seal(nil, nonce, plaintext, additionalData) return ciphertext, nonce, nil } -func decrypt(key []byte, nonce []byte, ciphertext []byte) ([]byte, error) { +func decrypt(key []byte, nonce []byte, ciphertext []byte, additionalData []byte) ([]byte, error) { block, err := aes.NewCipher(key) if err != nil { return nil, err @@ -155,7 +158,7 @@ func decrypt(key []byte, nonce []byte, ciphertext []byte) ([]byte, error) { if err != nil { return nil, err } - plaintext, err := aesgcm.Open(nil, nonce, ciphertext, nil) + plaintext, err := aesgcm.Open(nil, nonce, ciphertext, additionalData) if err != nil { return nil, err } diff --git a/signer/storage/aes_gcm_storage_test.go b/signer/storage/aes_gcm_storage_test.go index 77804905a..a421a8449 100644 --- a/signer/storage/aes_gcm_storage_test.go +++ b/signer/storage/aes_gcm_storage_test.go @@ -18,6 +18,7 @@ package storage import ( "bytes" + "encoding/json" "fmt" "io/ioutil" "testing" @@ -33,13 +34,13 @@ func TestEncryption(t *testing.T) { key := []byte("AES256Key-32Characters1234567890") plaintext := []byte("exampleplaintext") - c, iv, err := encrypt(key, plaintext) + c, iv, err := encrypt(key, plaintext, nil) if err != nil { t.Fatal(err) } fmt.Printf("Ciphertext %x, nonce %x\n", c, iv) - p, err := decrypt(key, iv, c) + p, err := decrypt(key, iv, c, nil) if err != nil { t.Fatal(err) } @@ -113,3 +114,51 @@ func TestEnd2End(t *testing.T) { t.Errorf("Expected bazonk->foobar, got '%v'", v) } } + +func TestSwappedKeys(t *testing.T) { + // It should not be possible to swap the keys/values, so that + // K1:V1, K2:V2 can be swapped into K1:V2, K2:V1 + log.Root().SetHandler(log.LvlFilterHandler(log.Lvl(3), log.StreamHandler(colorable.NewColorableStderr(), log.TerminalFormat(true)))) + + d, err := ioutil.TempDir("", "eth-encrypted-storage-test") + if err != nil { + t.Fatal(err) + } + + s1 := &AESEncryptedStorage{ + filename: fmt.Sprintf("%v/vault.json", d), + key: []byte("AES256Key-32Characters1234567890"), + } + s1.Put("k1", "v1") + s1.Put("k2", "v2") + // Now make a modified copy + + creds := make(map[string]storedCredential) + raw, err := ioutil.ReadFile(s1.filename) + if err != nil { + t.Fatal(err) + } + if err = json.Unmarshal(raw, &creds); err != nil { + t.Fatal(err) + } + swap := func() { + // Turn it into K1:V2, K2:V2 + v1, v2 := creds["k1"], creds["k2"] + creds["k2"], creds["k1"] = v1, v2 + raw, err = json.Marshal(creds) + if err != nil { + t.Fatal(err) + } + if err = ioutil.WriteFile(s1.filename, raw, 0600); err != nil { + t.Fatal(err) + } + } + swap() + if v := s1.Get("k1"); v != "" { + t.Errorf("swapped value should return empty") + } + swap() + if v := s1.Get("k1"); v != "v1" { + t.Errorf("double-swapped value should work fine") + } +}