lightningd: fix up malformed onion handling.

In the case where we can't decrypt the onion, we can't fail it in the
normal way (which is encrypted using the onion shared secret), we need
to respond with a update_fail_malformed_htlc message.

Moreover, we need to remember this for persistence.  This means that
we really have three conclusions for an HTLC: fulfilled, failed,
malformed.  Fix up the logic everywhere which assumed failed or
fulfilled.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

daemon/htlc.h

@@ -71,6 +71,8 @@ struct htlc {
 	/* Previous HTLC (if any) which made us offer this (LOCAL only) */
 	struct htlc *src;
 	const u8 *fail;
+	/* FIXME: actually an enum onion_type */
+	u8 malformed;
 };
 
 const char *htlc_state_name(enum htlc_state s);

lightningd/channel.c

@@ -67,7 +67,8 @@ static void dump_htlc(const struct htlc *htlc, const char *prefix)
 		     htlc->id,
 		     htlc_state_name(htlc->state),
 		     htlc_state_name(remote_state),
-		     htlc->r ? "FULFILLED" : htlc->fail ? "FAILED" : "");
+		     htlc->r ? "FULFILLED" : htlc->fail ? "FAILED" :
+		     htlc->malformed ? "MALFORMED" : "");
 }
 
 void dump_htlcs(const struct channel *channel, const char *prefix)
