feerates: bound peer-supplied and stored values (v26.06.7 security, 1/7)

bitcoin/feerate.c

@@ -12,8 +12,15 @@ u32 feerate_from_style(u32 feerate, enum feerate_style style)
 		return feerate;
 	case FEERATE_PER_KBYTE:
 		/* Everyone uses satoshi per kbyte, but we use satoshi per ksipa
-		 * (don't round down to zero though)! */
-		return (feerate + 3) / 4;
+		 * (don't round down to zero though)!
+		 *
+		 * Widen before rounding up: on a u32 the +3 wraps for the top
+		 * three values, turning an absurd feerate into 0 or 1 perkw.
+		 * That is the dangerous direction (it underpays our unilateral
+		 * close), and it slips under every bound we check afterwards,
+		 * since those are applied to the converted value.  The result
+		 * always fits a u32: (UINT_MAX + 3) / 4 < UINT_MAX. */
+		return ((u64)feerate + 3) / 4;
 	}
 	abort();
 }
@@ -31,6 +38,30 @@ u32 feerate_to_style(u32 feerate_perkw, enum feerate_style style)
 	abort();
 }
 
+bool next_funding_feerate(u32 last_feerate, u32 *next_feerate)
+{
+	u64 next;
+
+	/* Not a feerate we could ever have proposed, and 25/24 of it is
+	 * still 0. */
+	if (last_feerate == 0)
+		return false;
+
+	/* Widen: anything above UINT_MAX/25 overflows a u32 here, and that
+	 * is exactly the range a broken fee estimator can leave in the db. */
+	next = (u64)last_feerate * 25 / 24;
+	if (next > UINT_MAX)
+		return false;
+
+	/* Rounding down means feerates below 24 map back onto themselves,
+	 * and the rule requires strictly more to be a valid bump. */
+	if (next <= last_feerate)
+		return false;
+
+	*next_feerate = next;
+	return true;
+}
+
 const char *feerate_style_name(enum feerate_style style)
 {
 	switch (style) {

bitcoin/feerate.h

@@ -33,6 +33,29 @@
  */
 #define FEERATE_FLOOR 253
 
+/*
+ * Sanity ceiling on any feerate estimate entering lightningd (sat/kw).
+ * This is a sanity bound rather than a policy limit: an estimate above it
+ * means a broken fee source rather than an expensive mempool, so it sits
+ * far above anything the real chain has ever seen (4000 sat/vB, several
+ * times the historical peak).  Clamping on the way in keeps every
+ * downstream feerate calculation working on a plausible number.
+ */
+#define FEERATE_CEILING 1000000
+
+/*
+ * The most we are ever willing to pay ourselves (sat/kw).
+ *
+ * Unlike FEERATE_CEILING this *is* a policy limit, and the two are
+ * deliberately an order of magnitude apart because they answer different
+ * questions.  FEERATE_CEILING bounds what we let a peer drive us to: their
+ * estimator being broken is not by itself worth dropping a channel over, so
+ * it only has to exclude the absurd.  This one bounds what we propose with
+ * our own money, where we can simply decline: 400 sat/vB is around 0.011 BTC
+ * for a bare anchor commitment, which we would rather not spend by accident.
+ */
+#define MAX_OUR_FEERATE_PER_KW 100000
+
 enum feerate_style {
 	FEERATE_PER_KSIPA,
 	FEERATE_PER_KBYTE
@@ -57,4 +80,16 @@ u32 feerate_from_style(u32 feerate, enum feerate_style style);
 u32 feerate_to_style(u32 feerate_perkw, enum feerate_style style);
 const char *feerate_style_name(enum feerate_style style);
 
+/* Sets *next_feerate to the smallest feerate which satisfies the BOLT #2
+ * rule that the next funding transaction pays 25/24 times the feerate of
+ * the previously constructed one, rounded down, and returns true.  Returns
+ * false, leaving *next_feerate untouched, if last_feerate admits no such
+ * value: it is 0, or 25/24 of it does not fit a u32, or rounding down lands
+ * back on last_feerate.
+ *
+ * last_feerate is generally read back out of the database, where a broken
+ * fee estimator (ours or a peer's) may have left something absurd, so
+ * callers must handle false rather than assume it away. */
+bool next_funding_feerate(u32 last_feerate, u32 *next_feerate);
+
 #endif /* LIGHTNING_BITCOIN_FEERATE_H */

channeld/channeld.c

@@ -11,6 +11,7 @@
  *    limits, unlikely as that is.
  */
 #include "config.h"
+#include <bitcoin/feerate.h>
 #include <bitcoin/script.h>
 #include <ccan/asort/asort.h>
 #include <ccan/cast/cast.h>
@@ -82,6 +83,14 @@ struct peer {
 	/* Tolerable amounts for feerate (only relevant for fundee). */
 	u32 feerate_min, feerate_max;
 
+	/* The most we're prepared to pay ourselves: stricter than
+	 * feerate_max, which is what we'll tolerate from them. */
+	u32 our_feerate_max;
+
+	/* Set by --ignore-fee-limits or dev-ignore-fee-limits: drop the
+	 * policy bounds above (but never the sanity ceiling). */
+	bool ignore_fee_limits;
+
 	/* Feerate to be used when creating penalty transactions. */
 	u32 feerate_penalty;
 
@@ -690,6 +699,32 @@ static void handle_peer_add_htlc(struct peer *peer, const u8 *msg)
 				 channel_add_err_name(add_err));
 }
 
+/* Ignoring the fee limits drops the policy bounds, but never the sanity
+ * ceiling: a feerate above that means a broken fee source, and whatever we
+ * accept here we go on to store. */
+static u32 accepted_feerate_min(const struct peer *peer)
+{
+	if (peer->ignore_fee_limits)
+		return 1;
+	return peer->feerate_min;
+}
+
+static u32 accepted_feerate_max(const struct peer *peer)
+{
+	if (peer->ignore_fee_limits)
+		return FEERATE_CEILING;
+	return peer->feerate_max;
+}
+
+/* The most we'll pay ourselves, as opposed to what we'll put up with
+ * from them. */
+static u32 proposed_feerate_max(const struct peer *peer)
+{
+	if (peer->ignore_fee_limits)
+		return FEERATE_CEILING;
+	return peer->our_feerate_max;
+}
+
 /* We don't get upset if they're outside the range, as long as they're
  * improving (or at least, not getting worse!). */
 static bool feerate_same_or_better(const struct channel *channel,
@@ -728,7 +763,8 @@ static void handle_peer_feechange(struct peer *peer, const u8 *msg)
 				 "update_fee from non-opener?");
 
 	status_debug("update_fee %u, range %u-%u",
-		     feerate, peer->feerate_min, peer->feerate_max);
+		     feerate, accepted_feerate_min(peer),
+		     accepted_feerate_max(peer));
 
 	/* BOLT #2:
 	 *
@@ -739,12 +775,14 @@ static void handle_peer_feechange(struct peer *peer, const u8 *msg)
 	 *       `error` and fail the channel.
 	 */
 	if (!feerate_same_or_better(peer->channel, feerate,
-				    peer->feerate_min, peer->feerate_max))
+				    accepted_feerate_min(peer),
+				    accepted_feerate_max(peer)))
 		peer_failed_warn(peer->pps, &peer->channel_id,
 				 "update_fee %u outside range %u-%u"
 				 " (currently %u)",
 				 feerate,
-				 peer->feerate_min, peer->feerate_max,
+				 accepted_feerate_min(peer),
+				 accepted_feerate_max(peer),
 				 channel_feerate(peer->channel, LOCAL));
 
 	/* BOLT #2:
@@ -1928,8 +1966,10 @@ static void check_tx_abort(struct peer *peer, const u8 *msg, struct bitcoin_txid
 	exit(0);
 }
 
-static void splice_abort(struct peer *peer, struct inflight *inflight,
-			 const char *fmt, ...)
+/* Sends tx_abort, waits for their ack, tells master, and exits: callers rely
+ * on this not returning (check_balances falls through to further checks). */
+static NORETURN void splice_abort(struct peer *peer, struct inflight *inflight,
+				  const char *fmt, ...)
 {
 	struct bitcoin_outpoint *outpoint;
 	u8 *msg;
@@ -3599,10 +3639,18 @@ static struct amount_sat check_balances(struct peer *peer,
 
 	/* As a safeguard max feerate is checked (only) locally, if it's
 	 * particularly high we fail and tell the user but allow them to
-	 * override with `splice_force_feerate` */
-	max_accepter_fee = amount_tx_fee(peer->feerate_max,
+	 * override with `splice_force_feerate`.
+	 *
+	 * Whichever side is ours is held to what we're prepared to pay; the
+	 * other side is their money, so it only has to clear the looser
+	 * bound we apply to anything they propose. */
+	max_accepter_fee = amount_tx_fee(opener
+					 ? accepted_feerate_max(peer)
+					 : proposed_feerate_max(peer),
 					 calc_weight(TX_ACCEPTER, psbt, false));
-	max_initiator_fee = amount_tx_fee(peer->feerate_max,
+	max_initiator_fee = amount_tx_fee(opener
+					  ? proposed_feerate_max(peer)
+					  : accepted_feerate_max(peer),
 					  calc_weight(TX_INITIATOR, psbt, opener));
 
 	if (opener) {
@@ -4277,9 +4325,27 @@ static void splice_accepter(struct peer *peer, const u8 *inmsg)
 		       &peer->channel->funding_pubkey[REMOTE]))
 		status_info("Splice peer is rotating funding pubkey");
 
-	if (funding_feerate_perkw < peer->feerate_min)
+	/* They initiated, so it's their fee: the looser bound applies.
+	 *
+	 * We disconnect rather than tx_abort here.  A tx_abort has to be
+	 * acked, and splice_abort() blocks reading until it is: a peer that
+	 * proposes a nonsense feerate and then goes silent would leave us
+	 * parked in that read with the channel quiesced in STFU.  Since the
+	 * bound is FEERATE_CEILING, a peer reaching it is not disagreeing
+	 * with us about the mempool, they are broken. */
+	if (funding_feerate_perkw < accepted_feerate_min(peer))
+		peer_failed_warn(peer->pps, &peer->channel_id,
+				 "Splice feerate_perkw %u is below our"
+				 " minimum %u",
+				 funding_feerate_perkw,
+				 accepted_feerate_min(peer));
+
+	if (funding_feerate_perkw > accepted_feerate_max(peer))
 		peer_failed_warn(peer->pps, &peer->channel_id,
-				 "Splice feerate_perkw is too low");
+				 "Splice feerate_perkw %u is above our"
+				 " maximum %u",
+				 funding_feerate_perkw,
+				 accepted_feerate_max(peer));
 
 	/* TODO: Add plugin hook for user to adjust accepter amount */
 	peer->splicing->accepter_relative = 0;
@@ -5014,14 +5080,30 @@ static void handle_splice_init(struct peer *peer, const u8 *inmsg)
 		wire_sync_write(MASTER_FD, take(msg));
 		return;
 	}
-	if (peer->splicing->feerate_per_kw < peer->feerate_min) {
+	if (peer->splicing->feerate_per_kw < accepted_feerate_min(peer)) {
 		msg = towire_channeld_splice_state_error(NULL, tal_fmt(tmpctx,
 							 "Feerate %u is too"
 							 " low. Lower than"
 							 " channel feerate_min"
 							 " %u",
 							 peer->splicing->feerate_per_kw,
-							 peer->feerate_min));
+							 accepted_feerate_min(peer)));
+		wire_sync_write(MASTER_FD, take(msg));
+		return;
+	}
+	/* We initiated, so this is our money: hold it to what we're
+	 * prepared to pay, not to what we'd tolerate from them.  Like the
+	 * fee check in check_balances, `force_feerate` is the user saying
+	 * they meant it: this is a policy limit, not a safety one. */
+	if (!peer->splicing->force_feerate
+	    && peer->splicing->feerate_per_kw > proposed_feerate_max(peer)) {
+		msg = towire_channeld_splice_state_error(NULL, tal_fmt(tmpctx,
+						 "Feerate %u is too"
+						 " high. Higher than the most"
+						 " we'll pay ourselves"
+						 " %u",
+						 peer->splicing->feerate_per_kw,
+						 proposed_feerate_max(peer)));
 		wire_sync_write(MASTER_FD, take(msg));
 		return;
 	}
