Unsafe prompts #1126

matejcik · 2020-07-23T13:49:58Z

partial fix for #1064

adds a setting called "unsafe prompts", available via trezorctl. When enabled, Keychain will not raise hard errors on non-conforming paths.

This necessitated (yet another) refactor of the Keychain, in which I took the time to also make a cleaner split on files.

The current Keychain implementation has a default curve set -- which is necessary, as previously we would take curve info from the namespace, and now we are being asked to derive paths that do not belong to a namespace.
The other option would have been to specify curve at every call to derive(), but that just seems spammy. We might come back to it in time though.

This way using "slip21" curve is not possible, so I had to introduce a separate derive_slip21() method and a corresponding slip21_namespaces argument. I don't particularly like this, but it seems best of bad options. If this grows cumbersome in the future, we could add a special keychain.slip21 accessor (and keychain.bip32 or keychain[curve_name] or something).

in addition, I removed the load_settings godfunction, in favor of individual setters.

matejcik · 2020-07-23T13:53:59Z

@andrewkozlik this is hitting you because you're a code owner of storage, but I'd appreciate a review of the Keychain design

fwiw we might add you as a code owner of apps.bitcoin now, right?

tsusanka

Partial review: except core/src/apps/common/keychain.py, I will try to review that soon:tm:.

core/src/apps/debug/load_device.py

core/src/apps/management/apply_settings.py

core/src/storage/device.py

tsusanka · 2020-07-23T14:28:55Z

python/src/trezorlib/cli/settings.py

@@ -132,6 +132,20 @@ def homescreen(client, filename):
    return device.apply_settings(client, homescreen=img)


+@cli.command()
+@click.argument("allow", type=click.Choice(("on", "off")))


Here we use "on/off" and for passphrase for example we use trezorctl set passphrase enabled. We should unify those IMHO. Also "on" might be the default value? Maybe let's create a separate issue for that?

#1130
I am not sure what to do here. I chose "on/off" instead of "enabled/disabled" because of a vague feeling that "set unsafe-prompts disabled" sounds like the less secure setting. But that's not very strong argument. Do you think we should change it to enabled/disabled?

I like on/off. I wouldn't mind having also "trezorctl set passphrase on". Anyway, since we are not sure I suggest to leave this as is and deal with it in #1130.

I chose "on/off" instead of "enabled/disabled" because of a vague feeling that "set unsafe-prompts disabled" sounds like the less secure setting.

I don't think on/off vs. enabled/disabled makes a difference in this respect. If a configuration sounds less secure it's more likely because of the name of the setting.

tsusanka · 2020-07-23T14:30:09Z

python/src/trezorlib/cli/settings.py

