Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Review wallet encryption mechanism #3573

Open
str4d opened this issue Oct 8, 2018 · 3 comments
Open

Review wallet encryption mechanism #3573

str4d opened this issue Oct 8, 2018 · 3 comments
Labels
A-crypto Area: Cryptography A-wallet Area: Wallet I-SECURITY Problems and improvements related to security.
Projects
Arborist Team

Comments

@str4d
Copy link
Contributor

str4d commented Oct 8, 2018 •
edited

If we ever decide to make this a non-experimental feature, we must review the implementation and update it as necessary (for both security and consistency). We can do this arbitrarily because experimental features have no user stability guarantees, and anyone who was using it previously can be expected to migrate to a fresh wallet.dat.
@str4d str4d added I-SECURITY Problems and improvements related to security. A-wallet Area: Wallet A-crypto Area: Cryptography labels Oct 8, 2018
@str4d str4d mentioned this issue Oct 8, 2018
Merged
@str4d
Copy link
Contributor Author

str4d commented Oct 8, 2018

e.g. #3517 (comment)

@str4d
Copy link
Contributor Author

str4d commented Oct 16, 2018

See also #1528 for a UX blocker that might be solveable by implementation changes.

@mms710 mms710 added this to Needs Prioritization in Arborist Team Jan 3, 2019
@str4d str4d mentioned this issue Jan 31, 2019
Open
@Eirik0 Eirik0 moved this from Needs Prioritization to Bugs/TechDebt Backlog in Arborist Team Sep 12, 2019
@Eirik0 Eirik0 moved this from Bugs/TechDebt Backlog to Security Issue Backlog in Arborist Team Sep 12, 2019
@softminus
Copy link
Contributor

We should make sure we use something with authenticated encryption, the current scheme depends on CBCDecrypt verifying PKCS#7-style padding, which has a non-negligible probability of not detecting decryption failure: approximately 257/65536. If the decryption failure is not detected, DecryptMnemonicSeed (and presumably other functions) will happily attempt to deserialize and use random data, causing sporadic test failures: #5807

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-crypto Area: Cryptography A-wallet Area: Wallet I-SECURITY Problems and improvements related to security.
Projects
Arborist Team
  
Security Issue Backlog
Milestone
No milestone
Development

No branches or pull requests
4 participants
@daira @softminus @str4d @zebambam