@@ -384,6 +385,7 @@ static enum channel_add_err add_htlc(struct channel *channel,
 
 	htlc->rhash = *payment_hash;
 	htlc->fail = NULL;
+	htlc->malformed = 0;
 	htlc->r = NULL;
 	htlc->routing = tal_dup_arr(htlc, u8, routing, TOTAL_PACKET_SIZE, 0);
 
@@ -815,7 +817,7 @@ static bool adjust_balance(struct channel *channel, struct htlc *htlc)
 		if (htlc_has(htlc, HTLC_FLAG(side, HTLC_F_COMMITTED)))
 			continue;
 
-		if (!htlc->fail && !htlc->r) {
+		if (!htlc->fail && !htlc->malformed && !htlc->r) {
 			status_trace("%s HTLC %"PRIu64
 				     " %s neither fail nor fulfull?",
 				     htlc_state_owner(htlc->state) == LOCAL
@@ -943,6 +945,12 @@ bool channel_force_htlcs(struct channel *channel,
 				     failed[i].id);
 			return false;
 		}
+		if (htlc->malformed) {
+			status_trace("Fail %s HTLC %"PRIu64" already malformed",
+				     failed_sides[i] == LOCAL ? "out" : "in",
+				     failed[i].id);
+			return false;
+		}
 		if (!htlc_has(htlc, HTLC_REMOVING)) {
 			status_trace("Fail %s HTLC %"PRIu64" state %s",
 				     failed_sides[i] == LOCAL ? "out" : "in",
@@ -950,8 +958,12 @@ bool channel_force_htlcs(struct channel *channel,
 				     htlc_state_name(htlc->state));
 			return false;
 		}
+		if (failed[i].malformed)
+			htlc->malformed = failed[i].malformed;
+		else
 			htlc->fail = tal_dup_arr(htlc, u8, failed[i].failreason,
-					 tal_len(failed[i].failreason), 0);
+						 tal_len(failed[i].failreason),
+						 0);
 	}
 
 	for (i = 0; i < tal_count(htlcs); i++) {

lightningd/channel/channel.c

@@ -663,6 +663,7 @@ static u8 *got_commitsig_msg(const tal_t *ctx,
 				assert(htlc->fail);
 				f = tal_arr_append(&failed);
 				f->id = htlc->id;
+				f->malformed = htlc->malformed;
 				f->failreason = cast_const(u8 *, htlc->fail);
 			}
 		} else {
@@ -1472,23 +1473,39 @@ static void handle_fail(struct peer *peer, const u8 *inmsg)
 	u8 *msg;
 	u64 id;
 	u8 *errpkt;
+	u16 malformed;
 	enum channel_remove_err e;
 
-	if (!fromwire_channel_fail_htlc(inmsg, inmsg, NULL, &id, &errpkt))
+	if (!fromwire_channel_fail_htlc(inmsg, inmsg, NULL, &id, &malformed,
+					&errpkt))
 		status_failed(WIRE_CHANNEL_BAD_COMMAND,
 			      "Invalid channel_fail_htlc");
 
+	if (malformed && !(malformed & BADONION))
+		status_failed(WIRE_CHANNEL_BAD_COMMAND,
+			      "Invalid channel_fail_htlc: bad malformed 0x%x",
+			      malformed);
+
 	e = channel_fail_htlc(peer->channel, REMOTE, id);
 	switch (e) {
 	case CHANNEL_ERR_REMOVE_OK:
+		if (malformed) {
+			struct htlc *h;
+			struct sha256 sha256_of_onion;
+			h = channel_get_htlc(peer->channel, REMOTE, id);
+			sha256(&sha256_of_onion, h->routing,
+			       tal_len(h->routing));
+			msg = towire_update_fail_malformed_htlc(peer,
+							&peer->channel_id,
+							id, &sha256_of_onion,
+							malformed);
+		} else {
 			msg = towire_update_fail_htlc(peer, &peer->channel_id,
 						      id, errpkt);
+		}
 		msg_enqueue(&peer->peer_out, take(msg));
 		start_commit_timer(peer);
 		return;
-	/* These shouldn't happen, because any offered HTLC (which would give
-	 * us the preimage) should have timed out long before.  If we
-	 * were to get preimages from other sources, this could happen. */
 	case CHANNEL_ERR_NO_SUCH_ID:
 	case CHANNEL_ERR_ALREADY_FULFILLED:
 	case CHANNEL_ERR_HTLC_UNCOMMITTED:

lightningd/channel/channel_wire.csv

@@ -90,6 +90,9 @@ channel_fulfill_htlc,,payment_preimage,struct preimage
 # Main daemon says HTLC failed
 channel_fail_htlc,6
 channel_fail_htlc,,id,8
+# If malformed is non-zero, it's a BADONION code
+channel_fail_htlc,,malformed,u16
+# Otherwise, error_pkt contains failreason.
 channel_fail_htlc,,len,2
 channel_fail_htlc,,error_pkt,len*u8
 

lightningd/htlc_end.c

@@ -81,6 +81,8 @@ struct htlc_in *htlc_in_check(const struct htlc_in *hin, const char *abortstr)
 			       htlc_state_name(hin->hstate));
 	else if (hin->failuremsg && hin->preimage)
 		return corrupt(hin, abortstr, "Both failed and succeeded");
+	else if (hin->failuremsg && hin->malformed)
+		return corrupt(hin, abortstr, "Both failed and malformed");
 
 	return cast_const(struct htlc_in *, hin);
 }
@@ -105,6 +107,7 @@ struct htlc_in *new_htlc_in(const tal_t *ctx,
 
 	hin->hstate = RCVD_ADD_COMMIT;
 	hin->failuremsg = NULL;
+	hin->malformed = 0;
 	hin->preimage = NULL;
 
 	return htlc_in_check(hin, "new_htlc_in");
@@ -147,6 +150,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
 
 	hout->hstate = SENT_ADD_HTLC;
 	hout->failuremsg = NULL;
+	hout->malformed = 0;
 	hout->preimage = NULL;
 
 	hout->in = in;

lightningd/htlc_end.h

@@ -5,6 +5,7 @@
 #include <ccan/short_types/short_types.h>
 #include <daemon/htlc_state.h>
 #include <lightningd/sphinx.h>
+#include <wire/gen_onion_wire.h>
 
 /* We look up HTLCs by peer & id */
 struct htlc_key {
@@ -27,9 +28,12 @@ struct htlc_in {
 	/* Shared secret for us to send any failure message. */
 	struct secret shared_secret;
 
-	/* If they failed HTLC, here's the message. */
+	/* If we failed HTLC, here's the message. */
 	const u8 *failuremsg;
 
+	/* If it was malformed, here's the error. */
+	enum onion_type malformed;
+
 	/* If they fulfilled, here's the preimage. */
 	struct preimage *preimage;
 };
@@ -48,6 +52,9 @@ struct htlc_out {
 	/* If we failed HTLC, here's the message. */
 	const u8 *failuremsg;
 
+	/* If it was malformed, here's the error. */
+	enum onion_type malformed;
+
 	/* If we fulfilled, here's the preimage. */
 	struct preimage *preimage;
 

lightningd/htlc_wire.c

@@ -22,6 +22,7 @@ void towire_fulfilled_htlc(u8 **pptr, const struct fulfilled_htlc *fulfilled)
 void towire_failed_htlc(u8 **pptr, const struct failed_htlc *failed)
 {
 	towire_u64(pptr, failed->id);
+	towire_u16(pptr, failed->malformed);
 	towire_u16(pptr, tal_count(failed->failreason));
 	towire_u8_array(pptr, failed->failreason, tal_count(failed->failreason));
 }
@@ -66,6 +67,7 @@ void fromwire_failed_htlc(const tal_t *ctx, const u8 **cursor, size_t *max,
 	u16 failreason_len;
 
 	failed->id = fromwire_u64(cursor, max);
+	failed->malformed = fromwire_u16(cursor, max);
 	failreason_len = fromwire_u16(cursor, max);
 	failed->failreason = tal_arr(ctx, u8, failreason_len);
 	fromwire_u8_array(cursor, max, failed->failreason, failreason_len);

lightningd/htlc_wire.h

@@ -5,6 +5,7 @@
 #include <ccan/short_types/short_types.h>
 #include <daemon/htlc.h>
 #include <lightningd/sphinx.h>
+#include <wire/gen_onion_wire.h>
 
 /* These are how we communicate about HTLC state to the master daemon */
 struct added_htlc {
@@ -22,6 +23,7 @@ struct fulfilled_htlc {
 
 struct failed_htlc {
 	u64 id;
+	enum onion_type malformed;
 	u8 *failreason;
 };
 

lightningd/pay.c

@@ -76,6 +76,9 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout)
 	enum onion_type failcode;
 	struct onionreply *reply;
 
+	if (hout->malformed)
+		failcode = hout->malformed;
+	else {
 		reply = unwrap_onionreply(pc, pc->path_secrets,
 					  tal_count(pc->path_secrets),
 					  hout->failuremsg);
@@ -93,6 +96,7 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout)
 				 reply->origin_index,
 				 failcode, onion_type_name(failcode));
 		}
+	}
 
 	/* FIXME: save ids we can turn reply->origin_index into sender. */
 

lightningd/peer_htlcs.c

@@ -31,20 +31,30 @@ static void save_htlc_stub(struct lightningd *ld,
 /* This obfuscates the message, whether local or forwarded. */
 static void relay_htlc_failmsg(struct htlc_in *hin)
 {
-	u8 *reply;
-
 	if (!hin->key.peer->owner)
 		return;
 
-	reply = wrap_onionreply(hin, &hin->shared_secret, hin->failuremsg);
+	if (hin->malformed) {
+		subd_send_msg(hin->key.peer->owner,
+			      take(towire_channel_fail_htlc(hin,
+							    hin->key.id,
+							    hin->malformed,
+							    NULL)));
+	} else {
+		u8 *reply;
+
+		reply = wrap_onionreply(hin, &hin->shared_secret,
+					hin->failuremsg);
 		subd_send_msg(hin->key.peer->owner,
-		      take(towire_channel_fail_htlc(hin, hin->key.id, reply)));
+			      take(towire_channel_fail_htlc(hin, hin->key.id,
+							    0, reply)));
 		tal_free(reply);
+	}
 }
 
 static u8 *make_failmsg(const tal_t *ctx, const struct htlc_in *hin,
 			enum onion_type failcode,
-			const struct sha256 *onion_sha, const u8 *channel_update)
+			const u8 *channel_update)
 {
 	switch (failcode) {
 	case WIRE_INVALID_REALM:
@@ -55,12 +65,6 @@ static u8 *make_failmsg(const tal_t *ctx, const struct htlc_in *hin,
 		return towire_permanent_node_failure(ctx);
 	case WIRE_REQUIRED_NODE_FEATURE_MISSING:
 		return towire_required_node_feature_missing(ctx);
-	case WIRE_INVALID_ONION_VERSION:
-		return towire_invalid_onion_version(ctx, onion_sha);
-	case WIRE_INVALID_ONION_HMAC:
-		return towire_invalid_onion_hmac(ctx, onion_sha);
-	case WIRE_INVALID_ONION_KEY:
-		return towire_invalid_onion_key(ctx, onion_sha);
 	case WIRE_TEMPORARY_CHANNEL_FAILURE:
 		return towire_temporary_channel_failure(ctx, channel_update);
 	case WIRE_CHANNEL_DISABLED:
@@ -91,26 +95,35 @@ static u8 *make_failmsg(const tal_t *ctx, const struct htlc_in *hin,
 		return towire_final_incorrect_cltv_expiry(ctx, 0);
 	case WIRE_FINAL_INCORRECT_HTLC_AMOUNT:
 		return towire_final_incorrect_htlc_amount(ctx, hin->msatoshi);
+	case WIRE_INVALID_ONION_VERSION:
+	case WIRE_INVALID_ONION_HMAC:
+	case WIRE_INVALID_ONION_KEY:
+		log_broken(hin->key.peer->log, "Bad failmsg for %s",
+			   onion_type_name(failcode));
 	}
 	abort();
 }
 
-static void fail_htlc(struct htlc_in *hin, enum onion_type failcode,
-		      const struct sha256 *onion_sha)
+static void fail_htlc(struct htlc_in *hin, enum onion_type failcode)
 {
-	u8 *msg;
-
-	log_broken(hin->key.peer->log, "failed htlc %"PRIu64" code 0x%04x (%s)",
+	log_info(hin->key.peer->log, "failed htlc %"PRIu64" code 0x%04x (%s)",
 		 hin->key.id, failcode, onion_type_name(failcode));
 
+	if (failcode & BADONION)
+		hin->malformed = failcode;
+	else {
+		u8 *msg;
+
 		if (failcode & UPDATE) {
 			/* FIXME: Ask gossip daemon for channel_update. */
 		}
 
-	msg = make_failmsg(hin, hin, failcode, onion_sha, NULL);
-	hin->failuremsg = create_onionreply(hin, &hin->shared_secret, msg);
+		msg = make_failmsg(hin, hin, failcode, NULL);
+		hin->failuremsg = create_onionreply(hin, &hin->shared_secret,
+						    msg);
 		tal_free(msg);
-
+	}
+	htlc_in_check(hin, __func__);
 	relay_htlc_failmsg(hin);
 }
 
@@ -269,7 +282,7 @@ static void handle_localpay(struct htlc_in *hin,
 	return;
 
 fail:
-	fail_htlc(hin, failcode, NULL);
+	fail_htlc(hin, failcode);
 }
 
 /*
@@ -283,7 +296,7 @@ static void hend_subd_died(struct htlc_out *hout)
 		  "Failing HTLC %"PRIu64" due to peer death",
 		  hout->in->key.id);
 
-	fail_htlc(hout->in, WIRE_TEMPORARY_CHANNEL_FAILURE, NULL);
+	fail_htlc(hout->in, WIRE_TEMPORARY_CHANNEL_FAILURE);
 }
 
 /* This is where channeld gives us the HTLC id, and also reports if it
@@ -309,7 +322,7 @@ static bool rcvd_htlc_reply(struct subd *subd, const u8 *msg, const int *fds,
 			  onion_type_name(failure_code),
 			  (int)tal_len(failurestr), (char *)failurestr);
 
-		fail_htlc(hout->in, failure_code, NULL);
+		fail_htlc(hout->in, failure_code);
 		return true;
 	}
 
@@ -427,7 +440,7 @@ static void forward_htlc(struct htlc_in *hin,
 	return;
 
 fail:
-	fail_htlc(hin, failcode, NULL);
+	fail_htlc(hin, failcode);
 }
 
 /* Temporary information, while we resolve the next hop */
@@ -454,7 +467,7 @@ static bool channel_resolve_reply(struct subd *gossip, const u8 *msg,
 	}
 
 	if (tal_count(nodes) == 0) {
-		fail_htlc(gr->hin, WIRE_UNKNOWN_NEXT_PEER, NULL);
+		fail_htlc(gr->hin, WIRE_UNKNOWN_NEXT_PEER);
 		return true;
 	} else if (tal_count(nodes) != 2) {
 		log_broken(gossip->log,
@@ -665,7 +678,11 @@ static bool peer_failed_our_htlc(struct peer *peer,
 	if (!htlc_out_update_state(peer, hout, RCVD_REMOVE_COMMIT))
 		return false;
 
-	log_debug(peer->log, "Our HTLC %"PRIu64" failed", failed->id);
+	log_debug(peer->log, "Our HTLC %"PRIu64" failed (%u)", failed->id,
+		  failed->malformed);
+	if (failed->malformed)
+		hout->malformed = failed->malformed;
+	else
 		hout->failuremsg = tal_dup_arr(hout, u8, failed->failreason,
 					       tal_len(failed->failreason), 0);
 	htlc_out_check(hout, __func__);
@@ -675,11 +692,13 @@ static bool peer_failed_our_htlc(struct peer *peer,
 static void remove_htlc_in(struct peer *peer, struct htlc_in *hin)
 {
 	htlc_in_check(hin, __func__);
-	assert(hin->failuremsg || hin->preimage);
+	assert(hin->failuremsg || hin->preimage || hin->malformed);
 
 	log_debug(peer->log, "Removing in HTLC %"PRIu64" state %s %s",
 		  hin->key.id, htlc_state_name(hin->hstate),
-		  hin->failuremsg ? "FAILED" : "FULFILLED");
+		  hin->failuremsg ? "FAILED"
+		  : hin->malformed ? "MALFORMED"
+		  : "FULFILLED");
 
 	/* If we fulfilled their HTLC, credit us. */
 	if (hin->preimage) {
@@ -695,18 +714,25 @@ static void remove_htlc_in(struct peer *peer, struct htlc_in *hin)
 static void remove_htlc_out(struct peer *peer, struct htlc_out *hout)
 {
 	htlc_out_check(hout, __func__);
-	assert(hout->failuremsg || hout->preimage);
+	assert(hout->failuremsg || hout->preimage || hout->malformed);
 	log_debug(peer->log, "Removing out HTLC %"PRIu64" state %s %s",
 		  hout->key.id, htlc_state_name(hout->hstate),
-		  hout->failuremsg ? "FAILED" : "FULFILLED");
+		  hout->failuremsg ? "FAILED"
+		  : hout->malformed ? "MALFORMED"
+		  : "FULFILLED");
 
 	/* If it's failed, now we can forward since it's completely locked-in */
-	if (hout->failuremsg) {
+	if (!hout->preimage) {
 		if (hout->in) {
+			if (hout->in->malformed)
+				hout->malformed = hout->in->malformed;
+			else {
 				hout->in->failuremsg
 					= tal_dup_arr(hout->in, u8,
 						      hout->failuremsg,
-					      tal_len(hout->failuremsg), 0);
+						      tal_len(hout->failuremsg),
+						      0);
+			}
 			relay_htlc_failmsg(hout->in);
 		} else {
 			payment_failed(peer->ld, hout);
@@ -1072,16 +1098,13 @@ int peer_got_revoke(struct peer *peer, const u8 *msg)
 
 	/* Now, any HTLCs we need to immediately fail? */
 	for (i = 0; i < tal_count(changed); i++) {
-		struct sha256 bad_onion_sha;
 		struct htlc_in *hin;
 
 		if (!failcodes[i])
 			continue;
 
 		hin = find_htlc_in(&peer->ld->htlcs_in, peer, changed[i].id);
-		sha256(&bad_onion_sha, hin->onion_routing_packet,
-		       sizeof(hin->onion_routing_packet));
-		fail_htlc(hin, failcodes[i], &bad_onion_sha);
+		fail_htlc(hin, failcodes[i]);
 	}
 	return 0;
 }