Encrypted walletstore and keystore #2426
Conversation
mcdee
changed the title
mcdee
changed the title
|
|
||
| ## Abstract | ||
|
|
||
| Ethereum [walletstore](https://eips.ethereum.org/EIPS/eip-2386) and [keystore](https://eips.ethereum.org/EIPS/eip-2335) files (hereafter just "stores") have various fields that some users may consider private (_e.g._ name, pubkey). In addition, there is always the risk of stores being stolen or made publicly accessible. To counteract potential issues this defines an encrypted format for stores. |
There was a problem hiding this comment.
this link is a 404 for me
There was a problem hiding this comment.
Ah I see it references this: #2386 which is not merged yet
|
|
@paulmillr main reason is because this is what is already used in the related ERCs, most especially #2335, so the same encryption/decryption code can be used for both. |
| - iv: the initialization vector as generated above | ||
| - data: the store as supplied above | ||
|
|
||
| Finally, a _checksum_ is generated by appending the encrypted data to the last 16 bytes of the encryption key and calculating its [SHA-256](https://tools.ietf.org/html/rfc6234) hash. |
There was a problem hiding this comment.
This is not safe, use HMAC or AEAD instead.
There was a problem hiding this comment.
Thank you for your comment. Please could you provide information regarding what specifically is unsafe so that it can be examined? Thank you.
There was a problem hiding this comment.
You can start by reading HMAC Design Principles
There was a problem hiding this comment.
I'm afraid I must ask again: what specifically is unsafe here?
Given the lack of information to date I will assume your concern is about a length extension attack on the checksum. However, the checksum is not a MAC: a successful length extension attack would not result in a valid keystore or walletstore, and as such would be immediately noticed.
If the concern is about something else please could you expand? And if my understanding above is incorrect please could you provide more detail? Thank you.
There was a problem hiding this comment.
Your design is vulnerable to chosen cipher text attacks because the MAC can be forged by length extension. If you don't need to detect malicious modifications of the ciphertext before processing it then you don't need to include a secret in the hash input. If the modifications always result an invalid keystore then the impact of the potential attack is low but the impact can be higher in a future implementation with a slightly different use case. Chosen ciphertext attacks can have higher impact in Interactive systems or when they are used to create side channels.
There was a problem hiding this comment.
Your design is vulnerable to chosen cipher text attacks because the MAC can be forged by length extension.
But it's just a checksum, not a MAC.
If you don't need to detect malicious modifications of the ciphertext before processing it then you don't need to include a secret in the hash input.
This is true, but as per the spec it carries out the same process as #2335 to allow for re-use of code (hence less code hence easier testing).
If the modifications always result an invalid keystore then the impact of the potential attack is low but the impact can be higher in a future implementation with a slightly different use case.
This is true, although I don't see this being used elsewhere.
Paging @CarlBeek for his thoughts, as he's the author of #2335 and this vulnerability could have a higher impact there.
| Encrypting the store requires the following information: | ||
|
|
||
| - the store to encrypt, which MUST be at least 16 bytes in length | ||
| - the passphrase, which MUST be at least 1 byte in length |
There was a problem hiding this comment.
What is the point of accepting 1 byte "passphrases" ?
There was a problem hiding this comment.
It's more of a "> 0" check, which catches many issues of bad coding where empty or null values are given as the passphrase. Picking an actual minimum passphrase length falls in to the realm of good security practice, which although important is not part of the ERC.
|
There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review. |
|
This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment. |
No description provided.