@@ -6492,6 +6574,8 @@ static void handle_feerates(struct peer *peer, const u8 *inmsg)
 					&feerate,
 				        &peer->feerate_min,
 				        &peer->feerate_max,
+				        &peer->our_feerate_max,
+				        &peer->ignore_fee_limits,
 				        &peer->feerate_penalty,
 				        &peer->feerate_opening,
 				        &peer->feerate_splice))
@@ -6865,6 +6949,8 @@ static void init_channel(struct peer *peer)
 				    &peer->feerate_splice,
 				    &peer->feerate_min,
 				    &peer->feerate_max,
+				    &peer->our_feerate_max,
+				    &peer->ignore_fee_limits,
 				    &peer->feerate_penalty,
 				    &peer->feerate_opening,
 				    &peer->their_commit_sig,
@@ -6956,7 +7042,7 @@ static void init_channel(struct peer *peer)
 		     peer->next_index[LOCAL], peer->next_index[REMOTE],
 		     peer->revocations_received,
 		     fmt_fee_states(tmpctx, fee_states),
-		     peer->feerate_min, peer->feerate_max,
+		     accepted_feerate_min(peer), accepted_feerate_max(peer),
 		     fmt_height_states(tmpctx, blockheight_states),
 		     peer->our_blockheight);
 

