Extra address checks, with new HSM support

common/hsm_version.h

@@ -10,7 +10,9 @@
  * v2: dd89bf9323dff42200003fb864abb6608f3aa645b636fdae3ec81d804ac05196
  * v3: edd3d288fc88a5470adc2f99abcbfe4d4af29fae0c7a80b4226f28810a815524
  * v3 without v1: 3f813898f7de490e9126ab817e1c9a29af79c0413d5e37068acedce3ea7b5429
+ * v4: 41a730986c51b930e2d8d12b3169d24966c2004e08d424bdda310edbbde5ba70
+ * v4 with check_pubkey: 48b3992745aa3c6ab6ce5cdaee9082cb7d70017f523d322015e9710bf49fd193
  */
-#define HSM_MIN_VERSION 2
-#define HSM_MAX_VERSION 3
+#define HSM_MIN_VERSION 3
+#define HSM_MAX_VERSION 4
 #endif /* LIGHTNING_COMMON_HSM_VERSION_H */

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;
-}

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 */

common/test/run-json_filter.c

@@ -149,10 +149,6 @@ void towire_u8(u8 **pptr UNNEEDED, u8 v UNNEEDED)
 /* Generated stub for towire_u8_array */
 void towire_u8_array(u8 **pptr UNNEEDED, const u8 *arr UNNEEDED, size_t num UNNEEDED)
 { fprintf(stderr, "towire_u8_array called!\n"); abort(); }
-/* Generated stub for type_to_string_ */
-const char *type_to_string_(const tal_t *ctx UNNEEDED, const char *typename UNNEEDED,
-			    union printable_types u UNNEEDED)
-{ fprintf(stderr, "type_to_string_ called!\n"); abort(); }
 /* AUTOGENERATED MOCKS END */
 
 bool deprecated_apis;

hsmd/hsmd.c

@@ -680,6 +680,7 @@ static struct io_plan *handle_client(struct io_conn *conn, struct client *c)
 	case WIRE_HSMD_SIGN_LOCAL_HTLC_TX:
 	case WIRE_HSMD_SIGN_REMOTE_HTLC_TO_US:
 	case WIRE_HSMD_SIGN_DELAYED_PAYMENT_TO_US:
