offers, xpay, hsmd, gossipd: assorted fixes (v26.06.7 security, 7/7)

gossipd/gossmap_manage.c

@@ -669,6 +669,10 @@ const char *gossmap_manage_channel_announcement(const tal_t *ctx,
 	if (!bitcoin_blkid_eq(&chain_hash, &chainparams->genesis_blockhash))
 		return NULL;
 
+	/* Immediately discard claims of ancient channels */
+	if (short_channel_id_blocknum(scid) < chainparams->when_lightning_became_cool)
+		return tal_fmt(ctx, "Unknown UTXO %s", fmt_short_channel_id(tmpctx, scid));
+
 	/* If a prior txout lookup failed there is little point it trying
 	 * again. Just drop the announcement and walk away whistling.
 	 *

hsmd/hsmd.c

@@ -102,31 +102,15 @@ static bool is_lightningd(const struct client *client)
 /* Pre-declare this, due to mutual recursion */
 static struct io_plan *handle_client(struct io_conn *conn, struct client *c);
 
-/*~ ccan/compiler.h defines PRINTF_FMT as the gcc compiler hint so it will
- * check that fmt and other trailing arguments really are the correct type.
+/*~ Tell lightningd a client sent a bad request.  This should never
+ * happen, of course, but we definitely want to log if it does.
  *
- * This is a convenient helper to tell lightningd we've received a bad request
- * and closes the client connection.  This should never happen, of course, but
- * we definitely want to log if it does.
- */
-static struct io_plan *bad_req_fmt(struct io_conn *conn,
-				   struct client *c,
-				   const u8 *msg_in,
-				   const char *fmt, ...)
-	PRINTF_FMT(4,5);
-
-static struct io_plan *bad_req_fmt(struct io_conn *conn,
-				   struct client *c,
-				   const u8 *msg_in,
-				   const char *fmt, ...)
+ * Does not close the connection: the caller decides.  bad_req_fmt
+ * closes immediately; hsmd_status_bad_request returns NULL and
+ * handle_client closes, so we don't io_close/free conn before
+ * handle_client can return. */
+static void report_bad_req(struct client *c, const u8 *msg_in, const char *str)
 {
-	va_list ap;
-	char *str;
-
-	va_start(ap, fmt);
-	str = tal_vfmt(tmpctx, fmt, ap);
-	va_end(ap);
-
 	/*~ If the client was actually lightningd, it's Game Over; we actually
 	 * fail in this case, and it will too. */
 	if (is_lightningd(c)) {
@@ -148,6 +132,33 @@ static struct io_plan *bad_req_fmt(struct io_conn *conn,
 								  &c->id,
 								  str,
 								  msg_in)));
+}
+
+/*~ ccan/compiler.h defines PRINTF_FMT as the gcc compiler hint so it will
+ * check that fmt and other trailing arguments really are the correct type.
+ *
+ * This is a convenient helper to tell lightningd we've received a bad request
+ * and closes the client connection.
+ */
+static struct io_plan *bad_req_fmt(struct io_conn *conn,
+				   struct client *c,
+				   const u8 *msg_in,
+				   const char *fmt, ...)
+	PRINTF_FMT(4,5);
+
+static struct io_plan *bad_req_fmt(struct io_conn *conn,
+				   struct client *c,
+				   const u8 *msg_in,
+				   const char *fmt, ...)
+{
+	va_list ap;
+	char *str;
+
+	va_start(ap, fmt);
+	str = tal_vfmt(tmpctx, fmt, ap);
+	va_end(ap);
+
+	report_bad_req(c, msg_in, str);
 
 	/*~ The way ccan/io works is that you return the "plan" for what to do
 	 * next (eg. io_read).  io_close() is special: it means to close the
@@ -658,10 +669,11 @@ u8 *hsmd_status_bad_request(struct hsmd_client *client, const u8 *msg, const cha
 	/* Extract the pointer to the hsmd representation of the
 	 * client which has access to the underlying connection. */
 	struct client *c = (struct client*)client->extra;
-	bad_req_fmt(c->conn, c, msg, "%s", error);
+
+	report_bad_req(c, msg, error);
 
 	/* We often use `return hsmd_status_bad_request` to drop out, and NULL
-	 * means we encountered an error. */
+	 * means we encountered an error.  handle_client then io_close's. */
 	return NULL;
 }
 
