From 3db3dc946f1ee6249394186216a89041ab979617 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 21 Mar 2023 14:28:15 +1030 Subject: [PATCH] lightningd: move bip32_pubkey here from common/, add hsm check. At the moment only lightingd needs it, and this avoids missing any places where we do bip32 derivation. This uses a hsm capability to mean we're backwards compatible with older hsmds. Signed-off-by: Rusty Russell Changelog-Added: Protocol: we now always double-check bitcoin addresses are correct (no memory errors!) before issuing them. --- common/key_derive.c | 19 ------- common/key_derive.h | 5 -- lightningd/hsm_control.c | 32 ++++++++++++ lightningd/hsm_control.h | 4 ++ lightningd/lightningd.c | 1 + lightningd/onchain_control.c | 8 +-- lightningd/peer_control.c | 4 +- lightningd/test/run-invoice-select-inchan.c | 4 ++ wallet/db.c | 4 +- wallet/reservation.c | 10 ++-- wallet/test/run-db.c | 4 ++ wallet/test/run-wallet.c | 55 +++++++++++++++++++++ wallet/walletrpc.c | 12 ++--- 13 files changed, 113 insertions(+), 49 deletions(-) diff --git a/common/key_derive.c b/common/key_derive.c index bdf87d99382c..52c78ac5e9ad 100644 --- a/common/key_derive.c +++ b/common/key_derive.c @@ -248,22 +248,3 @@ bool derive_revocation_privkey(const struct secret *base_secret, #endif return true; } - - -bool bip32_pubkey(const struct ext_key *bip32_base, - struct pubkey *pubkey, u32 index) -{ - const uint32_t flags = BIP32_FLAG_KEY_PUBLIC | BIP32_FLAG_SKIP_HASH; - struct ext_key ext; - - if (index >= BIP32_INITIAL_HARDENED_CHILD) - return false; - - if (bip32_key_from_parent(bip32_base, index, flags, &ext) != WALLY_OK) - return false; - - if (!secp256k1_ec_pubkey_parse(secp256k1_ctx, &pubkey->pubkey, - ext.pub_key, sizeof(ext.pub_key))) - return false; - return true; -} diff --git a/common/key_derive.h b/common/key_derive.h index 59be4794a7f7..b078cc9261e0 100644 --- a/common/key_derive.h +++ b/common/key_derive.h @@ -28,9 +28,4 @@ bool derive_revocation_privkey(const struct secret *base_secret, const struct pubkey *basepoint, const struct pubkey *per_commitment_point, struct privkey *key); - - -struct ext_key; -bool bip32_pubkey(const struct ext_key *bip32_base, - struct pubkey *pubkey, u32 index); #endif /* LIGHTNING_COMMON_KEY_DERIVE_H */ diff --git a/lightningd/hsm_control.c b/lightningd/hsm_control.c index d35dc94530ae..0eb281f97593 100644 --- a/lightningd/hsm_control.c +++ b/lightningd/hsm_control.c @@ -177,6 +177,38 @@ struct ext_key *hsm_init(struct lightningd *ld) return bip32_base; } +/*~ There was a nasty LND bug report where the user issued an address which it + * couldn't spend, presumably due to a bitflip. We check every address using our + * hsm, to be sure it's valid. Expensive, but not as expensive as losing BTC! */ +void bip32_pubkey(struct lightningd *ld, struct pubkey *pubkey, u32 index) +{ + const uint32_t flags = BIP32_FLAG_KEY_PUBLIC | BIP32_FLAG_SKIP_HASH; + struct ext_key ext; + + if (index >= BIP32_INITIAL_HARDENED_CHILD) + fatal("Can't derive keu %u (too large!)", index); + + if (bip32_key_from_parent(ld->bip32_base, index, flags, &ext) != WALLY_OK) + fatal("Can't derive key %u", index); + + if (!secp256k1_ec_pubkey_parse(secp256k1_ctx, &pubkey->pubkey, + ext.pub_key, sizeof(ext.pub_key))) + fatal("Can't parse derived key %u", index); + + /* Don't assume hsmd supports it! */ + if (hsm_capable(ld, WIRE_HSMD_CHECK_PUBKEY)) { + bool ok; + u8 *msg = towire_hsmd_check_pubkey(NULL, index, pubkey); + wire_sync_write(ld->hsm_fd, take(msg)); + msg = wire_sync_read(tmpctx, ld->hsm_fd); + if (!fromwire_hsmd_check_pubkey_reply(msg, &ok)) + fatal("Invalid check_pubkey_reply from hsm"); + if (!ok) + fatal("HSM said key derivation of %u != %s", + index, type_to_string(tmpctx, struct pubkey, pubkey)); + } +} + static struct command_result *json_makesecret(struct command *cmd, const char *buffer, const jsmntok_t *obj UNNEEDED, diff --git a/lightningd/hsm_control.h b/lightningd/hsm_control.h index ffef42a81524..6fc222fe646f 100644 --- a/lightningd/hsm_control.h +++ b/lightningd/hsm_control.h @@ -21,4 +21,8 @@ int hsm_get_global_fd(struct lightningd *ld, int capabilities); bool hsm_capable(struct lightningd *ld, u32 msgtype); struct ext_key *hsm_init(struct lightningd *ld); + +/* Get (and check!) a bip32 derived pubkey */ +void bip32_pubkey(struct lightningd *ld, struct pubkey *pubkey, u32 index); + #endif /* LIGHTNING_LIGHTNINGD_HSM_CONTROL_H */ diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 822ba04529a1..ae8f8afc0426 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -59,6 +59,7 @@ #include #include #include +#include #include #include #include diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index c2417f887d93..c243b47358bc 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -662,12 +662,8 @@ enum watch_result onchaind_funding_spent(struct channel *channel, return KEEP_WATCHING; } - if (!bip32_pubkey(ld->bip32_base, &final_key, - channel->final_key_idx)) { - log_broken(channel->log, "Could not derive onchain key %"PRIu64, - channel->final_key_idx); - return KEEP_WATCHING; - } + bip32_pubkey(ld, &final_key, channel->final_key_idx); + struct ext_key final_wallet_ext_key; if (bip32_key_from_parent( ld->bip32_base, diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index bbee7dc6febb..7ab13ba3f9c3 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -214,9 +214,7 @@ u8 *p2wpkh_for_keyidx(const tal_t *ctx, struct lightningd *ld, u64 keyidx) { struct pubkey shutdownkey; - if (!bip32_pubkey(ld->bip32_base, &shutdownkey, keyidx)) - return NULL; - + bip32_pubkey(ld, &shutdownkey, keyidx); return scriptpubkey_p2wpkh(ctx, &shutdownkey); } diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 58a8715bb478..e830e40f56b4 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -13,6 +13,10 @@ struct channel *any_channel_by_scid(struct lightningd *ld UNNEEDED, const struct short_channel_id *scid UNNEEDED, bool privacy_leak_ok UNNEEDED) { fprintf(stderr, "any_channel_by_scid called!\n"); abort(); } +/* Generated stub for bip32_pubkey */ +void bip32_pubkey(struct lightningd *ld UNNEEDED, + struct pubkey *pubkey UNNEEDED, u32 index UNNEEDED) +{ fprintf(stderr, "bip32_pubkey called!\n"); abort(); } /* Generated stub for bitcoind_getutxout_ */ void bitcoind_getutxout_(struct bitcoind *bitcoind UNNEEDED, const struct bitcoin_outpoint *outpoint UNNEEDED, diff --git a/wallet/db.c b/wallet/db.c index ea2e2d0672db..1e903f714d75 100644 --- a/wallet/db.c +++ b/wallet/db.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -1147,8 +1148,7 @@ void fillin_missing_scriptpubkeys(struct lightningd *ld, struct db *db, } else { db_col_ignore(stmt, "peer_id"); db_col_ignore(stmt, "commitment_point"); - /* Build from bip32_base */ - bip32_pubkey(mc->bip32_base, &key, keyindex); + bip32_pubkey(ld, &key, keyindex); if (type == p2sh_wpkh) { u8 *redeemscript = bitcoin_redeem_p2sh_p2wpkh(stmt, &key); scriptPubkey = scriptpubkey_p2sh(tmpctx, redeemscript); diff --git a/wallet/reservation.c b/wallet/reservation.c index 109cf9ccc8de..da1e27454b26 100644 --- a/wallet/reservation.c +++ b/wallet/reservation.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -246,7 +247,6 @@ static bool inputs_sufficient(struct amount_sat input, static struct wally_psbt *psbt_using_utxos(const tal_t *ctx, struct wallet *wallet, struct utxo **utxos, - const struct ext_key *bip32_base, u32 nlocktime, u32 nsequence) { @@ -261,7 +261,7 @@ static struct wally_psbt *psbt_using_utxos(const tal_t *ctx, struct bitcoin_tx *tx; if (utxos[i]->is_p2sh) { - bip32_pubkey(bip32_base, &key, utxos[i]->keyindex); + bip32_pubkey(wallet->ld, &key, utxos[i]->keyindex); scriptSig = bitcoin_scriptsig_p2sh_p2wpkh(tmpctx, &key); redeemscript = bitcoin_redeem_p2sh_p2wpkh(tmpctx, &key); scriptPubkey = scriptpubkey_p2sh(tmpctx, redeemscript); @@ -357,7 +357,6 @@ static struct command_result *finish_psbt(struct command *cmd, } psbt = psbt_using_utxos(cmd, cmd->ld->wallet, utxos, - cmd->ld->bip32_base, *locktime, BITCOIN_TX_RBF_SEQUENCE); /* Should we add a change output for the excess? */ @@ -381,10 +380,7 @@ static struct command_result *finish_psbt(struct command *cmd, "Failed to generate change address." " Keys exhausted."); - if (!bip32_pubkey(cmd->ld->bip32_base, &pubkey, keyidx)) - return command_fail(cmd, LIGHTNINGD, - "Failed to generate change address." - " Keys generation failure"); + bip32_pubkey(cmd->ld, &pubkey, keyidx); b32script = scriptpubkey_p2wpkh(tmpctx, &pubkey); txfilter_add_scriptpubkey(cmd->ld->owned_txfilter, b32script); diff --git a/wallet/test/run-db.c b/wallet/test/run-db.c index 5059d7ae1923..e6c0987e9805 100644 --- a/wallet/test/run-db.c +++ b/wallet/test/run-db.c @@ -20,6 +20,10 @@ static void db_log_(struct log *log UNUSED, enum log_level level UNUSED, const s #include /* AUTOGENERATED MOCKS START */ +/* Generated stub for bip32_pubkey */ +void bip32_pubkey(struct lightningd *ld UNNEEDED, + struct pubkey *pubkey UNNEEDED, u32 index UNNEEDED) +{ fprintf(stderr, "bip32_pubkey called!\n"); abort(); } /* Generated stub for derive_channel_id */ void derive_channel_id(struct channel_id *channel_id UNNEEDED, const struct bitcoin_outpoint *outpoint UNNEEDED) diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 89a7da9873c5..eb658d50c4f5 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -30,6 +30,7 @@ void db_fatal(const char *fmt, ...) #endif /* DB_FATAL */ #include "wallet/wallet.c" +#include "lightningd/hsm_control.c" #include "lightningd/htlc_end.c" #include "lightningd/peer_control.c" #include "lightningd/peer_htlcs.c" @@ -170,15 +171,33 @@ bool fromwire_connectd_peer_spoke(const void *p UNNEEDED, struct node_id *id UNN /* Generated stub for fromwire_dualopend_dev_memleak_reply */ bool fromwire_dualopend_dev_memleak_reply(const void *p UNNEEDED, bool *leak UNNEEDED) { fprintf(stderr, "fromwire_dualopend_dev_memleak_reply called!\n"); abort(); } +/* Generated stub for fromwire_hsmd_check_pubkey_reply */ +bool fromwire_hsmd_check_pubkey_reply(const void *p UNNEEDED, bool *ok UNNEEDED) +{ fprintf(stderr, "fromwire_hsmd_check_pubkey_reply called!\n"); abort(); } +/* Generated stub for fromwire_hsmd_client_hsmfd_reply */ +bool fromwire_hsmd_client_hsmfd_reply(const void *p UNNEEDED) +{ fprintf(stderr, "fromwire_hsmd_client_hsmfd_reply called!\n"); abort(); } +/* Generated stub for fromwire_hsmd_derive_secret_reply */ +bool fromwire_hsmd_derive_secret_reply(const void *p UNNEEDED, struct secret *secret UNNEEDED) +{ fprintf(stderr, "fromwire_hsmd_derive_secret_reply called!\n"); abort(); } /* Generated stub for fromwire_hsmd_get_output_scriptpubkey_reply */ bool fromwire_hsmd_get_output_scriptpubkey_reply(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u8 **script UNNEEDED) { fprintf(stderr, "fromwire_hsmd_get_output_scriptpubkey_reply called!\n"); abort(); } +/* Generated stub for fromwire_hsmd_init_reply_v2 */ +bool fromwire_hsmd_init_reply_v2(const void *p UNNEEDED, struct node_id *node_id UNNEEDED, struct ext_key *bip32 UNNEEDED, struct pubkey *bolt12 UNNEEDED) +{ fprintf(stderr, "fromwire_hsmd_init_reply_v2 called!\n"); abort(); } +/* Generated stub for fromwire_hsmd_init_reply_v4 */ +bool fromwire_hsmd_init_reply_v4(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u32 *hsm_version UNNEEDED, u32 **hsm_capabilities UNNEEDED, struct node_id *node_id UNNEEDED, struct ext_key *bip32 UNNEEDED, struct pubkey *bolt12 UNNEEDED) +{ fprintf(stderr, "fromwire_hsmd_init_reply_v4 called!\n"); abort(); } /* Generated stub for fromwire_hsmd_new_channel_reply */ bool fromwire_hsmd_new_channel_reply(const void *p UNNEEDED) { fprintf(stderr, "fromwire_hsmd_new_channel_reply called!\n"); abort(); } /* Generated stub for fromwire_hsmd_sign_commitment_tx_reply */ bool fromwire_hsmd_sign_commitment_tx_reply(const void *p UNNEEDED, struct bitcoin_signature *sig UNNEEDED) { fprintf(stderr, "fromwire_hsmd_sign_commitment_tx_reply called!\n"); abort(); } +/* Generated stub for fromwire_hsmstatus_client_bad_request */ +bool fromwire_hsmstatus_client_bad_request(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct node_id *id UNNEEDED, wirestring **description UNNEEDED, u8 **msg UNNEEDED) +{ fprintf(stderr, "fromwire_hsmstatus_client_bad_request called!\n"); abort(); } /* Generated stub for fromwire_onchaind_dev_memleak_reply */ bool fromwire_onchaind_dev_memleak_reply(const void *p UNNEEDED, bool *leak UNNEEDED) { fprintf(stderr, "fromwire_onchaind_dev_memleak_reply called!\n"); abort(); } @@ -191,6 +210,9 @@ u32 get_block_height(const struct chain_topology *topo UNNEEDED) /* Generated stub for get_channel_update */ const u8 *get_channel_update(struct channel *channel UNNEEDED) { fprintf(stderr, "get_channel_update called!\n"); abort(); } +/* Generated stub for hsmd_wire_name */ +const char *hsmd_wire_name(int e UNNEEDED) +{ fprintf(stderr, "hsmd_wire_name called!\n"); abort(); } /* Generated stub for htlc_is_trimmed */ bool htlc_is_trimmed(enum side htlc_owner UNNEEDED, struct amount_msat htlc_amount UNNEEDED, @@ -283,6 +305,9 @@ void invoices_waitone(const tal_t *ctx UNNEEDED, void (*cb)(const struct invoice * UNNEEDED, void*) UNNEEDED, void *cbarg UNNEEDED) { fprintf(stderr, "invoices_waitone called!\n"); abort(); } +/* Generated stub for is_hsm_secret_encrypted */ +int is_hsm_secret_encrypted(const char *path UNNEEDED) +{ fprintf(stderr, "is_hsm_secret_encrypted called!\n"); abort(); } /* Generated stub for json_add_address */ void json_add_address(struct json_stream *response UNNEEDED, const char *fieldname UNNEEDED, const struct wireaddr *addr UNNEEDED) @@ -488,6 +513,14 @@ struct chain_coin_mvt *new_coin_wallet_deposit(const tal_t *ctx UNNEEDED, enum mvt_tag tag) { fprintf(stderr, "new_coin_wallet_deposit called!\n"); abort(); } +/* Generated stub for new_global_subd */ +struct subd *new_global_subd(struct lightningd *ld UNNEEDED, + const char *name UNNEEDED, + const char *(*msgname)(int msgtype) UNNEEDED, + unsigned int (*msgcb)(struct subd * UNNEEDED, const u8 * UNNEEDED, + const int *fds) UNNEEDED, + ...) +{ fprintf(stderr, "new_global_subd called!\n"); abort(); } /* Generated stub for new_peer_fd */ struct peer_fd *new_peer_fd(const tal_t *ctx UNNEEDED, int peer_fd UNNEEDED) { fprintf(stderr, "new_peer_fd called!\n"); abort(); } @@ -574,6 +607,11 @@ void outpointfilter_remove(struct outpointfilter *of UNNEEDED, bool param(struct command *cmd UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t params[] UNNEEDED, ...) { fprintf(stderr, "param called!\n"); abort(); } +/* Generated stub for param_bin_from_hex */ +struct command_result *param_bin_from_hex(struct command *cmd UNNEEDED, const char *name UNNEEDED, + const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, + u8 **bin UNNEEDED) +{ fprintf(stderr, "param_bin_from_hex called!\n"); abort(); } /* Generated stub for param_bool */ struct command_result *param_bool(struct command *cmd UNNEEDED, const char *name UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, @@ -617,6 +655,11 @@ struct command_result *param_short_channel_id(struct command *cmd UNNEEDED, const jsmntok_t *tok UNNEEDED, struct short_channel_id **scid UNNEEDED) { fprintf(stderr, "param_short_channel_id called!\n"); abort(); } +/* Generated stub for param_string */ +struct command_result *param_string(struct command *cmd UNNEEDED, const char *name UNNEEDED, + const char * buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, + const char **str UNNEEDED) +{ fprintf(stderr, "param_string called!\n"); abort(); } /* Generated stub for param_u64 */ struct command_result *param_u64(struct command *cmd UNNEEDED, const char *name UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, @@ -786,9 +829,21 @@ u8 *towire_final_incorrect_htlc_amount(const tal_t *ctx UNNEEDED, struct amount_ /* Generated stub for towire_gossipd_discovered_ip */ u8 *towire_gossipd_discovered_ip(const tal_t *ctx UNNEEDED, const struct wireaddr *discovered_ip UNNEEDED) { fprintf(stderr, "towire_gossipd_discovered_ip called!\n"); abort(); } +/* Generated stub for towire_hsmd_check_pubkey */ +u8 *towire_hsmd_check_pubkey(const tal_t *ctx UNNEEDED, u32 index UNNEEDED, const struct pubkey *pubkey UNNEEDED) +{ fprintf(stderr, "towire_hsmd_check_pubkey called!\n"); abort(); } +/* Generated stub for towire_hsmd_client_hsmfd */ +u8 *towire_hsmd_client_hsmfd(const tal_t *ctx UNNEEDED, const struct node_id *id UNNEEDED, u64 dbid UNNEEDED, u64 capabilities UNNEEDED) +{ fprintf(stderr, "towire_hsmd_client_hsmfd called!\n"); abort(); } +/* Generated stub for towire_hsmd_derive_secret */ +u8 *towire_hsmd_derive_secret(const tal_t *ctx UNNEEDED, const u8 *info UNNEEDED) +{ fprintf(stderr, "towire_hsmd_derive_secret called!\n"); abort(); } /* Generated stub for towire_hsmd_get_output_scriptpubkey */ u8 *towire_hsmd_get_output_scriptpubkey(const tal_t *ctx UNNEEDED, u64 channel_id UNNEEDED, const struct node_id *peer_id UNNEEDED, const struct pubkey *commitment_point UNNEEDED) { fprintf(stderr, "towire_hsmd_get_output_scriptpubkey called!\n"); abort(); } +/* Generated stub for towire_hsmd_init */ +u8 *towire_hsmd_init(const tal_t *ctx UNNEEDED, const struct bip32_key_version *bip32_key_version UNNEEDED, const struct chainparams *chainparams UNNEEDED, const struct secret *hsm_encryption_key UNNEEDED, const struct privkey *dev_force_privkey UNNEEDED, const struct secret *dev_force_bip32_seed UNNEEDED, const struct secrets *dev_force_channel_secrets UNNEEDED, const struct sha256 *dev_force_channel_secrets_shaseed UNNEEDED, u32 hsm_wire_min_version UNNEEDED, u32 hsm_wire_max_version UNNEEDED) +{ fprintf(stderr, "towire_hsmd_init called!\n"); abort(); } /* Generated stub for towire_hsmd_new_channel */ u8 *towire_hsmd_new_channel(const tal_t *ctx UNNEEDED, const struct node_id *id UNNEEDED, u64 dbid UNNEEDED) { fprintf(stderr, "towire_hsmd_new_channel called!\n"); abort(); } diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index f50f2579f20a..aa3e2ea7e587 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -127,8 +128,7 @@ static struct command_result *json_newaddr(struct command *cmd, return command_fail(cmd, LIGHTNINGD, "Keys exhausted "); } - if (!bip32_pubkey(cmd->ld->bip32_base, &pubkey, keyidx)) - return command_fail(cmd, LIGHTNINGD, "Keys generation failure"); + bip32_pubkey(cmd->ld, &pubkey, keyidx); b32script = scriptpubkey_p2wpkh(tmpctx, &pubkey); if (*addrtype & ADDR_BECH32) @@ -189,8 +189,7 @@ static struct command_result *json_listaddrs(struct command *cmd, break; } - if (!bip32_pubkey(cmd->ld->bip32_base, &pubkey, keyidx)) - abort(); + bip32_pubkey(cmd->ld, &pubkey, keyidx); // p2sh u8 *redeemscript_p2sh; @@ -251,7 +250,7 @@ static void json_add_utxo(struct json_stream *response, if (utxo->is_p2sh) { struct pubkey key; - bip32_pubkey(wallet->ld->bip32_base, &key, utxo->keyindex); + bip32_pubkey(wallet->ld, &key, utxo->keyindex); json_add_hex_talarr(response, "redeemscript", bitcoin_redeem_p2sh_p2wpkh(tmpctx, &key)); @@ -650,8 +649,7 @@ static struct command_result *match_psbt_inputs_to_utxos(struct command *cmd, u8 *redeemscript; int wally_err; - bip32_pubkey(cmd->ld->bip32_base, &key, - utxo->keyindex); + bip32_pubkey(cmd->ld, &key, utxo->keyindex); redeemscript = bitcoin_redeem_p2sh_p2wpkh(tmpctx, &key); scriptPubKey = scriptpubkey_p2sh(tmpctx, redeemscript);