channeld/channeld_wire.csv

@@ -31,6 +31,8 @@ msgdata,channeld_init,fee_states,fee_states,
 msgdata,channeld_init,feerate_splice,u32,
 msgdata,channeld_init,feerate_min,u32,
 msgdata,channeld_init,feerate_max,u32,
+msgdata,channeld_init,our_feerate_max,u32,
+msgdata,channeld_init,ignore_fee_limits,bool,
 msgdata,channeld_init,feerate_penalty,u32,
 msgdata,channeld_init,feerate_opening,u32,
 msgdata,channeld_init,first_commit_sig,bitcoin_signature,
@@ -331,6 +333,8 @@ msgtype,channeld_feerates,1027
 msgdata,channeld_feerates,feerate,u32,
 msgdata,channeld_feerates,min_feerate,u32,
 msgdata,channeld_feerates,max_feerate,u32,
+msgdata,channeld_feerates,our_max_feerate,u32,
+msgdata,channeld_feerates,ignore_fee_limits,bool,
 msgdata,channeld_feerates,penalty_feerate,u32,
 msgdata,channeld_feerates,opening_feerate,u32,
 msgdata,channeld_feerates,feerate_splice,u32,

closingd/closingd.c

@@ -142,13 +142,22 @@ static void send_offer(struct per_peer_state *pps,
 		       struct amount_sat our_dust_limit,
 		       struct amount_sat fee_to_offer,
 		       const struct bitcoin_outpoint *wrong_funding,
-		       const struct tlv_closing_signed_tlvs_fee_range *tlv_fees)
+		       const struct tlv_closing_signed_tlvs_fee_range *tlv_fees,
+		       struct amount_sat max_fee_to_accept)
 {
 	struct bitcoin_tx *tx;
 	struct bitcoin_signature our_sig;
 	struct tlv_closing_signed_tlvs *close_tlvs;
 	u8 *msg;
 
+	/* We can arrive here in multiple ways, so add a final sanity check
+	 * that we did not go over our max fee */
+	if (amount_sat_greater(fee_to_offer, max_fee_to_accept))
+		peer_failed_warn(pps, channel_id, "Fee %s became larger than our"
+				 " max fee %s",
+				 fmt_amount_sat(tmpctx, fee_to_offer),
+				 fmt_amount_sat(tmpctx, max_fee_to_accept));
+
 	/* BOLT #2:
 	 *
 	 *   - MUST set `signature` to the Bitcoin signature of the close
@@ -731,7 +740,8 @@ static void do_quickclose(struct amount_sat offer[NUM_SIDES],
 				   our_dust_limit,
 				   offer[LOCAL],
 				   wrong_funding,
-				   our_feerange);
+				   our_feerange,
+				   our_feerange->max_fee_satoshis);
 		}
 	} else {
 		/* BOLT #2:
@@ -767,7 +777,8 @@ static void do_quickclose(struct amount_sat offer[NUM_SIDES],
 			   our_dust_limit,
 			   offer[LOCAL],
 			   wrong_funding,
-			   our_feerange);
+			   our_feerange,
+			   our_feerange->max_fee_satoshis);
 
 		/* They will reply unless we completely agreed. */
 		if (!amount_sat_eq(offer[LOCAL], offer[REMOTE])) {
@@ -941,7 +952,8 @@ int main(int argc, char *argv[])
 				   our_dust_limit,
 				   offer[LOCAL],
 				   wrong_funding,
-				   our_feerange);
+				   our_feerange,
+				   max_fee_to_accept);
 		} else {
 			if (i == 0)
 				peer_billboard(false, "Waiting for their initial"
@@ -1011,7 +1023,8 @@ int main(int argc, char *argv[])
 				   our_dust_limit,
 				   offer[LOCAL],
 				   wrong_funding,
-				   our_feerange);
+				   our_feerange,
+				   max_fee_to_accept);
 		} else {
 			peer_billboard(false, "Waiting for another"
 				       " closing fee offer:"