@@ -802,11 +814,13 @@ static struct io_plan *handle_client(struct io_conn *conn, struct client *c)
 	case WIRE_HSMD_SIGN_ANY_REMOTE_HTLC_TO_US:
 	case WIRE_HSMD_SIGN_ANY_LOCAL_HTLC_TX:
 	case WIRE_HSMD_SIGN_ANCHORSPEND:
-	case WIRE_HSMD_SIGN_HTLC_TX_MINGLE:
-		/* Hand off to libhsmd for processing */
-		return req_reply(conn, c,
-				 take(hsmd_handle_client_message(
-				     tmpctx, c->hsmd_client, c->msg_in)));
+	case WIRE_HSMD_SIGN_HTLC_TX_MINGLE: {
+		u8 *reply = hsmd_handle_client_message(tmpctx, c->hsmd_client,
+						       c->msg_in);
+		if (!reply)
+			return io_close(conn);
+		return req_reply(conn, c, take(reply));
+	}
 
 	case WIRE_HSMD_ECDH_RESP:
 	case WIRE_HSMD_CANNOUNCEMENT_SIG_REPLY:

hsmd/libhsmd.c

@@ -232,7 +232,7 @@ static u8 *hsmd_status_bad_request_fmt(struct hsmd_client *client,
 	char *str;
 
 	va_start(ap, fmt);
-	str = tal_fmt(tmpctx, fmt, ap);
+	str = tal_vfmt(tmpctx, fmt, ap);
 	va_end(ap);
 	return hsmd_status_bad_request(client, msg, str);
 }
@@ -2593,3 +2593,11 @@ u8 *hsmd_init(const u8 *secret_data, size_t secret_len, const u64 hsmd_version,
 		    &node_id, &secretstuff.bip32,
 		    &bolt12, tlvs));
 }
+
+void hsmd_deinit(void)
+{
+	/* Frees off NULL, so it also fires the mlock_tal_memory destructor
+	 * which wipes and munlocks it. */
+	secretstuff.bip32_seed = tal_free(secretstuff.bip32_seed);
+	initialized = false;
+}

hsmd/libhsmd.h

