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 https://github.com/ethereum/web3.js/issues/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.

[sshelton76]

Alright, so let's break this conversation down into 2 parts.

1 My analysis shows that there is an XSS vulnerability in the current implementation of the wallet that could result in private key leakage. This is high level security vulnerability and it needs re-mediated right away, but in such a manner that it breaks the minimum amount of existing code as possible. So #1 needs to be addressed ASAP.

2 There are numerous ways to reconstruct the wallet system to make it far more secure than it is now. However it will break current APIs and thus we should table that discussion until 1.0 has been released and then target for a new wallet system in the 1.0 to 2.0 cycle.

I have a solution to #1 and it's pretty simple and is inline with existing practices for RPC.

The current problem is rooted in the fact that once a wallet is loaded from storage, all details about the wallet including the privateKey are immediately available to all and sundry. In truth the need for protected details such as the privateKey is very limited and we need to take precautions in order to guard that key from attackers whether they be a malicious website or a plugin, or even just errant code.

Here are the changes we should make... When loading a wallet from localstorage we keep the process the same, but we do not populate the privateKey field. Instead we make the privateKey field a getter with a check to see if the wallet is presently unlocked.

Therefore, anything that needs the privateKey will have to first call wallet.unlock(account,password). Unlocking will cause the privateKey to be decrypted and the next time someone accesses account.privateKey they get the key string that they presently do.

Ergo to access the privateKey will require...

await wallet.unlock('0',password);
let pk = wallet['0'].privateKey;
...

Furthermore, I'm going to recommend that upon being read, the privateKey getter immediately locks the wallet, thereby preventing further use of the privateKey. This prevents malicious entities from simply waiting for the unlock event to do their deeds. If someone attempts to access the privateKey while locked it will throw an exception,

If you agree with me that this is the correct approach, then I will submit a PR shortly in the next day or two.

[nivida]

Thanks for your conclusion and the provided solution!

But wouldn't it be possible to create a web3-eth-wallet module with the following interface:

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;
}

Edit: I could create a starter module with some comments if required.

[sshelton76]

Thanks for your conclusion and the provided solution!

But wouldn't it be possible to create a web3-eth-wallet module with the following interface

While it's possible, it would be a major breaking change if we did it right away and got rid of the old wallet in the process. My opinion is that a phased in approach would be best especially this close to release.

As it stands I'm tempted to modify my own solution so that we keep existing behavior for now but add a deprecation warning and then begin enforcing the new behavior with the last RC before release. This will give folks a little time to upgrade.

It would look something like...

let w = wallet.loadSecure(name);
await w.unlock("0",password);
let privateKey = w["0"].privateKey;

Then of course it would autolock once it coughed up the privateKey.

In the meantime we would just add a deprecation warning to wallet.load(password,name) and remove it completely sometime during the RC cycle.

[nivida]

While it's possible, it would be a major breaking change if we did it right away and got rid of the old wallet in the process. My opinion is that a phased in approach would be best especially this close to release.

Yes, but if we create a web3-eth-wallet module and we do use this module in the accounts module can we improve the current module and provide backward compatibility. We can have the web3-eth-wallet module as a module by side of the current accounts module.

[sshelton76]

Ok so this is embarrassing.

I went through and performed all the changes I recommended against a recent pull of Web3js.

Then I went ahead and analyzed it thoroughly and came to a conclusion. The flaw here is putting a wallet object in the global space at all.

Doing it my original way is overkill and breaks a lot of things. It's still a bit more secure because even access to the cipher text, does not grant access to the key even with unlimited amounts of time to brute force. The user has a per account password which is hashed/stretched then XOR'd over the privatekey before storage, making key recovery impossible.

But in retrospect doing it that way breaks so much that it would make a better candidate for 2.0.

In the meantime the best and simplest answer is to only load / decrypt the wallet when you actually need the private key or public key. When you don't need access to the keys themselves, you can access the addresses just by loading from local storage using

let wallet = JSON.parse(localStorage["web3js_wallet"])

By doing it this way and only decrypting when we need access to the keystore, the security level is much, much higher than simply loading the wallet and attaching it to a global object for later use.

I would like to recommend that sometime between 1.0 and 1.5 we have the "load" function do only the above and then have a separate "decrypt" function which does what the previous load function did, i.e. decrypting the keystore.

For 2.0 we can look at full redesign involving an XOR mask using a user created pin or password, just to add an extra layer of security.

@nivida If we update the docs with an explicit instruction to NEVER EVER perform wallet.load into an object in the global scope, because it could cause private key leakage via an XSS, then this addresses my security concern. So updating the docs should be all that's needed to close this issue and we can revisit it sometime after the 1.0 RELEASE.

