aboutsummaryrefslogtreecommitdiffstats
path: root/signer/core/api.go
diff options
context:
space:
mode:
authorMartin Holst Swende <martin@swende.se>2018-09-25 21:54:58 +0800
committerGitHub <noreply@github.com>2018-09-25 21:54:58 +0800
commitd3441ebb563439bac0837d70591f92e2c6080303 (patch)
treecec46689f8ec4fd4570322e79ad7167c3b792c74 /signer/core/api.go
parenta95a601f35c49be6045de522138f639fbb68c885 (diff)
downloaddexon-d3441ebb563439bac0837d70591f92e2c6080303.tar.gz
dexon-d3441ebb563439bac0837d70591f92e2c6080303.tar.zst
dexon-d3441ebb563439bac0837d70591f92e2c6080303.zip
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
Diffstat (limited to 'signer/core/api.go')
-rw-r--r--signer/core/api.go120
1 files changed, 68 insertions, 52 deletions
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
- }
- 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.
@@ -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)
generated by cgit (git 2.34.1) at 2025-05-29 16:55:12 +0800