Feature Discussion - Let's Upgrade Security On the Wallet

[sshelton76]

This is not a bug per se, but a request for discussion and if approved as a feature request and if assigned to me, I am volunteering to implement it and submit a PR sometime in the cycle between 1.0 and 2.0.

Here is the deal...
https://web3js.readthedocs.io/en/1.0/web3-eth-accounts.html#wallet-save

When a wallet is saved, it presently encrypts it into local storage and you need a password to load the wallet. The problem is that once loaded, the privatekey is available and this leaves the privatekey vulnerable to XSS.

A better solution would be to have 2 passwords, an app level password which is used for marshalling the wallet into and out of storage, and a user supplied password required for signing.
The privatekey should remain encrypted until needed.

There are only a handful of times that anyone actually needs the private key. Basically it's only used for signing purposes. So how about we don't store the private key at all?

Instead, we could store half the key, by XORing it against a the bytes derived from a user supplied password "p" stretched into it's own key using a password based key derivation function pbkdf.

We would upgrade the transaction signing logic to require a Uint8Array[32] which would be the bytes from, pbkdf(p). Each time a signing needs to occur, then we supply the user's half of the key, which ideally we should be prompting the user for. Then we just XOR the user supplied part, pbkdf(p), with the stored part to recover the private key, long enough to sign with before disposing of it properly again.

The reason for using typed arrays here is so that they can be zero'd out after each use.

Now keep in mind, there already exist PBKDF and PBKDF2 which are both standards, but are not specifically what I'm talking about. Instead we could either roll our own using argon2
, or just re-use whatever they are doing in ethereumjs-wallet which appears to be 262144 rounds of HMAC-SHA256 according to their README.

For pbkdf2:

c - Number of iterations. Defaults to 262144.
prf - The only supported (and default) value is hmac-sha256. So no point changing it.

That particular function is slow, but could easily be accelerated in the browser using crypto.subtle, the built in crypto library...

There are other things we could do to limit pain points. However I think it goes without saying that our current model has a serious flaw and the only reason it isn't being taken advantage of in the wild, is that it isn't widely known. DApps with their own wallets are presently vulnerable to key leakage if they are storing a wallet in the browser.

If you'd like to see an example of my proposed fix in action, I am taking this direction on one of the projects I'm working on and will be glad to post a link as soon as it's live. This way you can see what the code would look like.

Thank you for taking the time to read this!

[nivida]

Great idea! Feel free to work on it!
If possible we could use the kv-storage module where you can create "isolated" storage areas:

• https://github.com/WICG/kv-storage
• https://developers.google.com/web/updates/2019/03/kv-storage#what_if_a_browser_doesnt_support_a_built-in_module

[nivida]

Because of security reasons should we add these properties to the proxy handler of the Wallet class:

const throwError = () => {
  throw new Error('Not allowed action!');
};

const handlerObj = {
  set: throwError,
  defineProperty: throwError,
  deleteProperty: throwError,
  preventExtensions: throwError,
  setPrototypeOf: throwError;
}

This will turn the Wallet class to a read-only object and we could store all the account information in the isolated storage.

[sshelton76]

We already use localForage in another project I'm involved in. This kv-storage idea is great and is probably the wave of the future. However it wouldn't really change anything in the above design since we are talking about changing WHAT we store rather than HOW we store it.

Right now we just store a JSON.stringify'd version of the wallet, but at least inside of localstorage the privatekey is properly encrypted and managed.

The real problem here is that once decrypted, the wallet is fully available and the private key is readable in plaintext.

Which means that if you're in the DApp or it's still in memory, you're only a single XSS away from having your private keys read.

By storing the XOR'd bytes of the key though, it wouldn't matter even if someone managed to obtain the entire wallet from localstorage, or managed to get at the decrypted wallet object. It's useless without the user supplied part of the key.