Thanks!

[nivida]

Thanks for doing all these tests and providing the required details for following up your progress!

By doing it this way and only decrypting when we need access to the keystore, the security level is much, much higher than simply loading the wallet and attaching it to a global object for later use.

Yes, I really like the idea of not adding the decrypted keystores to the Wallet object.

I would like to recommend that sometime between 1.0 and 1.5 we have the "load" function do only the above and then have a separate "decrypt" function which does what the previous load function did, i.e. decrypting the keystore.

Sounds good to me if the decrypt method is showing up a prompt for entering the user-specific password. (as you already have shown in the referenced issue #2825)

For 2.0 we can look at full redesign involving an XOR mask using a user created pin or password, just to add an extra layer of security.

Yes, I agree with you these changes would break too much and have to be released later.

If we update the docs with an explicit instruction to NEVER EVER perform wallet.load into an object in the global scope, because it could cause private key leakage via an XSS, then this addresses my security concern. So updating the docs should be all that's needed to close this issue and we can revisit it sometime after the 1.0 RELEASE.

Yes, I've added the console warning some months ago and will update the documentation. Thanks for the hint.

Is there a public GitHub repository for testing your POC closer?

Edit: We shouldn't forget about the usage of the wallet in a NodeJS runtime. (prompt)

[sshelton76]

Yeah I don't think web3js has any business handling things like prompting for the user's password. Things like that are most definitely a UI layer thing. I am only saying that the decryption action itself should decrypt only as long as absolutely necessary to complete the task at hand. If there were a sever side password cache for server apps that would be up to the implementor.

I did not keep my POC, once I realized how many things were breaking, I realized it was the wrong course of action and gave the problem a complete rethink. Once we get to 1.0 RELEASE I will update my fork here and begin with a new implementation. But for the short term I have my own deliverables within XVault I need to ship ASAP and so I've been focused on completing that. I won't have another break to look at the wallet refactor until July or August.

[perich]

WhiteSource just flagged this issue and sent out a GitHub security alert email, so there will probably be a lot of new eyes on this issue today. For those just arriving, this seems to be the current state of the issue (via @sshelton76 above):

"If we update the docs with an explicit instruction to NEVER EVER perform wallet.load into an object in the global scope, because it could cause private key leakage via an XSS, then this addresses my security concern. So updating the docs should be all that's needed to close this issue and we can revisit it sometime after the 1.0 RELEASE."

As I understand it, there is no patch available yet. Right now, any developers who are using the library should check their implementations to make sure they aren't leaking in the global scope.

[nivida]

@perich Thanks for giving anyone a hint!

I will implement the secure wallet in version 2.0 of Web3 and will improve or remove the current wallet in version 1.0. The reasons for this decision are explained above and further details about the roadmap will be published asap.

[sshelton76]

To be clear, there is at present nothing wrong with the wallet as is. We could re-implement this system to be much more secure than it already is, and that is a good target for 2.0, but the current implementation isn't insecure in any realistic way if used properly.

To use it properly, you just need to NOT perform wallet.load into any object available from within the global scope. Instead create a local variable, assign to it with wallet.load and pass it around as needed then simply delete it as soon as you are done with it. That addresses the issues I mentioned. In short, I wasn't using it right and that was what introduced the vulnerability.

[nivida]

@sshelton76 Calling of web3.eth.accounts.wallet.add(...) is also insecure because the account object gets added to the wallet with the privateKey. The only safe way to use the wallet is to not leak the whole Web3 object to the global scope. If you use the web3-eth-accounts module as a standalone module then keep the accounts module reference also out of the global scope.

[sshelton76]

@nivida Fair enough, I didn't think about it because that's not how I'm using it. The wallet is not loaded until the privatekey needs to be used. We don't add privatekeys after wallet generation is complete and we explicitly "delete wallet" after it has been saved.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions

[wbt]

I don't think this should be marked as stale.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[wbt]

@GregTheGreek you have to remove the "stale" label when reopening, if you want to keep it open for a little longer.

[huntr-helper]

Bug Bounty

We have opened up a bounty for this issue on our bug bounty platform. Want to solve this vulnerability and get rewarded 💰? Go to https://huntr.dev/

We will submit a pull request directly to your repository with the fix as soon as possible. Want to learn more? Go to https://github.com/418sec/huntr 📚

Automatically generated by @huntr-helper...

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[developerfred]

@GregTheGreek , Happy New Year! Do you have updates about this feature?

[GregTheGreek]

@developerfred someone had mentioned they were working on it. Seems to have gone stale.

[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: https://github.com/ChainSafe/web3.js/issues/2739#issuecomment-807598271

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?