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

mnemonic.make_seed: add "extra_entropy" arg, and expose it to CLI/RPC #8839

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SomberNight
Copy link
Member

@SomberNight SomberNight commented Jan 22, 2024

This is useful for the following threat-model:

The randomness generated by the CPU/OS is assumed weak, but otherwise the user trusts the CPU/OS to execute code as-is.
The user can inspect the source code to see what they provide as custom entropy is used in a sane way.

As the extra entropy is simply XOR-ed in, into the OS-generated randomness, it cannot be used as a footgun.

Note this significantly differs from the old custom_entropy option [0] that existed between version ~2.0 and 3.1.2 [1]:

  • the old option replaced the OS-entropy with the user-provided entropy
    • e.g. if the user provided what looked like 64 bits of entropy, and by default we wanted to create a 132 bit seed, the resulting seed only used 132-64=68 bits of os.urandom()
    • hence I think the old option was a footgun -- it required expert knowledge to use properly
    • instead, the new option mixes the user-provided entropy with os.urandom(), in a way that can never make the result have less entropy
  • with the old option, the "custom_entropy" arg was an int, of which every "bit" was used as-is
    • for example, if the user wanted to provide some dice rolls, e.g. "6 3 5 2 6 6 2", and they passed the int "6352662" (base 10), they lost a lot of entropy by not using high decimal digits - i.e. the user was required to know how to convert their entropy to the expected format (e.g. dice rolls as base6, and then convert to base10)
    • instead, the new option takes an arbitrary string, which is then hashed internally, hence it is not possible to misuse the API
      • e.g. it is safe to provide dice rolls as a string, e.g. "6 3 5 2 6 6 2" or "6352662" or in any imaginable way
  • the old option exposed a "check_seed" CLI command, that could be used to verify the user-provided entropy was used as part of the seed-generation. This is not possible with the new option.

closes #523

[0]: https://github.com/spesmilo/electrum/blame/883f9be7d15cf1aba16895a0848f0d7af99f2ff3/lib/mnemonic.py#L149
[1]: 5e5134b

@SomberNight
Copy link
Member Author

@ecdsa in 5e5134b you wrote you removed the option as it was redundant with seed extensions. With the threat model stated in the OP, I think that argument does not really apply:

  • as then if they don't trust the CPU/OS for entropy they might as well just use an "empty" seed (e.g. the 9dk seed in practice) with a very long passphrase
    • which is a bit ridiculous, and also makes the backup process error-prone

Note that the passphrase can obviously still be used in addition this, they are not redundant at all. For example, the passphrase can be used as a counter, to create "accounts" for the same seed. Or "hidden" wallets. etc.

This is useful for the following threat-model:
> The randomness generated by the CPU/OS is assumed weak, but otherwise the user trusts the CPU/OS to execute code as-is.
> The user can inspect the source code to see what they provide as custom entropy is used in a sane way.

As the extra entropy is simply XOR-ed in, into the OS-generated randomness, it cannot be used as a footgun.

Note this significantly differs from the old custom_entropy option [0] that existed between version ~2.0 and 3.1.2 [1]:
- the old option *replaced* the OS-entropy with the user-provided entropy
  - e.g. if the user provided what looked like 64 bits of entropy, and by default we wanted to create a 132 bit seed,
    the resulting seed only used 132-64=68 bits of os.urandom()
  - hence I think the old option was a footgun -- it required expert knowledge to use properly
  - instead, the new option mixes the user-provided entropy with os.urandom(), in a way that can never make the
    result have less entropy
- with the old option, the "custom_entropy" arg was an int, of which every "bit" was used as-is
  - for example, if the user wanted to provide some dice rolls,
    e.g. "6 3 5 2 6 6 2", and they passed the int "6352662" (base 10), they lost a lot of entropy by not using high decimal digits
    - i.e. the user was required to know *how* to convert their entropy to the expected format
    (e.g. dice rolls as base6, and then convert to base10)
  - instead, the new option takes an arbitrary string, which is then hashed internally, hence it is not possible to misuse the API
    - e.g. it is safe to provide dice rolls as a string, e.g. "6 3 5 2 6 6 2" or "6352662" or in any imaginable way
- the old option exposed a "check_seed" CLI command, that could be used to verify the user-provided entropy was used
  as part of the seed-generation. This is not possible with the new option.

related spesmilo#523 (comment)

[0]: https://github.com/spesmilo/electrum/blame/883f9be7d15cf1aba16895a0848f0d7af99f2ff3/lib/mnemonic.py#L149
[1]: 5e5134b
@@ -353,10 +353,12 @@ async def setconfig(self, key, value):
cv.set(value)

@command('')
async def make_seed(self, nbits=None, language=None, seed_type=None):
async def make_seed(self, nbits=None, language=None, seed_type=None, extra_entropy: str = None):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThomasV remarked:

re extra_entropy, I think that option should not be named that way. it is misleading. some users might believe that their seed will have more bits of entropy than what is passed in nbits

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative names:

  • addentropy, add_entropy, add_to_entropy,
  • mixin_entropy, mix_in_entropy,
  • user_entropy

Out of these, I prefer add_entropy or user_entropy, and re behaviour, we can describe how the value is used here:

'extra_entropy': (None, "Arbitrary string used as additional entropy"),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant