diff --git a/gossipd/gossmap_manage.c b/gossipd/gossmap_manage.c index 82a20a8bd617..3676fa9caaa6 100644 --- a/gossipd/gossmap_manage.c +++ b/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. * diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 739a148bda66..e240e64715e8 100644 --- a/hsmd/hsmd.c +++ b/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: diff --git a/hsmd/libhsmd.c b/hsmd/libhsmd.c index c605198a056f..2dfe79f0d297 100644 --- a/hsmd/libhsmd.c +++ b/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; +} diff --git a/hsmd/libhsmd.h b/hsmd/libhsmd.h index 484089d2c009..f69e9f0f59b1 100644 --- a/hsmd/libhsmd.h +++ b/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); diff --git a/hsmd/test/Makefile b/hsmd/test/Makefile new file mode 100644 index 000000000000..36605a4e65ef --- /dev/null +++ b/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/%) diff --git a/hsmd/test/run-bad-request-close.c b/hsmd/test/run-bad-request-close.c new file mode 100644 index 000000000000..78c96effc229 --- /dev/null +++ b/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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* 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; +} diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index bc336c700aa2..317180e85ee5 100644 --- a/lightningd/bitcoind.c +++ b/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); } diff --git a/lightningd/onion_message.c b/lightningd/onion_message.c index 70efb66e53d1..84d2f6a9b95e 100644 --- a/lightningd/onion_message.c +++ b/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": "", diff --git a/plugins/offers.c b/plugins/offers.c index 2cd0c97f1a87..a214e5790f4d 100644 --- a/plugins/offers.c +++ b/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)); } diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index 5b37463edbb4..b24e0cc5cd19 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -2206,9 +2206,11 @@ preapprove_succeed(struct command *cmd, const char *method, const char *buf, return age_layer(cmd, payment); } +/* If it returns NULL, *authorized is the most we agreed to pay. */ static struct command_result *check_offer_payable(struct command *cmd, const char *offerstr, - const struct amount_msat *msat) + const struct amount_msat *msat, + struct amount_msat *authorized) { const char *err; struct tlv_offer *b12offer = offer_decode(tmpctx, @@ -2219,6 +2221,17 @@ static struct command_result *check_offer_payable(struct command *cmd, if (!b12offer) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid bolt12 offer: %s", err); + /* BOLT #12: + * - if `offer_amount` is not present: + * - MUST specify `invreq_amount`. + * - otherwise: + * - MAY omit `invreq_amount`. + * - if it sets `invreq_amount`: + * - MUST specify `invreq_amount`.`msat` as greater or equal to amount expected by `offer_amount` (and, if present, `offer_currency` and `invreq_quantity`). + */ + /* We can't work out the expected amount without a conversion rate, so we + * refuse currency offers here. We also require the exact offer amount, + * which is stricter than the "greater or equal" the spec allows. */ /* We will only one-shot if we know amount! (FIXME: Convert!) */ if (b12offer->offer_currency) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, @@ -2241,6 +2254,11 @@ static struct command_result *check_offer_payable(struct command *cmd, return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Cannot xpay recurring offers"); + if (msat) + *authorized = *msat; + else + *authorized = amount_msat(*b12offer->offer_amount); + return NULL; } @@ -2275,6 +2293,10 @@ check_offer_sendamount_payable(struct command *cmd, const char *offerstr) struct xpay_params { struct amount_msat *msat, *maxfee, *partial, *includefees_msat; + /* What we agreed to pay, if we're paying an offer: the amount we sent + * as invreq_amount, or the offer amount if we sent none. NULL for + * sendamount, where xpay_core demands the invoice match *msat. */ + struct amount_msat *authorized_msat; const char **layers; unsigned int retryfor; u32 maxdelay; @@ -2290,11 +2312,51 @@ invoice_fetched(struct command *cmd, const jsmntok_t *result, struct xpay_params *params) { - const char *inv; + const char *inv, *err; inv = json_strdup(tmpctx, buf, json_get_member(buf, result, "invoice")); - inv = to_canonical_invstr(NULL, inv); - return xpay_core(cmd, take(inv), + inv = to_canonical_invstr(tmpctx, inv); + + /* BOLT #12: + * - if `invreq_amount` is present: + * - MUST reject the invoice if `invoice_amount` is not equal to `invreq_amount` + * - otherwise: + * - SHOULD confirm authorization if `invoice_amount`.`msat` is not within + * the amount range authorized. + */ + /* invoice_amount is not one of the fields the invoice must copy from our + * invoice_request, so it is set independently of what we asked for and + * has to be checked here. We set invreq_amount iff we were given an + * amount, which selects which of the two rules above applies. */ + if (params->authorized_msat) { + struct amount_msat invoice_msat; + struct tlv_invoice *b12inv + = invoice_decode(tmpctx, inv, strlen(inv), + plugin_feature_set(cmd->plugin), + chainparams, &err); + if (!b12inv) + return command_fail(cmd, OFFER_BAD_INVREQ_REPLY, + "Invalid bolt12 invoice: %s", err); + /* invoice_decode() has already insisted on invoice_amount. */ + invoice_msat = amount_msat(*b12inv->invoice_amount); + if (params->msat) { + if (!amount_msat_eq(invoice_msat, *params->authorized_msat)) + return command_fail(cmd, OFFER_BAD_INVREQ_REPLY, + "Invoice amount is %s, but we asked for %s", + fmt_amount_msat(tmpctx, invoice_msat), + fmt_amount_msat(tmpctx, + *params->authorized_msat)); + } else if (amount_msat_greater(invoice_msat, + *params->authorized_msat)) { + return command_fail(cmd, OFFER_BAD_INVREQ_REPLY, + "Invoice amount is %s, more than the %s we authorized", + fmt_amount_msat(tmpctx, invoice_msat), + fmt_amount_msat(tmpctx, + *params->authorized_msat)); + } + } + + return xpay_core(cmd, inv, NULL, params->maxfee, params->layers, params->retryfor, params->partial, params->maxdelay, params->label, NULL, false, false, @@ -2349,8 +2411,15 @@ bip353_fetched(struct command *cmd, if (xparams->includefees_msat) ret = check_offer_sendamount_payable(cmd, offerstr); - else - ret = check_offer_payable(cmd, offerstr, xparams->msat); + else { + struct amount_msat authorized; + ret = check_offer_payable(cmd, offerstr, xparams->msat, + &authorized); + if (!ret) + xparams->authorized_msat + = tal_dup(xparams, struct amount_msat, + &authorized); + } if (ret) return ret; @@ -2393,8 +2462,9 @@ static struct command_result *json_xpay_params(struct command *cmd, /* Is this a one-shot vibe payment? Kids these days! */ if (!as_pay && bolt12_has_offer_prefix(invstring)) { struct command_result *ret; + struct amount_msat authorized; - ret = check_offer_payable(cmd, invstring, msat); + ret = check_offer_payable(cmd, invstring, msat, &authorized); if (ret) return ret; @@ -2420,6 +2490,8 @@ static struct command_result *json_xpay_params(struct command *cmd, xparams->payer_note = payer_note; xparams->label = label; xparams->includefees_msat = NULL; + xparams->authorized_msat = tal_dup(xparams, struct amount_msat, + &authorized); return do_fetchinvoice(cmd, invstring, xparams); } @@ -2448,6 +2520,8 @@ static struct command_result *json_xpay_params(struct command *cmd, xparams->payer_note = payer_note; xparams->label = label; xparams->includefees_msat = NULL; + /* Set once bip353_fetched() knows the offer. */ + xparams->authorized_msat = NULL; req = jsonrpc_request_start(cmd, "fetchbip353", bip353_fetched, @@ -2645,6 +2719,13 @@ static struct command_result *xpay_core(struct command *cmd, if (amount_msat_is_zero(amount_msat(*b12inv->invoice_amount))) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid bolt12 invoice with zero amount"); + /* BOLT #12: + * - if `invoice_relative_expiry` is present: + * - MUST reject the invoice if the current time since 1970-01-01 UTC is greater than `invoice_created_at` plus `seconds_from_creation`. + * - otherwise: + * - MUST reject the invoice if the current time since 1970-01-01 UTC is greater than `invoice_created_at` plus 7200. + */ + /* invoice_expiry() applies the 7200 second default for us. */ invexpiry = invoice_expiry(b12inv); invoice_msat = amount_msat(*b12inv->invoice_amount); @@ -2937,6 +3018,8 @@ static struct command_result *json_sendamount(struct command *cmd, xparams->bip353 = NULL; xparams->payer_note = payer_note; xparams->label = label; + /* xpay_core() insists the invoice match *msat exactly here. */ + xparams->authorized_msat = NULL; return do_fetchinvoice(cmd, invstring, xparams); } @@ -2955,6 +3038,8 @@ static struct command_result *json_sendamount(struct command *cmd, xparams->bip353 = invstring; xparams->payer_note = payer_note; xparams->label = label; + /* xpay_core() insists the invoice match *msat exactly here. */ + xparams->authorized_msat = NULL; req = jsonrpc_request_start(cmd, "fetchbip353", bip353_fetched, forward_error, xparams); diff --git a/tests/plugins/xpay_mismatched_invoice_amount.py b/tests/plugins/xpay_mismatched_invoice_amount.py new file mode 100755 index 000000000000..f38c0e45277a --- /dev/null +++ b/tests/plugins/xpay_mismatched_invoice_amount.py @@ -0,0 +1,30 @@ +#!/usr/bin/env python3 +"""Plugin which makes fetchinvoice hand back a mismatched invoice. + +Stands in for a payee which sets invoice_amount to something other than the +amount our invoice_request asked for. The invoice is validly signed; +fetchinvoice reports the difference in `changes.amount_msat`. + +Call `setinvoiceamount` to arm it; until then fetchinvoice behaves normally. +""" +from pyln.client import Plugin + +plugin = Plugin() + + +@plugin.method("setinvoiceamount") +def setinvoiceamount(plugin, invoice, amount_msat): + plugin.mismatched = {"invoice": invoice, + "changes": {"amount_msat": amount_msat}} + return {} + + +@plugin.hook("rpc_command") +def on_rpc_command(plugin, rpc_command, **kwargs): + if rpc_command["method"] != "fetchinvoice" or plugin.mismatched is None: + return {"result": "continue"} + return {"return": {"result": plugin.mismatched}} + + +plugin.mismatched = None +plugin.run() diff --git a/tests/test_connection.py b/tests/test_connection.py index 0e672477c94d..baa3f1345464 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -3,7 +3,9 @@ from decimal import Decimal from pathlib import Path from pyln.client import RpcError, Millisatoshi +from pyln.proto.onion import TlvPayload import pyln.proto.wire as wire +from hashlib import sha256 from utils import ( only_one, wait_for, sync_blockheight, TIMEOUT, expected_peer_features, expected_node_features, @@ -15,6 +17,8 @@ ) from pyln.testing.utils import VALGRIND, EXPERIMENTAL_DUAL_FUND, FUNDAMOUNT, RUST, SLOW_MACHINE +import coincurve +import hmac import os import pytest import random @@ -4579,6 +4583,49 @@ def test_injectonionmessage(node_factory): l1.daemon.wait_for_log('lightningd: Got onionmsg with pathsecret') +def test_onionmessage_reply_path_no_hops(node_factory): + """A reply_path with num_hops=0 must not take the node down. + + The blinded reply_path's num_hops is a single attacker-supplied byte + with no lower bound; a zero-hop path serialises an empty "hops" array + to the offers plugin, which used to reject it via plugin_err and (as + an important builtin) stop lightningd. + """ + l1, l2 = node_factory.line_graph(2) + + def ecdh(privkey_bytes, pubkey_bytes): + priv = coincurve.PrivateKey(privkey_bytes) + return priv.ecdh(coincurve.PublicKey(pubkey_bytes).public_key) + + # Build an onion message to l2 with a reply_path carrying no hops. + l2_pub = bytes.fromhex(l2.info['id']) + blinding = coincurve.PrivateKey() + path_key = blinding.public_key.format(True) + + # Route-blinding tweak so the onion decrypts as l2's real key. + ss = ecdh(blinding.secret, l2_pub) + tweak = hmac.new(b'blinded_node_id', ss, sha256).digest() + blinded_pub = coincurve.PublicKey(l2_pub).multiply(tweak).format(True) + + # blinded_path: first_node_id (point), first_path_key (point), num_hops=0 + reply_path = l2_pub + path_key + b'\x00' + tlv = TlvPayload() + tlv.add_field(2, reply_path) + + onion = l1.rpc.createonion(hops=[{'pubkey': blinded_pub.hex(), + 'payload': tlv.to_bytes().hex()}], + assocdata="") + + l2.rpc.injectonionmessage(message=onion['onion'], path_key=path_key.hex()) + + # With the fix, lightningd drops the hopless reply path when it decodes + # the message (logged synchronously, before the offers hook runs), so the + # node stays up. Without the fix this log never appears: the offers plugin + # calls plugin_err and lightningd_exit takes the node down instead. + l2.daemon.wait_for_log('Ignoring reply path with no hops', timeout=30) + assert l2.rpc.getinfo()['id'] == l2.info['id'] + + def test_connect_ratelimit(node_factory, bitcoind): """l1 has 5 peers, restarts, make sure we limit""" # Sending nodes SIGSTOP at the wrong time makes connectd complain about diff --git a/tests/test_xpay.py b/tests/test_xpay.py index 1568438f6d15..3dba55eab128 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -1031,6 +1031,45 @@ def test_xpay_offer(node_factory): l1.rpc.xpay(offer2, 5000) +def test_xpay_offer_invoice_amount_mismatch(node_factory): + """xpay must not pay an invoice whose amount isn't the one we asked for. + + invoice_amount is not one of the fields the invoice has to echo from our + invoice_request, so the payee sets it independently. BOLT #12 requires us + to reject the invoice if it doesn't equal the invreq_amount we sent, in + either direction. + """ + plugin = Path(__file__).parent / "plugins" / "xpay_mismatched_invoice_amount.py" + l1, l2 = node_factory.line_graph(2, wait_for_announce=True, + opts=[{'plugin': str(plugin)}, {}]) + + offer = l2.rpc.offer('any')['bolt12'] + + # Genuine, validly-signed invoices from l2, for amounts we won't ask for. + # Fetch both before arming the plugin, since it intercepts fetchinvoice. + larger = l1.rpc.fetchinvoice(offer, 10000000)['invoice'] + smaller = l1.rpc.fetchinvoice(offer, 50000)['invoice'] + + before = only_one(l1.rpc.listpeerchannels()['channels'])['to_us_msat'] + for inv, amount_msat in ((larger, 10000000), (smaller, 50000)): + l1.rpc.call('setinvoiceamount', {'invoice': inv, + 'amount_msat': amount_msat}) + with pytest.raises(RpcError, match=r"Invoice amount"): + l1.rpc.xpay(offer, 100000) + after = only_one(l1.rpc.listpeerchannels()['channels'])['to_us_msat'] + assert before == after + + # We send no invreq_amount when the offer has its own amount, so here the + # offer amount is what we authorized. + fixed = l2.rpc.offer('100000msat', 'fixed amount offer')['bolt12'] + l1.rpc.call('setinvoiceamount', {'invoice': larger, + 'amount_msat': 10000000}) + with pytest.raises(RpcError, match=r"Invoice amount"): + l1.rpc.xpay(fixed) + after = only_one(l1.rpc.listpeerchannels()['channels'])['to_us_msat'] + assert before == after + + def test_xpay_circular_routehint(node_factory): """Test that xpay gracefully skips a circular bolt11 routehint (src == dst).""" l1, l2 = node_factory.line_graph(2)