search

urbit / urbit-key-generation

Key derivation and HD wallet generation functions for Urbit
MIT License
15 stars 8 forks source link

Silently accepts "invalid" hex strings #84

Closed Fang- closed 4 years ago

Fang- commented 4 years ago

I went bug hunting but found a can of worms instead.

See for example the argument to deriveNetworkKeys:

https://github.com/urbit/urbit-key-generation/blob/a5843d74d33681d47627a094b078046986df7ba7/src/index.js#L223

Then, observe the behavior:

// considered valid, produces unique keys
> kg.deriveNetworkKeys('abcd')
{ crypt: { private: 'beabf9c807eab8130441b5fef1b028509b221fc499d7925505a45003b2e5d404', ...

// invalid because leading 0x
> kg.deriveNetworkKeys('0xabcd')
{ crypt: { private: '3eda27f97a3238a5817a4147bd31b9632fec7e87d21883ffb0f2855d3cd1d047', ...

// all invalid values produce the same keys
> kg.deriveNetworkKeys('0xefff')
{ crypt: { private: '3eda27f97a3238a5817a4147bd31b9632fec7e87d21883ffb0f2855d3cd1d047', ...

Sanest behavior here, I think:

  1. check for leading 0x and strip that if present
  2. if the resulting seed buffer is the empty buffer (as in, result of Buffer.from('invalid', 'hex')), then throw an "invalid hex" error.

Will start work on that PR right now.