From 3f65f0f0008c9a1395c0237e0217110cb435ec4f Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 20 Jun 2017 15:37:03 +0930 Subject: [PATCH] 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 --- daemon/htlc.h | 2 + lightningd/channel.c | 20 ++++- lightningd/channel/channel.c | 29 +++++-- lightningd/channel/channel_wire.csv | 3 + lightningd/htlc_end.c | 4 + lightningd/htlc_end.h | 9 ++- lightningd/htlc_wire.c | 2 + lightningd/htlc_wire.h | 2 + lightningd/pay.c | 36 +++++---- lightningd/peer_htlcs.c | 115 +++++++++++++++++----------- 10 files changed, 149 insertions(+), 73 deletions(-) diff --git a/daemon/htlc.h b/daemon/htlc.h index 7a6e959a1..1f6731fd3 100644 --- a/daemon/htlc.h +++ b/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); diff --git a/lightningd/channel.c b/lightningd/channel.c index 8aa420759..c8058b400 100644 --- a/lightningd/channel.c +++ b/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; } - htlc->fail = tal_dup_arr(htlc, u8, failed[i].failreason, - tal_len(failed[i].failreason), 0); + 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); } for (i = 0; i < tal_count(htlcs); i++) { diff --git a/lightningd/channel/channel.c b/lightningd/channel/channel.c index 3219e8928..5f695d64a 100644 --- a/lightningd/channel/channel.c +++ b/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: - msg = towire_update_fail_htlc(peer, &peer->channel_id, - id, errpkt); + 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: diff --git a/lightningd/channel/channel_wire.csv b/lightningd/channel/channel_wire.csv index db9eab1e7..81786ea93 100644 --- a/lightningd/channel/channel_wire.csv +++ b/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 diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c index 8d152bc0a..124fb11e3 100644 --- a/lightningd/htlc_end.c +++ b/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; diff --git a/lightningd/htlc_end.h b/lightningd/htlc_end.h index 8f2937210..30142c153 100644 --- a/lightningd/htlc_end.h +++ b/lightningd/htlc_end.h @@ -5,6 +5,7 @@ #include #include #include +#include /* 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; diff --git a/lightningd/htlc_wire.c b/lightningd/htlc_wire.c index 3e6430728..e7f7048c0 100644 --- a/lightningd/htlc_wire.c +++ b/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); diff --git a/lightningd/htlc_wire.h b/lightningd/htlc_wire.h index 5c918620c..734f75a1e 100644 --- a/lightningd/htlc_wire.h +++ b/lightningd/htlc_wire.h @@ -5,6 +5,7 @@ #include #include #include +#include /* 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; }; diff --git a/lightningd/pay.c b/lightningd/pay.c index f4b1c317b..d596cd2b1 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -76,22 +76,26 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout) enum onion_type failcode; struct onionreply *reply; - reply = unwrap_onionreply(pc, pc->path_secrets, - tal_count(pc->path_secrets), - hout->failuremsg); - if (!reply) { - log_info(hout->key.peer->log, - "htlc %"PRIu64" failed with bad reply (%s)", - hout->key.id, - tal_hex(pc, hout->failuremsg)); - failcode = WIRE_PERMANENT_NODE_FAILURE; - } else { - failcode = fromwire_peektype(reply->msg); - log_info(hout->key.peer->log, - "htlc %"PRIu64" failed from %ith node with code 0x%04x (%s)", - hout->key.id, - reply->origin_index, - failcode, onion_type_name(failcode)); + if (hout->malformed) + failcode = hout->malformed; + else { + reply = unwrap_onionreply(pc, pc->path_secrets, + tal_count(pc->path_secrets), + hout->failuremsg); + if (!reply) { + log_info(hout->key.peer->log, + "htlc %"PRIu64" failed with bad reply (%s)", + hout->key.id, + tal_hex(pc, hout->failuremsg)); + failcode = WIRE_PERMANENT_NODE_FAILURE; + } else { + failcode = fromwire_peektype(reply->msg); + log_info(hout->key.peer->log, + "htlc %"PRIu64" failed from %ith node with code 0x%04x (%s)", + hout->key.id, + reply->origin_index, + failcode, onion_type_name(failcode)); + } } /* FIXME: save ids we can turn reply->origin_index into sender. */ diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index f626eb73b..c3ba5cc66 100644 --- a/lightningd/peer_htlcs.c +++ b/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); - subd_send_msg(hin->key.peer->owner, - take(towire_channel_fail_htlc(hin, hin->key.id, reply))); - tal_free(reply); + 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, + 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_info(hin->key.peer->log, "failed htlc %"PRIu64" code 0x%04x (%s)", + hin->key.id, failcode, onion_type_name(failcode)); - log_broken(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. */ + if (failcode & UPDATE) { + /* FIXME: Ask gossip daemon for channel_update. */ + } + + msg = make_failmsg(hin, hin, failcode, NULL); + hin->failuremsg = create_onionreply(hin, &hin->shared_secret, + msg); + tal_free(msg); } - - msg = make_failmsg(hin, hin, failcode, onion_sha, 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,9 +678,13 @@ 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); - hout->failuremsg = tal_dup_arr(hout, u8, failed->failreason, - tal_len(failed->failreason), 0); + 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__); return true; } @@ -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) { - hout->in->failuremsg - = tal_dup_arr(hout->in, u8, - hout->failuremsg, - tal_len(hout->failuremsg), 0); + 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); + } 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; }