+	case WIRE_HSMD_CHECK_PUBKEY:
 		/* Hand off to libhsmd for processing */
 		return req_reply(conn, c,
 				 take(hsmd_handle_client_message(
@@ -695,6 +696,7 @@ static struct io_plan *handle_client(struct io_conn *conn, struct client *c)
 	case WIRE_HSMD_SIGN_WITHDRAWAL_REPLY:
 	case WIRE_HSMD_SIGN_INVOICE_REPLY:
 	case WIRE_HSMD_INIT_REPLY_V2:
+	case WIRE_HSMD_INIT_REPLY_V4:
 	case WIRE_HSMD_DERIVE_SECRET_REPLY:
 	case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST:
 	case WIRE_HSMD_SIGN_COMMITMENT_TX_REPLY:
@@ -711,6 +713,7 @@ static struct io_plan *handle_client(struct io_conn *conn, struct client *c)
 	case WIRE_HSMD_SIGN_BOLT12_REPLY:
 	case WIRE_HSMD_PREAPPROVE_INVOICE_REPLY:
 	case WIRE_HSMD_PREAPPROVE_KEYSEND_REPLY:
+	case WIRE_HSMD_CHECK_PUBKEY_REPLY:
 		return bad_req_fmt(conn, c, c->msg_in,
 				   "Received an incoming message of type %s, "
 				   "which is not a request",

hsmd/hsmd_wire.csv

@@ -19,11 +19,25 @@ msgdata,hsmd_init,hsm_wire_min_version,u32,
 msgdata,hsmd_init,hsm_wire_max_version,u32,
 
 #include <common/bip32.h>
+# DEPRECATED after 23.05, remove in two versions!
 msgtype,hsmd_init_reply_v2,113
 msgdata,hsmd_init_reply_v2,node_id,node_id,
 msgdata,hsmd_init_reply_v2,bip32,ext_key,
 msgdata,hsmd_init_reply_v2,bolt12,pubkey,
 
+# Sorry: I should have put version in v2 :(
+msgtype,hsmd_init_reply_v4,114
+# This gets upgraded when the wire protocol changes in incompatible
+# ways:
+msgdata,hsmd_init_reply_v4,hsm_version,u32,
+# Capabilities, by convention are message numbers, indicating
+# that the HSM supports you sending this message.
+msgdata,hsmd_init_reply_v4,num_hsm_capabilities,u16,
+msgdata,hsmd_init_reply_v4,hsm_capabilities,u32,num_hsm_capabilities
+msgdata,hsmd_init_reply_v4,node_id,node_id,
+msgdata,hsmd_init_reply_v4,bip32,ext_key,
+msgdata,hsmd_init_reply_v4,bolt12,pubkey,
+
 # Declare a new channel.
 msgtype,hsmd_new_channel,30
 msgdata,hsmd_new_channel,id,node_id,
@@ -309,3 +323,12 @@ msgdata,hsmd_derive_secret,info,u8,len
 # Reply with the derived secret
 msgtype,hsmd_derive_secret_reply,127
 msgdata,hsmd_derive_secret_reply,secret,secret,
+
+# Sanity check this pubkey derivation is correct (unhardened only)
+msgtype,hsmd_check_pubkey,28
+msgdata,hsmd_check_pubkey,index,u32,
+msgdata,hsmd_check_pubkey,pubkey,pubkey,
+
+# Reply
+msgtype,hsmd_check_pubkey_reply,128
+msgdata,hsmd_check_pubkey_reply,ok,bool,

hsmd/libhsmd.c

@@ -1,5 +1,6 @@
 #include "config.h"
 #include <bitcoin/script.h>
+#include <ccan/array_size/array_size.h>
 #include <ccan/crypto/hkdf_sha256/hkdf_sha256.h>
 #include <ccan/tal/str/str.h>
 #include <common/bolt12_merkle.h>
@@ -122,6 +123,7 @@ bool hsmd_check_client_capabilities(struct hsmd_client *client,
 	case WIRE_HSMD_PREAPPROVE_INVOICE:
 	case WIRE_HSMD_PREAPPROVE_KEYSEND:
 	case WIRE_HSMD_DERIVE_SECRET:
+	case WIRE_HSMD_CHECK_PUBKEY:
 		return (client->capabilities & HSM_CAP_MASTER) != 0;
 
 	/*~ These are messages sent by the HSM so we should never receive them. */
@@ -137,6 +139,7 @@ bool hsmd_check_client_capabilities(struct hsmd_client *client,
 	case WIRE_HSMD_SIGN_WITHDRAWAL_REPLY:
 	case WIRE_HSMD_SIGN_INVOICE_REPLY:
 	case WIRE_HSMD_INIT_REPLY_V2:
+	case WIRE_HSMD_INIT_REPLY_V4:
 	case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST:
 	case WIRE_HSMD_SIGN_COMMITMENT_TX_REPLY:
 	case WIRE_HSMD_VALIDATE_COMMITMENT_TX_REPLY:
@@ -153,6 +156,7 @@ bool hsmd_check_client_capabilities(struct hsmd_client *client,
 	case WIRE_HSMD_PREAPPROVE_INVOICE_REPLY:
 	case WIRE_HSMD_PREAPPROVE_KEYSEND_REPLY:
 	case WIRE_HSMD_DERIVE_SECRET_REPLY:
+	case WIRE_HSMD_CHECK_PUBKEY_REPLY:
 		break;
 	}
 	return false;
@@ -532,6 +536,33 @@ static u8 *handle_sign_to_us_tx(struct hsmd_client *c, const u8 *msg_in,
 	return towire_hsmd_sign_tx_reply(NULL, &sig);
 }
 
+/* This will check lightningd's key derivation: hopefully any errors in
+ * this process are independent of errors in lightningd! */
+static u8 *handle_check_pubkey(struct hsmd_client *c, const u8 *msg_in)
+{
+	u32 index;
+	struct pubkey their_pubkey, our_pubkey;
+	struct privkey our_privkey;
+
+	if (!fromwire_hsmd_check_pubkey(msg_in, &index, &their_pubkey))
+		return hsmd_status_malformed_request(c, msg_in);
+
+	/* We abort if lightningd asks for a stupid index. */
+	bitcoin_key(&our_privkey, &our_pubkey, index);
+	if (!pubkey_eq(&our_pubkey, &their_pubkey)) {
+		hsmd_status_failed(STATUS_FAIL_INTERNAL_ERROR,
+				   "BIP32 derivation index %u differed:"
+				   " they got %s, we got %s",
+				   index,
+				   type_to_string(tmpctx, struct pubkey,
+						  &their_pubkey),
+				   type_to_string(tmpctx, struct pubkey,
+						  &our_pubkey));
+	}
+
+	return towire_hsmd_check_pubkey_reply(NULL, true);
+}
+
 /*~ lightningd asks us to sign a message.  I tweeted the spec
  * in https://twitter.com/rusty_twit/status/1182102005914800128:
  *
@@ -1649,6 +1680,8 @@ u8 *hsmd_handle_client_message(const tal_t *ctx, struct hsmd_client *client,
 		return handle_sign_delayed_payment_to_us(client, msg);
 	case WIRE_HSMD_DERIVE_SECRET:
 		return handle_derive_secret(client, msg);
+	case WIRE_HSMD_CHECK_PUBKEY:
+		return handle_check_pubkey(client, msg);
 
 	case WIRE_HSMD_DEV_MEMLEAK:
 	case WIRE_HSMD_ECDH_RESP:
@@ -1662,6 +1695,7 @@ u8 *hsmd_handle_client_message(const tal_t *ctx, struct hsmd_client *client,
 	case WIRE_HSMD_SIGN_WITHDRAWAL_REPLY:
 	case WIRE_HSMD_SIGN_INVOICE_REPLY:
 	case WIRE_HSMD_INIT_REPLY_V2:
+	case WIRE_HSMD_INIT_REPLY_V4:
 	case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST:
 	case WIRE_HSMD_SIGN_COMMITMENT_TX_REPLY:
 	case WIRE_HSMD_VALIDATE_COMMITMENT_TX_REPLY:
@@ -1677,6 +1711,7 @@ u8 *hsmd_handle_client_message(const tal_t *ctx, struct hsmd_client *client,
 	case WIRE_HSMD_SIGN_BOLT12_REPLY:
 	case WIRE_HSMD_PREAPPROVE_INVOICE_REPLY:
 	case WIRE_HSMD_PREAPPROVE_KEYSEND_REPLY:
+	case WIRE_HSMD_CHECK_PUBKEY_REPLY:
 		break;
 	}
 	return hsmd_status_bad_request(client, msg, "Unknown request");
@@ -1690,6 +1725,7 @@ u8 *hsmd_init(struct secret hsm_secret,
 	u32 salt = 0;
 	struct ext_key master_extkey, child_extkey;
 	struct node_id node_id;
+	static const u32 capabilities[] = { WIRE_HSMD_CHECK_PUBKEY };
 
 	/*~ Don't swap this. */
 	sodium_mlock(secretstuff.hsm_secret.data,
@@ -1815,8 +1851,15 @@ u8 *hsmd_init(struct secret hsm_secret,
 
 	/*~ Note: marshalling a bip32 tree only marshals the public side,
 	 * not the secrets!  So we're not actually handing them out here!
+	 *
+	 * And version is 4: we offer limited compatibility (or at least,
+	 * incompatibility detection) with alternate implementations.
 	 */
-	return take(towire_hsmd_init_reply_v2(
-	    NULL, &node_id, &secretstuff.bip32,
-	    &bolt12));
+	return take(towire_hsmd_init_reply_v4(
+			    NULL, 4,
+			    /* Capabilities arg needs to be a tal array */
+			    tal_dup_arr(tmpctx, u32, capabilities,
+					ARRAY_SIZE(capabilities), 0),
+			    &node_id, &secretstuff.bip32,
+			    &bolt12));
 }

lightningd/channel_control.c

@@ -711,7 +711,7 @@ bool peer_start_channeld(struct channel *channel,
 
 	struct ext_key final_ext_key;
 	if (bip32_key_from_parent(
-		    ld->wallet->bip32_base,
+		    ld->bip32_base,
 		    channel->final_key_idx,
 		    BIP32_FLAG_KEY_PUBLIC,
 		    &final_ext_key) != WALLY_OK) {

lightningd/closing_control.c

@@ -465,7 +465,7 @@ void peer_start_closingd(struct channel *channel, struct peer_fd *peer_fd)
 		    &index_val,
 		    &is_p2sh)) {
 		if (bip32_key_from_parent(
-			    ld->wallet->bip32_base,
+			    ld->bip32_base,
 			    index_val,
 			    BIP32_FLAG_KEY_PUBLIC,
 			    &ext_key_val) != WALLY_OK) {
@@ -838,7 +838,7 @@ static struct command_result *json_close(struct command *cmd,
 					struct ext_key *final_ext_key = NULL;
 					if (final_index) {
 						if (bip32_key_from_parent(
-							    channel->peer->ld->wallet->bip32_base,
+							    channel->peer->ld->bip32_base,
 							    *final_index,
 							    BIP32_FLAG_KEY_PUBLIC,
 							    &ext_key_val) != WALLY_OK) {

lightningd/hsm_control.c

@@ -77,11 +77,23 @@ static unsigned int hsm_msg(struct subd *hsmd,
 	return 0;
 }
 
+/* Is this capability supported by the HSM? (So far, always a message
+ * number) */
+bool hsm_capable(struct lightningd *ld, u32 msgtype)
+{
+	for (size_t i = 0; i < tal_count(ld->hsm_capabilities); i++) {
+		if (ld->hsm_capabilities[i] == msgtype)
+			return true;
+	}
+	return false;
+}
+
 struct ext_key *hsm_init(struct lightningd *ld)
 {
 	u8 *msg;
 	int fds[2];
 	struct ext_key *bip32_base;
+	u32 hsm_version;
 
 	/* We actually send requests synchronously: only status is async. */
 	if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) != 0)
@@ -104,7 +116,6 @@ struct ext_key *hsm_init(struct lightningd *ld)
 	}
 
 	ld->hsm_fd = fds[0];
-	u32 min_version = 3; /* payment modifiers need hsmd_preapprove_{invoice,keysend} */
 	if (!wire_sync_write(ld->hsm_fd, towire_hsmd_init(tmpctx,
 							 &chainparams->bip32_key_version,
 							 chainparams,
@@ -113,20 +124,45 @@ struct ext_key *hsm_init(struct lightningd *ld)
 							 IFDEV(ld->dev_force_bip32_seed, NULL),
 							 IFDEV(ld->dev_force_channel_secrets, NULL),
 							  IFDEV(ld->dev_force_channel_secrets_shaseed, NULL),
-							  min_version,
+							  HSM_MIN_VERSION,
 							  HSM_MAX_VERSION)))
 		err(EXITCODE_HSM_GENERIC_ERROR, "Writing init msg to hsm");
 
 	bip32_base = tal(ld, struct ext_key);
 	msg = wire_sync_read(tmpctx, ld->hsm_fd);
-	if (!fromwire_hsmd_init_reply_v2(msg,
-					 &ld->id, bip32_base,
-					 &ld->bolt12_base)) {
+	if (fromwire_hsmd_init_reply_v4(ld, msg,
+					&hsm_version,
+					&ld->hsm_capabilities,
+					&ld->id, bip32_base,
+					&ld->bolt12_base)) {
+		/* nothing to do. */
+	} else if (fromwire_hsmd_init_reply_v2(msg,
+					       &ld->id, bip32_base,
+					       &ld->bolt12_base)) {
+		/* implicit version */
+		hsm_version = 3;
+		ld->hsm_capabilities = NULL;
+	} else {
 		if (ld->config.keypass)
 			errx(EXITCODE_HSM_BAD_PASSWORD, "Wrong password for encrypted hsm_secret.");
 		errx(EXITCODE_HSM_GENERIC_ERROR, "HSM did not give init reply");
 	}
 
+	if (hsm_version < HSM_MIN_VERSION)
+		errx(EXITCODE_HSM_GENERIC_ERROR,
+		     "HSM version %u below minimum %u",
+		     hsm_version, HSM_MIN_VERSION);
+	if (hsm_version > HSM_MAX_VERSION)
+		errx(EXITCODE_HSM_GENERIC_ERROR,
+		     "HSM version %u above maximum %u",
+		     hsm_version, HSM_MAX_VERSION);
+
+	/* Debugging help */
+	for (size_t i = 0; i < tal_count(ld->hsm_capabilities); i++) {
+		log_debug(ld->hsm->log, "capability +%s",
+			  hsmd_wire_name(ld->hsm_capabilities[i]));
+	}
+
 	/* This is equivalent to makesecret("bolt12-invoice-base") */
 	msg = towire_hsmd_derive_secret(NULL, tal_dup_arr(tmpctx, u8,
 							  (const u8 *)INVOICE_PATH_BASE_STRING,
@@ -141,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,

lightningd/hsm_control.h

@@ -16,5 +16,13 @@ int hsm_get_client_fd(struct lightningd *ld,
 /* Ask HSM for an fd for a global subdaemon to use (gossipd, connectd) */
 int hsm_get_global_fd(struct lightningd *ld, int capabilities);
 
+/* Is this capability supported by the HSM? (So far, always a message
+ * number) */
+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 */