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

[sshelton76]

The way things are handled in the image above is exactly the way we should be doing it once the wallet has been loaded (the image above is before I loaded the wallet and is just me exploring localStorage).

There's no need to decrypt the private key at all except for transaction signing, thus for the interim maybe we load it sans password and then require the password to decrypt for signing. I think that would be a much easier thing to do and would break a lot less things and would solve the initial reason behind this which is that the privatekey is vulnerable to ex-filtration via XSS as soon as the wallet is loaded.

[nivida]

Honestly, that private key should never, ever leave the account object.

Yes, I agree with you. We would have to move the TransactionSigner object from the Eth module to the Wallet and Account object.

It would be better at the library level to leave it locked all the time.

Yes, I agree with you. Let us do your proposed solution!

thus for the interim maybe we load it sans password and then require the password to decrypt for signing.

Yes if we are able to pass it securely why not.

Edit:
But we could also document it in a way that every developer understands that he has to unlock the account until the txHash got returned.

[nivida]

1# Solution:

class Wallet {
    constructor(transactionSigner: TransactionSigner);
    get(address: string): Account;
    has(address: string): boolean;
    add(account: Account, pass: string): Account;
    count(): number;
}

class Account {
    constructor(address: string, pk: string, transactionSigner: TransactionSigner);
    getAddress(): string;
    getPrivateKey(pass: string): Uint8Array signed;
    toJSON(pass: string): Object;
    sign(pass: string, data: string): string;
    signTransaction(pass, tx): SignedTransaction;
}

2# Solution:

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

class Account {
    constructor(address, pk);
    getAddress(): string;
    getPrivateKey(pass): Uint8Array signed;
    toJSON(pass): Object;
    unlock(pass): boolean;
    lock(): boolean;
}

3# Solution:

class Wallet {
    constructor(transactionSigner: TransactionSigner);
    get(address: string): Account;
    has(address: string): boolean;
    add(account: Account, pass: string): Account;
    count(): number;
    sign(account: Account, pass: string, data: string): string;
    signTransaction(account: Account, pass: string, tx: Transaction): SignedTransaction;
}

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

[nivida]

In the above interfaces of the Wallet class is the create method missing:
create(pass: string, entropy?: string): Account

Btw.: The new module should also fix #2725.

[sshelton76]

@nivida These are all excellent solutions. Let me think about them a bit and try to work through what they mean security wise. My background is security and crypto so I feel I can help most in this regard. In the end though this will probably be something we need to phase in, in order to give people time to adapt.

My opinion is that structure wise, the Account object should be an opaque black box that handles everything that would normally require the private key.

The Wallet object should be a wrapper around a collection of Accounts. One that manages adding Accounts, removing them encoding transactions, tracking history etc and when something needs signed it should be passed to the Wallet which then passes it to the Account for secure signing.

[nivida]

@sshelton76 My favorite solution is the third one. We could also define all get handlers in the proxy object of the Account object. This would allow us to restrict calling any property or method which isn't defined in the provided interface.

[sshelton76]

In the above interfaces of the Wallet class is the create method missing:
create(pass: string, entropy?: string): Account
Btw.: The new module should also fix #2725.

If the wallet is going to also create deterministic Accounts from an initial entropy value, we might get more mileage just incorporating the HD wallet code from ethereumjs-wallet and modifying it to keep the private key more secure. It wouldn't be much different to submit a PR there, especially since the plan to fix #2725 was to adopt their wallet code which was already a plan for other reasons.

[sshelton76]

BTW if you notice in the first screenshot I submitted, we also keep the mnemonic stored encrypted inside localStorage. This is encrypted using nacl secretbox encryption and a user supplied password, also fed to a pbkdf. The current app I'm showing you, recreates the entire wallet on demand from that encrypted mnemonic, and it's one of the reasons I opened up this discussion. For my app, the "wallet" doesn't even exist until the user needs to sign something.

