[BC/CWC] Wallet private key updates [1]

[MichaelAJay]

bitcore-client

Encryption

• adds encryptBuffer - enables normalized buffer input
• adds decryptToBuffer - enables improved security by not decrypting directly to string

Storage

• adds addKeysSafe - incoming keys' private key is encrypted, so not immediately available to be used to retrieve pubkey, which should be available anyway. Throws if missing a pubkey

Wallet

• Adds version: 2 & version checks for backwards compatibility (note: to create an old Wallet would require version: 0 - so 1 is skipped - looking for feedback on numbering/versioning).
• create encrypts HDPrivateKey xprivkey and privateKey so they can be decrypted as buffers instead of serializing the whole masterKey and THEN encrypting
• importKeys does the same for signing keys
• signTx decrypts to buffers then uses deriver for chain-aware decoding. In the next phase, this decoding should be made unnecessary by passing the buffer all the way through to use, if possible

Tests

• update prior tests to include versioning
• introduce new tests for version 2 specifics

crypto-wallet-core

Derivation

• Add two passthrough methods on DeriverProxy to buffer & serialize private keys
• Adds privateKeyToBuffer and privateKeyBufferToNativePrivateKey to IDeriver & implement for each Deriver - no need to override for any of the extending classes (UTXOs to AbstractBitcoreLibDeriver & EVM to EthDeriver)

[kajoseph]

I think a better approach may be to auto migrate wallets with version < 2 to v2. That way we can avoid carve-outs while actively push better security.

loadWallet() {
  const wallet = read wallet file;
  if (wallet.version < 2) {
    convert to v2 wallet;
    backup wallet file to .bak;
    overwrite wallet file with v2;
  }
}

[kajoseph]

Include the decipher.final() in the try.

try {
  return Buffer.concat([decrypted, decipher.final()]);

[kajoseph]

include decipher.final() in the try.

let final;
try {
  final = decipher.final();
  const output = Buffer.concat([payload, final]);
  return toBuffer ? output : output.toString('hex');
} finally {
  payload.fill(0);
  final?.fill(0);
}

Not sure if defining final outside the try is necessary. If this code is going to error, it's highly likely that it's due to an invalid checksum/tag in the decipher.final() call, so it may be fine to just do Buffer.concat([payload, decipher.final()]);. Whatever is decided also applies to the other encrypt/decrypt buffer methods.

[kajoseph]

Functionally, this seems to work well. Nice job!

In addition to the minor comments below, we should add test cases for the migration as well as tests for critical functions with the new version wallets (i.e. deriving, sending, sign message)

[kajoseph]

We should log that a migration is taking place. E.g. "Migrating wallet from version x to version y"

[kajoseph]

Should write to an expected directory instead of just dumping in the cwd. e.g. ~/.bitcore/bitcoreWallets/backup/ (next to where the text wallet files are saved)

[kajoseph]

Log migration successful

[kajoseph]

Log that a pre-migration wallet backup is being written to <file>

[kajoseph]

There seems to be an issue with maintaining the addressIndex through the migration. To reproduce:

1. Pre-migration (i.e. on master), create a BTC wallet
2. Generate some addresses bin/wallet derive --name <name> --gap 1
   • Run that a few times or change gap to the number of addresses you want to create
3. Running bin/wallet check --name <name> will show your latest address
4. Switch to this branch
5. Run the check command again and it'll tell you to derive an address
6. Run the derive command again and it'll start with m/0/0

Note, that bin/wallet balance --name <name> will still show the correct balance and send still works. It just seems like the addressIndex gets unset.