@@ -49,6 +49,13 @@ struct hsmd_client {
 u8 *hsmd_init(const u8 *secret_data, size_t secret_len, const u64 hsmd_version,
 	      struct bip32_key_version bip32_key_version, u8 hsm_secret_type);
 
+/* Release the secrets hsmd_init() cached.
+ *
+ * hsmd itself never needs this: it holds the seed for the life of the process
+ * and exits with it.  Unit tests do, since they return from main() and a
+ * process-lifetime allocation still shows up as a leak under valgrind. */
+void hsmd_deinit(void);
+
 struct hsmd_client *hsmd_client_new_main(const tal_t *ctx, u64 capabilities,
 					 void *extra);
 

hsmd/test/Makefile

@@ -0,0 +1,19 @@
+check-units: hsmd-tests
+
+# Note that these actually #include everything they need, except ccan/ and bitcoin/.
+# That allows for unit testing of statics, and special effects.
+HSMD_TEST_SRC := $(wildcard hsmd/test/run-*.c)
+HSMD_TEST_OBJS := $(HSMD_TEST_SRC:.c=.o)
+HSMD_TEST_PROGRAMS := $(HSMD_TEST_OBJS:.o=)
+
+ALL_C_SOURCES += $(HSMD_TEST_SRC)
+ALL_TEST_PROGRAMS += $(HSMD_TEST_PROGRAMS)
+
+# hsmd.c is #included by the tests (so we can see statics).  Do not also
+# link hsmd.o, or we get duplicate symbols.  libhsmd.o still supplies the
+# request handlers.
+$(HSMD_TEST_PROGRAMS): libcommon.a hsmd/libhsmd.o hsmd/hsm_utxo.o hsmd/hsmd_wiregen.o
+
+$(HSMD_TEST_OBJS): $(HSMD_HEADERS) $(HSMD_SRC) hsmd/test/Makefile
+
+hsmd-tests: $(HSMD_TEST_PROGRAMS:%=unittest/%)

hsmd/test/run-bad-request-close.c

@@ -0,0 +1,159 @@
+/* Test the client-connection lifecycle when a request is rejected:
+ * when libhsmd returns NULL from hsmd_handle_client_message, hsmd must
+ * close the client connection exactly once, and must not touch it
+ * again afterwards.
+ *
+ * That contract isn't observable from outside the process (the client
+ * just sees its fd close), so a black-box test can't verify it.
+ * Instead we compile hsmd.c into this test and watch the connection
+ * itself:
+ *
+ * 1. io_set_finish() on the client conn: ccan/io runs the finish
+ *    callback exactly when the conn is closed, so counting calls tells
+ *    us the conn was closed once (not zero times, not twice).
+ *
+ * 2. Every io_write_wire() from hsmd.c is intercepted (see the macro
+ *    below), and a write to the client conn *after* its finish
+ *    callback has run aborts the test.  This makes any ordering
+ *    violation fail deterministically, without needing ASan or
+ *    valgrind to notice.
+ */
+#include "config.h"
+#include <assert.h>
+#include <bitcoin/chainparams.h>
+#include <bitcoin/privkey.h>
+#include <bitcoin/pubkey.h>
+#include <common/setup.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <sys/socket.h>
+#include <unistd.h>
+#include <wire/wire_io.h>
+
+/* The client conn created in main(), watched by the checks below. */
+static struct io_conn *client_conn;
+
+/* How many times the client conn's finish callback has run. */
+static int finish_count;
+
+/* Distinct pointer for io_break, so main() knows why io_loop returned. */
+static char closed_token[] = "closed";
+
+/* Wrapper around the real io_write_wire_: a closed conn (finish
+ * callback already ran) must never be written to again. */
+static struct io_plan *check_client_write(struct io_conn *conn,
+					  const u8 *data,
+					  struct io_plan *(*next)(struct io_conn *, void *),
+					  void *next_arg)
+{
+	if (conn == client_conn && finish_count != 0) {
+		fprintf(stderr,
+			"write to client conn after close (finish_count=%d)\n",
+			finish_count);
+		abort();
+	}
+	return io_write_wire_(conn, data, next, next_arg);
+}
+
+/* Redefine io_write_wire before pulling in hsmd.c, so every write the
+ * daemon makes (req_reply in particular) goes through the check above. */
+#undef io_write_wire
+#define io_write_wire(conn, data, next, arg)				\
+	check_client_write((conn), (data),				\
+			   typesafe_cb_preargs(struct io_plan *, void *, \
+					       (next), (arg),		\
+					       struct io_conn *),	\
+			   (arg))
+
+/* Include the daemon itself so we can drive its statics (new_client,
+ * handle_client via the io loop).  Rename its main() out of the way. */
+int unused_main(int argc, char *argv[]);
+#define main unused_main
+#include "../hsmd.c"
+#undef main
+#undef io_write_wire
+
+/* Finish callback for the client conn: ccan/io calls this exactly when
+ * the conn is closed.  Closing is what ends the test, so break out. */
+static void client_finished(struct io_conn *conn UNUSED, int *count)
+{
+	(*count)++;
+	io_break(closed_token);
+}
+
+/* Any valid pubkey will do as the client's node id. */
+static void make_peer_id(struct node_id *id)
+{
+	struct privkey priv;
+	struct pubkey pub;
+
+	memset(&priv, 1, sizeof(priv));
+	assert(pubkey_from_privkey(&priv, &pub));
+	node_id_from_pubkey(id, &pub);
+}
+
+int main(int argc, char *argv[])
+{
+	int status_fds[2], client_fds[2];
+	struct node_id peer_id;
+	struct secret secret;
+	struct client *c;
+	const struct chainparams *params;
+	u8 hsmseed[32];
+	u8 *msg;
+	void *ret;
+
+	common_setup(argv[0]);
+	uintmap_init(&clients);
+
+	/* hsmd reports bad requests to lightningd over status_conn; give
+	 * it a socketpair whose other end we simply never read. */
+	assert(socketpair(AF_LOCAL, SOCK_STREAM, 0, status_fds) == 0);
+	status_conn = daemon_conn_new(NULL, status_fds[0], NULL, NULL, NULL);
+	status_setup_async(status_conn);
+
+	/* Initialize libhsmd's secrets from a dummy seed (normally done
+	 * via the WIRE_HSMD_INIT message from lightningd). */
+	params = chainparams_for_network("regtest");
+	memset(hsmseed, 1, sizeof(hsmseed));
+	msg = hsmd_init(hsmseed, sizeof(hsmseed), 6,
+			params->bip32_key_version, HSM_SECRET_PLAIN);
+	assert(msg);
+	/* hsmd_init returns take(): consume the marker, then free. */
+	taken(msg);
+	tal_free(msg);
+
+	/* Connect a fake per-channel client (as if lightningd had passed
+	 * an fd to a channeld).  dbid 1 = an ordinary channel client;
+	 * HSM_PERM_COMMITMENT_POINT allows check_future_secret. */
+	assert(socketpair(AF_LOCAL, SOCK_STREAM, 0, client_fds) == 0);
+	make_peer_id(&peer_id);
+	c = new_client(NULL, params, &peer_id, 1,
+		       HSM_PERM_COMMITMENT_POINT, client_fds[0]);
+	client_conn = c->conn;
+	io_set_finish(c->conn, client_finished, &finish_count);
+
+	/* Send a request libhsmd is guaranteed to reject: commitment
+	 * index 2^48 is beyond the shachain, so per_commit_secret fails
+	 * and the handler returns via hsmd_status_bad_request -> NULL. */
+	memset(&secret, 0, sizeof(secret));
+	msg = towire_hsmd_check_future_secret(NULL, 1ULL << SHACHAIN_BITS,
+					      &secret);
+	assert(wire_sync_write(client_fds[1], take(msg)));
+
+	/* Run the daemon's io loop.  It reads the request, rejects it,
+	 * and must close the client conn, which fires client_finished. */
+	ret = io_loop(NULL, NULL);
+	assert(ret == closed_token);
+
+	/* The heart of the test: closed exactly once. */
+	assert(finish_count == 1);
+
+	close(client_fds[1]);
+	close(status_fds[1]);
+	tal_free(status_conn);
+	hsmd_deinit();
+	common_shutdown();
+	return 0;
+}

lightningd/bitcoind.c

@@ -778,8 +778,24 @@ static void process_getfilteredblock_step1(struct bitcoind *bitcoind,
 	}
 }
 