I got the overall idea for our approach from AIM a chrome extension using ethereum that creates high strength passwords and stores them inside of a smart contract on Rinkeby. It's meant as an alternative to LastPass, KeyPass and the others because they could go out of business, but blockchain is eternal, ergo no chance you'll lose access one day.

However since this literally amounts to broadcasting the websites you log into to the world as well as your website username and password, there were some additional security considerations taken to keep it secure.

[sshelton76]

@nivida in reply to...

@sshelton76 My favorite solution is the third one. We could also define all get handlers in the proxy object of the Account object. This would allow us to restrict calling any property or method which isn't defined in the provided interface.

I agree, except for where signing is actually occurring. Sure let the wallet pass that request through to the Account, but it should be the Account object doing the heavylifting on signing.

[nivida]

I agree, except for where signing is actually occurring. Sure let the wallet pass that request through to the Account, but it should be the Account object doing the heavylifting on signing.

Yes I see your point.
But we would have to do: Account.from(object, transactionSigner) instead of Account.from(object)

[sshelton76]

Yes I see your point.
But we would have to do: Account.from(object, transactionSigner) instead of Account.from(object)

Right but if the wallet is responsible then you can still do Account.from(object) and just have the wallet pass the transactionSigner into the Account.sign method.

[nivida]

Right but if the wallet is responsible then you can still do Account.from(object) and just have the wallet pass the transactionSigner into the Account.sign method.

True that ;)

[sshelton76]

So I decided to check my assumptions on the pbkdf side of things.

Almost unbelievably, 262144 rounds of SHA-256 with the browser's built in "crypto.subtle.digest", takes 18000ms or ~18 seconds. Yet using nacl.hash for the same 262144 rounds completes in only 3700ms or ~3.7 seconds. I am using tweetnacljs-fast which is highly optimized for speed. But I fully expected it to be slower than the browser's built in functions.

Now the thing to keep in mind is that nacl.hash uses SHA-512 by default, but this is a non-issue for a key derivation function because we can just truncate the final key to the length we need for a proper XOR op. Plus when it comes to key sizes, bigger is better for general encryption purposes.

I'll dive into ethereumjs-wallet and see what they're using for this part. But the results I got just now floored me so much I just had to share.

update
Ok so I walked through their dependencies. Looks like they are ultimately using sha.js under the hood. I had to walk a fairly deep dependency graph to find that. I've examined the code and I can tell just by looking that sha.js is going to be a lot slower. For one thing it's using arrays instead of ArrayBuffers and TypedArrays, and we already know there's a huge performance penalty for that.

[nivida]

The bigint-hash package which would help does sadly just run on a nodeJS runtime.

[dbforma]

Any updates on this?

[defipunk]

Every single project that is using web3 right now (more than 3k dependents) has an "npm audit" warning due to this issue.

This normalizes vulnerability warnings and should not be acceptable.

It also leads to frequent reports of the same issue: #2739 (comment)

Is there any update on this?

[natclark]

Why are we worried about adding support for Berlin and updating the docs when this high-severity vulnerability has been sitting in the package for months?

Even if it's not that "high-severity" in practice, like @defipunk mentioned, it sets a bad precedent.

[tr8dr]

This is a critical security issue, and has been almost 2 yrs since reported. What is the status of this?

[wbt]

Even if the underlying issue itself is not that serious, (a) it's still enough to trigger warnings and audit failures, AND (b) the age of the open issue this clearly reveals where security stands as a relative priority for the maintenance and development of this project.

Either one of those are sufficient reason for serious users / those in an enterprise setting to move off of web3 and everything that depends on it.

[aytvill]

#jwz once wrote article, which ends with

But that's what happens when there is no incentive for people to do the parts of programming that aren't fun. Fixing bugs isn't fun; going through the bug list isn't fun; but rewriting everything from scratch is fun (because "this time it will be done right", ha ha) and so that's what happens, over and over again.

authors seem to belong to same CADT model

[wbt]

Closing as completed and splitting the history to a new issue doesn't seem to accomplish a whole lot more than getting fewer eyes on updates.