+    This is a power-user feature. With unsafe prompts enabled, Trezor will ask the user
+    to confirm possibly dangerous actions instead of rejecting them outright.
+    Use with caution.
+    """


I suggest we display this description to the user in CLI. Also with a Continue? [y/N] to make this a bit more scarier :).

I considered that, but OTOH the scary warning is already displayed on the Trezor screen?

Still - we can be more descriptive here. Exactly as you are in the comment.

tsusanka

keychain.py LGTM!

core/src/apps/common/keychain.py

tsusanka · 2020-07-24T07:07:57Z

Also let's add this to CHANGELOG.

andrewkozlik · 2020-07-24T09:55:30Z

core/src/apps/common/keychain.py

+        self.curve = curve
+        self.namespaces = namespaces
+        self.slip21_namespaces = slip21_namespaces
+        self.restrict = restrict


I am not sure how much sense it makes to set restrict per Keychain instance, because right now it's considered a global setting. On the other hand I would like to see the Keychain class moving in a direction, where it will just be holding the derived nodes that it was authorized to hold, i.e. it shouldn't keep a copy of the seed at all.

Re restrict, it's reasonable that the Keychain would look at the setting directly; I'm honestly not sure why I did it this way originally. I'll remove it.

7289d84 and 02fcf99

matejcik · 2020-07-24T10:10:57Z

#1126 (comment) -> moving discussion to top level so that it doesn't get lost in a review comment:

Re the other idea, that's intriguiging, but I'm not sure if it makes sense implementation-wise.
For one, the requirement "allow namespace override" means that we would need special behavior for always keeping the seed anyway?
For two, that would mean that we would need to derive and store all the prefixes, which could often take more time than the requested derivation itself (e.g., consider a GetAddress call)

Perhaps something like keychain.narrow() could request that explicitly with no option of going back? To be used where applicable.

andrewkozlik · 2020-07-24T10:25:01Z

Perhaps something like keychain.narrow() could request that explicitly with no option of going back? To be used where applicable.

Yeah, I am thinking something like that. Instead of the seed you'd be keeping the root node or one or more subnodes if you know beforehand that that's all you will be needing. Typically the Authorization objects would be doing that. BTW I am not suggesting we delve into this here, that's a separate issue, but it influences the way we think about the restrict parameter.

matejcik · 2020-07-24T11:33:43Z

Added commits that allow Casa/Greenaddress namespaces without prompts. @tsusanka please take a look.

core/src/apps/bitcoin/keychain.py

tsusanka

Looks good. I am not sure whether it makes sense to limit paths for sign_message since there is a "coin prefix". We also added a check lately not to sign an empty prefix.

matejcik · 2020-07-24T12:46:38Z

wrt naming:

in a meeting we decided to call the setting Safety checks, with possible levels being strict (default) and prompt (configurable by trezorctl). In the future we might add more granularity, e.g., to turn off prevtx streaming.

We decided to keep the naming as-is for this PR and for release. (This shouldn't even affect Connect, seeing as they won't be using this field at all.)

If we come up with something good, we can reword the dialogs relatively safely after the freeze.

For the next milestone (and for upcoming trezorlib release), I'm creating a separate issue for internal rewording.

andrewkozlik

Keychain LGTM, one nitpick.

andrewkozlik · 2020-07-24T11:40:35Z

core/src/apps/common/keychain.py

+        raise FORBIDDEN_KEY_PATH
+
+    def _derive_with_cache(
+        self, prefix_len: int, path: paths.PathType, new_root: Callable[[], NodeType],


The name prefix_len doesn't tell me what the parameter is doing. How about calling it max_cache_depth? But the function is private so no big deal.

i'm leaving that to #1135

also make a cleaner distinction between keychain, seed, path This enables using `unsafe_prompts`, because with the original code, if there was no namespace match, we wouldn't know which curve to use. For ease of implementation, we use a LRU cache for derived keys, instead of the original design "one cache entry per namespace". SLIP21 is now treated completely separately, via `slip21_namespaces` and `derive_slip21` method. If more slip21-like things come in the future, we can instead hang them on the keychain: put a per-curve Keychain object accessible by `keychain[curve_name].derive()`, and the majority usecase will just pass around `keychain[curve_name]` instead of having to specify the curve in every `derive()` call. Or alternately we'll just specify the curve in every `derive()` call, whichever seems more appropriate.

matejcik · 2020-07-24T14:11:23Z

@tsusanka a quick review of dde1945 if you could?

tsusanka · 2020-07-24T14:16:45Z

👍 one comment. I have probably not done it via the review functionality, sorry.

matejcik requested review from andrewkozlik, onvej-sl, prusnak and tsusanka as code owners July 23, 2020 13:49

matejcik removed request for onvej-sl and prusnak July 23, 2020 13:50

tsusanka reviewed Jul 23, 2020

View reviewed changes

tsusanka reviewed Jul 24, 2020

View reviewed changes

core/src/apps/common/keychain.py Outdated Show resolved Hide resolved

andrewkozlik reviewed Jul 24, 2020

View reviewed changes

matejcik requested a review from tsusanka July 24, 2020 11:33

prusnak reviewed Jul 24, 2020

View reviewed changes

core/src/apps/bitcoin/keychain.py Show resolved Hide resolved

tsusanka approved these changes Jul 24, 2020

View reviewed changes

tsusanka added this to the 2020-08 milestone Jul 24, 2020

andrewkozlik approved these changes Jul 24, 2020

View reviewed changes

This was referenced Jul 24, 2020

wording for "Safety checks" feature #1133

Closed

BIP-32 path schema support #1135

Closed

matejcik force-pushed the matejcik/unsafe-prompts-keychain-only branch from 39765eb to f80a0f9 Compare July 24, 2020 14:04

matejcik added 7 commits July 24, 2020 16:06

core: add option to allow unsafe prompts

b7d49df

core: replace load_settings with individual setters

ecf3c2c

core: improve type annotation for bip32.derive_path

a1ee651

core: update references to keychain everywhere

54b767c

tests: add test for unsafe prompts

c5ab349

core/tests: fix bitcoin unit tests

83ee257

matejcik added 8 commits July 24, 2020 16:08

core/tests: add unit tests for new functionality

aacf65a

core: raise error on setting passphrase-on-device without passphrase

2ef9cff

core: use public as named argument instead of comment

46ba34e

core/keychain: differentiate error message for ed25519 derivation

bc4bccd

core, python: add changelogs

443f85b

core/bitcoin: move BITCOIN_NAMES to common

58a1d0c

core/bitcoin: drop unused validate_path_for_bitcoin_public_key

f1f5816

core/bitcoin: allow compatibility namespaces for Casa/Greenaddress

2c4661e

matejcik force-pushed the matejcik/unsafe-prompts-keychain-only branch from f80a0f9 to 0ca9a54 Compare July 24, 2020 14:09

all: rename protobuf unsafe_prompts to safety_checks

dde1945

matejcik force-pushed the matejcik/unsafe-prompts-keychain-only branch from 0ca9a54 to dde1945 Compare July 24, 2020 14:10

matejcik requested a review from tsusanka July 24, 2020 14:11

tsusanka approved these changes Jul 24, 2020

View reviewed changes

matejcik merged commit fdcb64a into master Jul 24, 2020

matejcik deleted the matejcik/unsafe-prompts-keychain-only branch July 24, 2020 14:38

tsusanka mentioned this pull request Aug 21, 2020

Add option to settings to allow safety checks trezor/trezor-suite#2242

Closed

matejcik mentioned this pull request Sep 2, 2020

update usage of safety_checks in core code #1241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unsafe prompts #1126

Unsafe prompts #1126

matejcik commented Jul 23, 2020

matejcik commented Jul 23, 2020

tsusanka left a comment •

edited

Loading

tsusanka Jul 23, 2020

matejcik Jul 24, 2020

tsusanka Jul 24, 2020

andrewkozlik Jul 24, 2020

tsusanka Jul 23, 2020

matejcik Jul 23, 2020

tsusanka Jul 23, 2020 •

edited

Loading

tsusanka left a comment •

edited

Loading

tsusanka commented Jul 24, 2020

andrewkozlik Jul 24, 2020

matejcik Jul 24, 2020

matejcik Jul 24, 2020

matejcik commented Jul 24, 2020

andrewkozlik commented Jul 24, 2020

matejcik commented Jul 24, 2020

tsusanka left a comment

matejcik commented Jul 24, 2020

andrewkozlik left a comment

andrewkozlik Jul 24, 2020

matejcik Jul 24, 2020

matejcik commented Jul 24, 2020

tsusanka commented Jul 24, 2020

Unsafe prompts #1126

Unsafe prompts #1126

Conversation

matejcik commented Jul 23, 2020

matejcik commented Jul 23, 2020

tsusanka left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsusanka Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

tsusanka left a comment • edited Loading

Choose a reason for hiding this comment

tsusanka commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik commented Jul 24, 2020

andrewkozlik commented Jul 24, 2020

matejcik commented Jul 24, 2020

tsusanka left a comment

Choose a reason for hiding this comment

matejcik commented Jul 24, 2020

andrewkozlik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik commented Jul 24, 2020

tsusanka commented Jul 24, 2020

tsusanka left a comment •

edited

Loading

tsusanka Jul 23, 2020 •

edited

Loading

tsusanka left a comment •

edited

Loading