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.

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

Wallet private key derivation is brittle #4170

Closed
AaronFeickert opened this issue Jun 7, 2022 · 1 comment
Closed

Wallet private key derivation is brittle #4170

AaronFeickert opened this issue Jun 7, 2022 · 1 comment
Labels
A-security Area - Security related

Comments

@AaronFeickert
Copy link
Collaborator

Wallet key derivation relies on concatenation of seed entropy, a variable-length branch string, and an index. These values are hashed after concatenation using SHA-256, and parsed as a private key.

This approach is brittle:

  • The concatenation can collide on distinct inputs, since arbitrary-length branch strings are allowed and the key index is not subject to a fixed-length (or even platform-independent) encoding
  • The hash instantiation is not domain separated, which is unsafe if the inputs (or parts of them) are used elsewhere; it is therefore unsuitable as a key derivation function

The following are recommended:

  • Use a modern key derivation function like Blake2b or Blake3
  • Prepend the branch string length (where this length is itself in a fixed-length encoding) when encoding it as hash input
  • Use a fixed-length and platform-independent encoding for the key index

Any future hashing API should be considered when addressing this issue.

@jorgeantonio21
Copy link
Contributor

jorgeantonio21 commented Jul 17, 2022

A possible approach to this issue can be found here. We make use of the current hashing API, for proper domain separation. Moreover, current key index fixed length encoding is obtained by use of little endian representation of an u64.

aviator-app bot pushed a commit that referenced this issue Jul 28, 2022
Description
--- Fix issues #4138, #4333 and #4170.

Motivation and Context
--- Add domain separation and cipher authentication for out bound messaging

How Has This Been Tested?
--- Unit tests.
stringhandler pushed a commit that referenced this issue Aug 2, 2022
…n (see issue #4170) (#4316)

Description
--- 
Add domain separation for wallet key derivation for type `KeyManager`.

Motivation and Context
--- 
The current wallet key derivation uses plain concatenation of variable length inputs. This procedure is known to not be collision resistant. A better methodology, as pointed [here](#4170), is to prepend the length of each datum to it before passing it through the hash function. We attain this by using the current hashing [API](https://github.com/tari-project/tari-crypto/blob/main/src/hashing.rs), which gives a suitable interface for domain separation, (which always prepends input length).

How Has This Been Tested?
---
 Unit tests.
@hansieodendaal hansieodendaal added the A-security Area - Security related label Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security Area - Security related
Projects
None yet
Development

No branches or pull requests

3 participants