mirror of
https://github.com/simplex-chat/simplex-chat.git
synced 2026-10-06 14:18:22 +00:00
fable review fixes
This commit is contained in:
@@ -31,7 +31,7 @@ master seed the only key material to back up
|
||||
└── m/44'/60'/n'/0/0 account n, n >= 0
|
||||
```
|
||||
|
||||
An account index is BIP-44's own account level, the level [Ledger Live](https://github.com/LedgerHQ/ledger-live-common/blob/HEAD/docs/derivation.md) varies and also calls an account, numbering from one where its Account 1 is index 0. So the master phrase imported into another wallet derives the same addresses, and the two tools use the word for the same thing. That wallet discovers account 0 and stops, because BIP-44 specifies that discovery stops at the first account with no transaction history, and this app leaves account 0 empty (known limit 10) and an account that only owns a name has none; above it the user has to enter the path in a wallet that accepts one, such as MEW, Rabby or Frame, since Ledger Live, MetaMask and Trezor Suite do not, which leaves the exported account key for those.
|
||||
An account index is BIP-44's own account level, which [Ledger Live](https://github.com/LedgerHQ/ledger-live-common/blob/HEAD/docs/derivation.md) also varies and calls an account, so the master phrase imported into another wallet derives the same addresses. Other wallets reach an account above 0 only by entering the path, in a wallet that accepts one such as MEW, Rabby or Frame, or with the exported account key (known limit 7).
|
||||
|
||||
The tests pin the derivation against the standard `abandon ... about` test mnemonic, which is 12 words, with an empty BIP-39 passphrase. Addresses are in [EIP-55](https://eips.ethereum.org/EIPS/eip-55) mixed case.
|
||||
|
||||
@@ -46,7 +46,7 @@ The alternative is one account owning several names. A name's owner address is p
|
||||
|
||||
### Why the account level is hardened
|
||||
|
||||
The alternative is BIP-44's ordinary address level, `m/44'/60'/0'/0/n`, which is what MetaMask enumerates and is therefore the friendlier path. It is not hardened, and [BIP-32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki) has a known weakness there: the extended public key of a parent, together with one non-hardened child's private key, yields the parent private key and from it every sibling. An exported account key is one half, and any wallet that enumerates accounts produces the other. The two levels below an account are not hardened, so the two halves together reveal the extended private key at the account level, m/44'/60'/n'; nothing else is derived under an account by this app and the account level itself is hardened, so they reveal no other account's key; known limit 10 covers account 0, under which other wallets do derive. This satisfies objectives 2 and 3, and it is worth the loss of MetaMask's default path.
|
||||
The alternative is BIP-44's ordinary address level, `m/44'/60'/0'/0/n`, which is what MetaMask enumerates and is therefore the friendlier path. It is not hardened, and [BIP-32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki) has a known weakness there: the extended public key of a parent, together with one non-hardened child's private key, yields the parent private key and from it every sibling. An exported account key is one half, and any wallet that enumerates accounts produces the other. The two levels below an account are not hardened, so the two halves together reveal the extended private key at the account level, m/44'/60'/n'; nothing else is derived under an account by this app and the account level itself is hardened, so they reveal no other account's key, except under account 0 where other wallets derive (known limit 7). This satisfies objectives 2 and 3, and it is worth the loss of MetaMask's default path.
|
||||
|
||||
### Why 24 words
|
||||
|
||||
@@ -85,11 +85,11 @@ A command that acts on a profile's accounts names the profile and is rejected wh
|
||||
|
||||
`bind` without `account=` binds the account at a counter on the master, which is a high-water mark and not a count of bound accounts, and returns the bound account's index, path and address. With `account=<n>` it binds that account, which is how an account found by a scan is attached to the profile it belongs to, and it is rejected for an account another profile holds. After an import the counter is unknown rather than zero, because the phrase does not encode how many accounts it has been used for, so binding the next account is rejected until a scan sets the counter, while binding a known account is still allowed.
|
||||
|
||||
BIP-32 marks an index as hardened by setting its top bit, so an index at or above 2^31 already has that bit set and derives the same key as the index 2^31 below it: account 2^31 is account 0. That is a collision, not a loss of hardening, and it would put one key under two account indexes. For the one key in 2^128 that BIP-32 declares invalid, simplexmq recomputes it as SLIP-0010 specifies and Trezor implements, instead of skipping the index, up to three times; a third failure is reported as an internal error, not a wallet condition, since no input is expected to reach it. An account index is a simplexmq type that only holds values below 2^31, so the command parser rejects 2^31 and above as a bad command, the columns have CHECK constraints for that bound, and reading a row that violates it is an error. The counter's bound is one higher than an account's, because it contains the next index to bind, and 2^31 there means the counter has passed every index that can be hardened, which `bind` without an index reports.
|
||||
BIP-32 marks an index as hardened by setting its top bit, so account 2^31 would derive account 0's key: a collision, not a loss of hardening. An account index is therefore a simplexmq type holding values below 2^31; the parser rejects larger values as a bad command, the columns have CHECK constraints for the bound, and a row outside it is an error. The counter's bound is one higher, because it holds the next index to bind; at 2^31 it has passed every index, which `bind` without an index reports. For the one key in 2^128 that BIP-32 declares invalid, simplexmq recomputes it as SLIP-0010 specifies, up to three times; a third failure is an internal error, since no input is expected to reach it.
|
||||
|
||||
`address` reads the counter without changing it, so two calls return the same address, and it derives an address for an account the database has no row for, which a device that lost its database requires. One address per call is sufficient: a caller that scans the tree calls it in a loop.
|
||||
|
||||
`export account` is rejected unless the profile holds the account, so every exported key belongs to an account that is already bound, which `bind` without an index does not return, because the counter is past it or unknown; the exception is a database restored from a backup (known limit 9). An export is a copy and not a transfer: the device still derives what it exported and can still sign with it, so giving an account key away leaves two parties able to act as its owner until whatever it holds is transferred on chain. Signing is not in this change, and when it is added it is a command here that signs and returns a signature, not `export account` followed by signing elsewhere, which would make the narrow export the ordinary path. `delete` leaves whatever the accounts own on chain, recoverable only from the phrase.
|
||||
`export account` is rejected unless the profile holds the account, so every exported key belongs to a bound account, which `bind` without an index does not return again, except on a database restored from a backup (known limit 9). An export is a copy, not a transfer: the device still derives the key, so two parties can act as the owner until whatever the account holds is transferred on chain. Signing is not in this change; when it is added it is a command here that returns a signature, not `export account` followed by signing elsewhere. `delete` leaves whatever the accounts own on chain, recoverable only from the phrase.
|
||||
|
||||
```haskell
|
||||
data WalletAddress = WalletAddress {accountIndex :: AccountIndex, keyPath :: Text, address :: Address}
|
||||
@@ -131,7 +131,7 @@ CREATE TABLE wallet_seeds (
|
||||
CREATE TABLE wallet_accounts (
|
||||
wallet_account_id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
wallet_seed_id INTEGER NOT NULL REFERENCES wallet_seeds ON DELETE CASCADE,
|
||||
account_index INTEGER CHECK (account_index BETWEEN 0 AND 2147483647), -- null when the key was imported
|
||||
account_index INTEGER NOT NULL CHECK (account_index BETWEEN 0 AND 2147483647),
|
||||
user_id INTEGER REFERENCES users ON DELETE SET NULL
|
||||
) STRICT;
|
||||
|
||||
@@ -144,12 +144,9 @@ The master entropy, 32 bytes when generated and 16 to 32 bytes when imported, is
|
||||
|
||||
`users` is not changed: the mapping is stored in the account row, and the index on `user_id` is not unique, because a profile can hold any number of accounts. One seed per device is enforced by `single_seed` and the unique index on it, which a later change removes with a `DROP INDEX` and a `DROP COLUMN`; it is a named index rather than an inline `UNIQUE` because SQLite cannot drop an inline constraint without rebuilding the table. Deleting the master deletes its account rows, because an account index without its entropy derives nothing. The down migration drops both tables, which deletes the master entropy; it runs only when the user confirms "Downgrade and open chat" in an older app.
|
||||
|
||||
A null `account_index` marks an account whose key was imported rather than derived. The master phrase does not recover such an account, and the schema must not suggest that it does. Importing one is not implemented here; the column is nullable now so that a row written later is read correctly. That feature also requires storage for the imported secret and an optional link to a seed, because such an account belongs to no seed and must not be deleted with one; on SQLite, making `wallet_seed_id` nullable rebuilds the table.
|
||||
|
||||
## Threat model
|
||||
|
||||
- **Someone with the database file.** Gets everything, now and later: the stored entropy is the master phrase in another encoding, so an archive exported to move devices contains every key on the device. No export granularity protects against a file copy. On SQLite the connection sets `secure_delete`, so a deleted row's pages are zeroed; the journal and any copy already made are not.
|
||||
- **Someone with one account key.** Can act as that account's owner permanently, because an export is a copy and the device keeps deriving the same key. Cannot derive another account's key.
|
||||
- **A wallet the master phrase is imported into.** Enumerating BIP-44 accounts computes account extended public keys, and some wallets send them to a vendor, which gives that vendor every account on the device at once, across every profile. That is what an account for each name otherwise prevents.
|
||||
- **Whoever answers the recovery scan.** Receives every address the scan derives from the phrase, in one sequence of requests, so it can link every account on the device, across profiles, and recognise addresses that own nothing yet, which is where future accounts will be. `address` derives an address for any index directly from the master, so a caller can enumerate hidden profiles' addresses too. This is the largest privacy cost of the design.
|
||||
- **A paired device.** Wallet commands are allowed from a paired device like other chat commands: `export master` returns the whole wallet, `create mnemonic=` on a device that has no seed imports a phrase that the paired device sends, and `delete` deletes the master entropy, which may have no other copy.
|
||||
@@ -164,10 +161,9 @@ A null `account_index` marks an account whose key was imported rather than deriv
|
||||
4. **The same phrase on two devices collides.** The counter is stored in one database, so both devices bind the same account and each treats it as free. Sharing the counter requires a backup both devices can read.
|
||||
5. **A profile hidden after an account was bound to it keeps its accounts.** The check runs at bind time only.
|
||||
6. **Account indexes are not dense.** An account can be bound and never used, and a run of empty accounts ends the scan, so an account after a gap can be missed.
|
||||
7. **Other wallets do not discover the accounts.** A generated wallet leaves account 0 empty, so BIP-44 discovery in Ledger Live or Trezor Suite stops before the first account this app uses, whether or not later accounts hold funds or paid gas; they are reached only by entering the path or with the exported key. If an account ever pays for anything, whatever funds it links accounts on chain.
|
||||
7. **Account 0 is left to other wallets.** MetaMask, Ledger Live and Trezor Suite present `m/44'/60'/0'/0/0` first and hand a browser the extended public key of its address level, so with a phrase used in one of them the export of account 0 exposes that wallet's accounts. A generated wallet therefore starts its counter at 1, and account 0 is bound or exported only when asked for with `account=0`. In turn, BIP-44 discovery in those wallets stops at the empty account 0, so they reach this app's accounts only by path or exported key. If an account ever pays for anything, whatever funds it links accounts on chain.
|
||||
8. **Nothing records which layout a seed was used with.** Another wallet may have derived accounts from the phrase at paths this doc does not describe.
|
||||
9. **`bind` on a restored database can return an account that is already in use.** Its counter can be below the highest index used, and nothing detects that, so a name can be bought with an account that already owns one until the scan resets the counter.
|
||||
10. **Account 0 is the primary account of other wallets.** MetaMask, Ledger Live and Trezor Suite present `m/44'/60'/0'/0/0` first, and their hardware integrations hand a browser the extended public key of its address level, so with a phrase used in one of them the export of account 0 exposes that wallet's accounts. A generated wallet therefore starts its counter at 1, and account 0 is bound or exported only when asked for with `account=0`.
|
||||
|
||||
## Main files
|
||||
|
||||
|
||||
Reference in New Issue
Block a user