trezor / trezor-firmware

:lock: Trezor Firmware Monorepo
https://trezor.io
Other
1.35k stars 652 forks source link

Revise mechanism for obtaining secrets from Trezor #1508

Open andrewkozlik opened 3 years ago

andrewkozlik commented 3 years ago

Applications that need to obtain a secret value from Trezor can currently use the CipherKeyValue message, see SLIP11. The password manager (SLIP16) uses this to decrypt the file containing the password metadata and to decrypt the individual passwords. The CipherKeyValue system uses a very generic confirmation dialog on Trezor and the displayed strings, such as "Unlock encrypted storage?" or "Unlock github.com for user Satoshi Nakamoto?", are involved in the encryption/decryption process. One problem with this is that it is not well suited for translations or, to be more precise, for switching between languages and for changing the message in the future.

image image

Another use-case we encountered recently is obtaining private keys for Decred staking, see https://github.com/trezor/trezor-firmware/pull/1249#issuecomment-776725092. For staking the wallet needs to be online. Since the staking private keys cannot be misused to cause coin loss, a desktop wallet could generate them independently of Trezor. However, it's useful for the keys to be recoverable from the seed, so Trezor could be used as a source of some secret, which is handed over to the desktop application (after user confirmation). The desktop application can then derive the private keys from that secret. In addition it would be useful for Trezor to conduct the same derivation process on the device to derive the corresponding public keys, so that when it's creating a staking ticket it may verify that it owns the voting rights to the ticket.

Let's gather other possible use-cases for the above feature in this issue and decide whether we want to redesign the current mechanism or introduce an additional mechanism for obtaining secret values from Trezor.

tsusanka commented 3 years ago

We also use this with labeling and I am pretty sure the headline "Encrypt value" confuses some users.

emu00000000

prusnak commented 3 years ago

Let's discuss this together with FIDO2 extensions: hmac-secret, txAuthSimple and txAuthGeneric

andrewkozlik commented 7 months ago

In order to make use of translations in the dialogs and to avoid strange prompts, such as "Encrypt value, Enable labeling?", Trezor needs to understand what secret it is being asked to release. We could define a fairly generic message type with an enum to determine the requested secret. Based on the enum value and optional parameters Trezor would:

  1. determine whether a confirmation dialog needs to be displayed,
  2. optionally display a specific dialog, making use of translations, and
  3. determine how to compute the secret from the optional parameters.

Three types of secret that could be requested come to mind:

The obvious disadvantage of this system over SLIP-11 is that in order to support a new feature the firmware would need to be updated with a new dialog and processing instructions telling it how to obtain the result.

message GetSecret {
    optional SecretType secret_type = 1;  // The type of secret that Trezor is being asked to release.
    enum SecretType {
        GetLabelingKey = 1;         // The encryption key used for account and transaction labels. Parameters: none. No user confirmation.
        GetPasswordDbKey = 2;       // The encryption key used for the password database. Parameters: none. No user confirmation.
        EncryptPassword = 3;        // Encrypt a password. Parameters: service, user name, password. No user confirmation.
        DecryptPassword = 4;        // Decrypt a password. Parameters: service, user name, password. User confirmation required.
    }
    repeated bytes parameters = 2;
}

message Secret {
    optional bytes secret = 1;  // A symmetric key, ciphertext or plaintext, depending on the requested secret type.
}

message GetPrivateKey {
    repeated uint32 address_n = 1;         // BIP-32 path to derive the key from the master node.
    optional string ecdsa_curve_name = 2;  // ECDSA curve name to use.
}

message PrivateKey {
    optional string xprv = 1;  // Extended private key.
}

The purpose of this proposal is to define an easily extendable API. At this moment we are only interested in implementing GetLabelingKey. Other types, including the GetPrivateKey message can be added later as needed.

andrewkozlik commented 7 months ago

@matejcik what do you think about the proposal above? The main advantage over the existing CipherKeyValue scheme is in shifting the responsibility for the displayed message onto Trezor, i.e. enabling translations. In actual fact, right now we just need to implement the GetLabelingKey function, which should be silent. So the implementation effort on Trezor side is fairly minimal and Suite could start using it right away instead of doing 2 migrations:

  1. CipherKeyValue with dialog -> silent CipherKeyValue now (https://github.com/trezor/trezor-suite/pull/9130).
  2. Silent CipherKeyValue -> GetSecret some time in the future.

I personally don't have any preference about whether we should do it now or later. cc @matejkriz, @tsusanka and @mroz22

matejkriz commented 7 months ago

If we migrate "labeling" to silent CipherKeyValue, the main advantage (enabling translations) is not relevant because there will be nothing to translate.

SLIP-11 is more flexible from Suite perspective, so I believe there is zero motivation from our side for SLIP-21 for "labeling".

Only if you are sure that we will switch to GetSecret some time in the future because of other reasons, we can consider to do it now to save the second migration.

matejcik commented 7 months ago

@andrewkozlik I like the array of parameters, but the enum for secret types seems too limiting. Instead we could do a "symbolic name" for the secret_type parameter, something like trezor::labeling for labeling, etc.

(Then we could grandfather in the old prompts, so that "Enable labeling?" becomes a well-defined secret type instead of a raw string. Of course that would only make sense to make it backwards-compatible with SLIP11, and it won't work for password manager)


GetPrivateKey seems like its own thing