A good short term fix would be to not load the privatekey at all until it's specifically needed. This way the privatekey isn't available to people who get a little too curious. However this might break code some code that's already deployed. Specifically code with it's own transaction signers. Yet I'm tempted to say this is a high enough level security issue, we might be better off breaking their code by removing privatekey from the marshalled object and only loading it when someone wants to sign. Add a requirment that they send the password through a second time in order to do that.

Because divulging the privatekey even unintentionally, could lead to all kinds of nastiness.

[sshelton76]

Wish there was a threaded conversation option here. My above reply is in reply to @nivida 's comment

Great idea! Feel free to work on it!
If possible we could use the kv-storage module where you can create isolated storage areas:
https://github.com/WICG/kv-storage
https://developers.google.com/web/updates/2019/03/kv-storage#what_if_a_browser_doesnt_support_a_built-in_module

@nivida 's other idea of setting the proxy to readonly is an excellent first step and doesn't break ANYTHING I can possibly think of.

[nivida]

Yes, exactly I see the same problem. My idea was to use a better storage solution then localStorage and to not have an array of accounts in the Wallet JS object itself. The Wallet object should have a get, has, and add method to interact with the accounts and not like now with direct access to an object e.g.: web3.eth.accounts.wallet['0x0...'].

Wallet class:

class Wallet {
  get(account, password): Account;
  has(account): boolean;
  count(): number;
  add(pk, password): Account;
}

[sshelton76]

Yeah, except what is in that Account object returned from get(account,password) ?

[nivida]

I've updated it to an Account object.

I think an interface where the Account object does handle the encryption of the PK would be better. We could add there the same strict proxy handler as we have in the Wallet class.

class Wallet {
  get(address: string): Account;
  has(address: string): boolean;
  count(): number;
  add(account: Account): Account;
}

class Account {
...
}

[sshelton76]

That seems like a valid design. Mostly we just want to keep the privatekey tucked away somewhere it cannot be accessed without some extra credential supplied by the end user.

[sshelton76]

For the account class

class Account {
...
}

I would add a "signWith" method, that takes a user password p and whatever it is that's being signed.
As well as an exportAccount function to get a jsonified version of the account as per what we have now in order to allow people to back up their accounts.

class Account {
     signWith(pass, data) : Uint8Array signed;
     exportAccount(pass) : string jsonObject;
}

[nivida]

I wouldn't add any sign method to the account it should just be a secure simple VO.
I would pass the pk to a sign method e.g.: ```accounts.sign(pk, data)``

class Account {
     getAddress(): string;
     getPrivateKey(pass) : Uint8Array signed;
     toJSON(pass) : Object;
}

[nivida]

To have a better UX could we add a unlock and lock method to the account and the password parameter could be optional.

Edit:
I think we should create out of this idea a new module called web3-eth-wallet and the current web3-eth-accounts module could wrap the new module. This will give us the possibility to provide backward compatibility and to provide a secure wallet.

[sshelton76]

I wouldn't add any sign method to the account it should just be a secure simple VO.
I would pass the pk to a sign method e.g.: ```accounts.sign(pk, data)`

Doing it that way would provide another place for the pk to leak. Honestly, that private key should never, ever leave the account object. Gold standard would be for someone who was analyzing the object at runtime to not be able to access the key either. This is why I recommend the key bytes XORd with a user supplied password, be stored and then during signing the full key is reconstructed.

To have a better UX could we add a unlock and lock method to the account and the password parameter could be optional.

It would be better at the library level to leave it locked all the time. The implementer can decide what conditions are needed to trigger a password prompt and cache the intermediate bytes for the duration of the user session. Honestly there isn't any reason to lock/unlock. Just default "this information isn't available because it is constructed at run time from two factors, only one of which is known to the app, the other is known to the user."

I think we should create out of this idea a new module called web3-eth-wallet and the current web3-eth-accounts module could wrap the new module. This will give us the possibility to provide backward compatibility and to provide a secure wallet.

+1000 on that last idea