+/* Find the pending call for the highest block height: we prefer to
+ * satisfy the most recent request first, since it's usually the most
+ * urgent (e.g. catching up to the chain tip). */
+static struct filteredblock_call *
+most_recent_filteredblock_call(struct bitcoind *bitcoind)
+{
+	struct filteredblock_call *c, *best = NULL;
+
+	list_for_each(&bitcoind->pending_getfilteredblock, c, list) {
+		if (!best || c->height > best->height)
+			best = c;
+	}
+	return best;
+}
+
 /* Takes a call, dispatches it to all queued requests that match the same
- * height, and then kicks off the next call. */
+ * height, and then kicks off the call for the highest height still
+ * pending. */
 static void
 process_getfiltered_block_final(struct bitcoind *bitcoind,
 				const struct filteredblock_call *call)
@@ -804,8 +820,8 @@ process_getfiltered_block_final(struct bitcoind *bitcoind,
 	/* Nothing to free here, since `*call` was already deleted during the
 	 * iteration above. It was also removed from the list, so no need to
 	 * pop here. */
-	if (!list_empty(&bitcoind->pending_getfilteredblock)) {
-		c = list_top(&bitcoind->pending_getfilteredblock, struct filteredblock_call, list);
+	c = most_recent_filteredblock_call(bitcoind);
+	if (c) {
 		bitcoind_getrawblockbyheight(bitcoind, bitcoind, c->height,
 					     process_getfilteredblock_step1, c);
 	}

lightningd/onion_message.c

@@ -133,6 +133,12 @@ void handle_onionmsg_to_us(struct lightningd *ld, const u8 *msg)
 	}
 	tal_free(submsg);
 
+	/* A reply path with no hops is unusable: treat it as absent. */
+	if (payload->reply_path && tal_count(payload->reply_path->path) == 0) {
+		log_debug(ld->log, "Ignoring reply path with no hops");
+		payload->reply_path = tal_free(payload->reply_path);
+	}
+
 	/* Make sure connectd gets this right. */
 	log_debug(ld->log, "Got onionmsg%s%s",
 		  payload->pathsecret ? " with pathsecret": "",

plugins/offers.c

@@ -306,8 +306,10 @@ static struct command_result *onion_message_recv(struct command *cmd,
 	replytok = json_get_member(buf, om, "reply_blindedpath");
 	if (replytok) {
 		reply_path = json_to_blinded_path(cmd, buf, replytok);
+		/* Remote-supplied: a bad reply path must not kill us. */
 		if (!reply_path)
-			plugin_err(cmd->plugin, "Invalid reply path %.*s?",
+			plugin_log(cmd->plugin, LOG_UNUSUAL,
+				   "Ignoring invalid reply path %.*s",
 				   json_tok_full_len(replytok),
 				   json_tok_full(buf, replytok));
 	}