New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wallet encryption #214

Merged
merged 4 commits into from Feb 14, 2018

Conversation

Projects
None yet
4 participants
@dethos
Copy link
Contributor

dethos commented Feb 8, 2018

What current issue(s) does this address?, or what feature is it adding?
This pull request addresses a wallet security issue present in the current master branch. The issue regards the encryption method being used, as it does not match the one used on the reference implementation. At this moment the master key is encrypted using the passwordHash which is also stored on the wallet (the key is right next to the lock).

How did you solve this problem?
The problem was solved using the same method that is used in the reference (C#) implementation. The software should calculate the sha256 hash of the password and then calculate the sha256 hash of the hash, and call it encryption key. This key should be used to encrypt the master key (and never stored, only the sha256 hash of it).

How did you make sure your solution works?
Manually. Initially I could open all my wallets without needing their passwords (just by removing 2/3 lines of code). Now I can't open the wallets without a valid password.

Did you add any tests?
No

Are there any special changes in the code that we should be aware of?
After this pull request is merged, users will not be able to open the wallets created by previous versions. Because of this and as a PoC, a small script (reencrypt_wallet.py) was made to reset the wallet's current password in a way that it could be used in this new version.
The script duplicates the wallet to avoid losing any data. Perhaps it should be recommended that after "migrating" and making sure the new wallet can be opened, to create a new wallet and transfer everything to it, since this script does not change the master key, it only re-encrypts it with a new key.

Did you run make lint and make test?
Yes, make test

Are you making a PR to a feature branch or development rather than master?
This PR is being made into a feature-branch as indicated by the repository maintainer, at the time he was informed of the issue.

cc: @localhuman @metachris

@dethos dethos changed the title Fix wallets encryption Fix wallet encryption Feb 8, 2018

@metachris

This comment has been minimized.

Copy link
Collaborator

metachris commented Feb 8, 2018

Thanks, looks important. Can we detect if the user is trying to open an old wallet and print some info?

Btw, the tests are failing, please run make lint and make test.

@localhuman what do you think?

@localhuman

This comment has been minimized.

Copy link
Collaborator

localhuman commented Feb 8, 2018

@metachris this is an important changeset, thanks very much to @dethos for bringing up the issue and solution 🥇.

We'll need to fix the tests before we can get much further, we can go from there

@dethos

This comment has been minimized.

Copy link
Contributor Author

dethos commented Feb 8, 2018

Correct 👍 , I will try to fix the tests as soon as possible, so you can move forward with this.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 9, 2018

Coverage Status

Coverage decreased (-0.02%) to 70.113% when pulling ac9e456 on dethos:fix-wallets-encryption into d5612d1 on CityOfZion:feature-wallet-changes.

@dethos

This comment has been minimized.

Copy link
Contributor Author

dethos commented Feb 9, 2018

@localhuman the tests were fixed and the test wallets "migrated" using the reencrypt_wallet.py script.

@localhuman localhuman merged commit 15a318c into CityOfZion:feature-wallet-changes Feb 14, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.02%) to 70.113%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@localhuman localhuman referenced this pull request Feb 16, 2018

Merged

Feature wallet changes #232

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment