Add proper header to storage (to expose some data before decryption)

[SomberNight]

Currently, encrypted wallet files don't really have a header. The file contents are: https://github.com/spesmilo/electrum/blob/ce81957d25e66296c0baaf48683adcc623a3dae4/electrum/ecc.py#L503-L506 So it's just a 4 byte magic, and data needed for the decryption itself.

The magic itself is used to distinguish between two wallet types. BIE1 wallets are encrypted with a user provided password ("userpw"), and BIE2 wallets are encrypted with a pubkey used as password that comes from a hardware wallet (along a hardcoded bip32 derivation path) ("xpubpw"). https://github.com/spesmilo/electrum/blob/ce81957d25e66296c0baaf48683adcc623a3dae4/electrum/storage.py#L171-L176

It would be useful to have a proper header. For example, the header could include

• for xpubpw, the derivation path (related: https://github.com/spesmilo/electrum/pull/5993)
• a random salt, to go in the hmac with the password (useful for both userpw and xpubpw)
• maybe fancy things such as hmac iteration count, or even hmac algorithm
• IIRC some metadata would need to be stored to allow partial writes (https://github.com/spesmilo/electrum/issues/4823)

[SomberNight]

Note: if we make an incompatible change to the storage file format, we should also change the KDF to something stronger. Maybe scrypt (and introduce a random salt at the same time).

[SomberNight]

somewhat related: https://github.com/spesmilo/electrum/issues/5561

[SomberNight]

@ln2max take a look

[ln2max]

See my comment here: https://github.com/spesmilo/electrum/issues/4823#issuecomment-919348130

[ecdsa]

To allow password-encryption for hardware wallets (#5561), we need to have multiple ways to decrypt the same wallet. We can encrypt the wallet with a random "master key", and add several fields, each containing master key, encrypted with a different passwords. This also allows us to change the password without rewriting the entire file.

Regarding #5993, I am not sure why the derivation path should be part of the header. Since the derivation path depends on the hardware wallet, I believe it should be defined in the corresponding plugin.

Since the master key we are encrypting is random, no rainbow table can be built. Thus, I believe a salt is unnecessary.

The header would consist of:

• [8 bytes] magic: b"Electrum"
• [1 byte] version of the header
• [1 byte] flags (a flag is needed to tell whether blobs are zipped before encryption)
• [1 byte] N, number of encryption passwords
• [N*18 bytes] encrypted master key and flags:
  • [1 byte] flags (KDF algo, distinguish hw xpub from user-entered password)
  • [1 byte] number of rounds of KDF (log2, use power of 2)
  • [16 bytes] encrypt(master_key, password_1) ... encrypt(master_key, password_N)
• [16 bytes] hmac(master_key), to be able to test a password without decrypting the entire file.

Note: I think we can drop ECIES, and simply use symmetric encryption. Forward secrecy is not really useful here, because what we need to protect is the wallet file content, which contains seed/privkeys.

[ln2max]

On Thu, Sep 30, 2021 at 06:53:08AM -0700, ThomasV wrote:

The header would consist of:

• Magic: b"Electrum"
• Version (1 byte)

If I understand you correctly, the idea is to use the version number to provide for future changes to the header format, rather than trying to design a modular header format now?

[ecdsa]

If I understand you correctly, the idea is to use the version number to provide for future changes to the header format, rather than trying to design a modular header format now?

yes

[SomberNight]

Regarding #5993, I am not sure why the derivation path should be part of the header. Since the derivation path depends on the hardware wallet, I believe it should be defined in the corresponding plugin.

Currently all hardware devices use the same hardcoded bip32 path (m/4541509h/1112098098h) for deriving a pubkey used as encryption password.

The discussion in the linked thread was about the bitbox02 restricting get xpub requests to certain specific paths, so we had not been able to request the pubkey for the hardcoded path (m/4541509h/1112098098h) -- the resolution there was the bitbox02 adding an exception in the firmware for just this specific path we wanted. So atm all hardware wallets behave the same.

The current approach allows for encrypting a wallet file with a Trezor and decrypting it with a Ledger (although functionality is limited in that case, e.g. the wallet itself will not be able to sign). So even if you lose your device but have a different one (initialised with the same seed) you can easily decrypt the wallet file and recover your labels (for instance). Similar to it being encrypted with a user-password: just because you lost your device, if you have your secrets, ideally you should be able to recover information from the file.

I don't like the approach of plugins customising this derivation path directly and then not exposing what path they use outside of the plugin itself. Imagine the wizard pairs with a hw device and tries to decrypt a wallet file, deriving the password from a key along a bip32 path that is simply hardcoded in the plugin code - i.e. specific to that manufacturer and plugin. Critically, this would mean we might not be able to delete/remove a plugin - otherwise some existing wallet files could not be decrypted anymore. It is fine not being able to use a wallet file after the corresponding plugin has been deleted, but the file should be decryptable if possible, so watch-only information is still available (e.g. labels can be recovered).

This, along with the bitbox02 originally not supporting our hardcoded path, is the reason I had suggested that we could put the derivation path into the file header. That would mean different hww plugins/manufacturers can use different paths, which would then be encoded in a generic hww-agnostic way in the file header, making the file itself decryptable in a hww-agnostic way.

[ln2max]

On Mon, Oct 04, 2021 at 08:15:19AM -0700, ghost43 wrote:

This, along with the bitbox02 originally not supporting our hardcoded path, is the reason I had suggested that we could put the derivation path into the file header. That would mean different hww plugins/manufacturers can use different paths, which would then be encoded in a generic hww-agnostic way in the file header, making the file itself decryptable in a hww-agnostic way.

This makes sense. Unfortunately I don't see any good way to put a variable-length string into a binary header. The JSON-based approach would be much better for that kind of thing.

[SomberNight]

note: we should also add a header to unencrypted files. Even if only to contain a magic prefix. This would finally solve https://github.com/spesmilo/electrum/issues/6349#issuecomment-754158457 - those old software should not be able to open new wallet files as they just corrupt them.