From 6bf60d47cdf8f309c0026579869704308b07fedf Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Nov 2019 12:46:01 +1030 Subject: [PATCH 1/5] pytest: fix test_gossip_notices_close where we really do inject bad gossip! It currently works because we inject it so fast that it's still doing the txout lookup, but that's about to change. Signed-off-by: Rusty Russell --- tests/test_gossip.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_gossip.py b/tests/test_gossip.py index ef557328efb1..b2fc574cc9cf 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -1094,8 +1094,10 @@ def test_gossipwith(node_factory): def test_gossip_notices_close(node_factory, bitcoind): - # We want IO logging so we can replay a channel_announce to l1. - l1 = node_factory.get_node(options={'log-level': 'io'}) + # We want IO logging so we can replay a channel_announce to l1; + # We also *really* do feed it bad gossip! + l1 = node_factory.get_node(options={'log-level': 'io'}, + allow_bad_gossip=True) l2, l3 = node_factory.line_graph(2) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) # FIXME: sending SIGUSR1 immediately may kill it before handler installed. From 418d7a4d54ddc92484eb55269e9c2099daf8048d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Nov 2019 12:46:09 +1030 Subject: [PATCH 2/5] pytest: clean up test_gossip_notices_close now that gossipwith has more options. And drive-by fix: document that you can now (since e40f07803c64686fb80a02ddfc23e24fca0dea48) use --max-messages=0. Signed-off-by: Rusty Russell --- devtools/gossipwith.c | 2 +- tests/test_gossip.py | 18 +++++++----------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/devtools/gossipwith.c b/devtools/gossipwith.c index 960d1afaf409..0bdf44c5c4c4 100644 --- a/devtools/gossipwith.c +++ b/devtools/gossipwith.c @@ -221,7 +221,7 @@ int main(int argc, char *argv[]) "Stream complete gossip history at start"); opt_register_arg("--max-messages", opt_set_ulongval, opt_show_ulongval, &max_messages, - "Terminate after reading this many messages (> 0)"); + "Terminate after reading this many messages"); opt_register_noarg("--stdin", opt_set_bool, &stream_stdin, "Stream gossip messages from stdin."); opt_register_noarg("--no-init", opt_set_bool, &no_init, diff --git a/tests/test_gossip.py b/tests/test_gossip.py index b2fc574cc9cf..a041763049bb 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -1123,17 +1123,13 @@ def test_gossip_notices_close(node_factory, bitcoind): wait_for(lambda: l1.rpc.listchannels()['channels'] == []) wait_for(lambda: l1.rpc.listnodes()['nodes'] == []) - # FIXME: This is a hack: we should have a framework for canned conversations - # This doesn't naturally terminate, so we give it 5 seconds. - try: - subprocess.run(['devtools/gossipwith', - '{}@localhost:{}'.format(l1.info['id'], l1.port), - channel_announcement, - channel_update, - node_announcement], - timeout=5, stdout=subprocess.PIPE) - except subprocess.TimeoutExpired: - pass + subprocess.run(['devtools/gossipwith', + '--max-messages=0', + '{}@localhost:{}'.format(l1.info['id'], l1.port), + channel_announcement, + channel_update, + node_announcement], + timeout=TIMEOUT) # l1 should reject it. assert(l1.rpc.listchannels()['channels'] == []) From d944d7c94e60c3175c5ffc3084ad3bbd4b7ad158 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Nov 2019 12:46:09 +1030 Subject: [PATCH 3/5] gossipd: use in_txout_failures to do lookup in channel_announcement. This correctly refreshes the txout entry against aging. Signed-off-by: Rusty Russell --- gossipd/routing.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/gossipd/routing.c b/gossipd/routing.c index dc89b2a60130..18b3cc7beb8a 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -1721,9 +1721,8 @@ u8 *handle_channel_announcement(struct routing_state *rstate, } /* If a prior txout lookup failed there is little point it trying - * again. Just drop the announcement and walk away whistling. Any non-0 - * result means this failed before. */ - if (uintmap_get(&rstate->txout_failures, pending->short_channel_id.u64)) { + * again. Just drop the announcement and walk away whistling. */ + if (in_txout_failures(rstate, &pending->short_channel_id)) { SUPERVERBOSE( "Ignoring channel_announcement of %s due to a prior txout " "query failure. The channel was likely closed on-chain.", From f1cc77762d5c2bee58e813cd9f9d1d24030e7b76 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Nov 2019 12:46:09 +1030 Subject: [PATCH 4/5] gossipd: add txout_failure when a close is seen. This prevents a gratuitous lookup of we get a late channel_announce, but even better, it suppresses the "bad gossip" messages in case of a late channel_update, which have plagued Travis (especially since we got aggressive in pushing our own updates). Signed-off-by: Rusty Russell --- gossipd/gossipd.c | 2 ++ gossipd/routing.c | 4 ++-- gossipd/routing.h | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index c0040de7d91d..ed936807ffc7 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -1517,6 +1517,8 @@ static struct io_plan *handle_outpoint_spent(struct io_conn *conn, "Deleting channel %s due to the funding outpoint being " "spent", type_to_string(msg, struct short_channel_id, &scid)); + /* Suppress any now-obsolete updates/announcements */ + add_to_txout_failures(rstate, &scid); remove_channel_from_store(rstate, chan); /* Freeing is sufficient since everything else is allocated off * of the channel and this takes care of unregistering diff --git a/gossipd/routing.c b/gossipd/routing.c index 18b3cc7beb8a..2cfeccc7b79a 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -258,8 +258,8 @@ static void txout_failure_age(struct routing_state *rstate) txout_failure_age, rstate); } -static void add_to_txout_failures(struct routing_state *rstate, - const struct short_channel_id *scid) +void add_to_txout_failures(struct routing_state *rstate, + const struct short_channel_id *scid) { if (uintmap_add(&rstate->txout_failures, scid->u64, true) && ++rstate->num_txout_failures == 10000) { diff --git a/gossipd/routing.h b/gossipd/routing.h index 0880108d2cca..cf994511e4e6 100644 --- a/gossipd/routing.h +++ b/gossipd/routing.h @@ -529,4 +529,8 @@ void remove_channel_from_store(struct routing_state *rstate, const char *unfinalized_entries(const tal_t *ctx, struct routing_state *rstate); void remove_all_gossip(struct routing_state *rstate); + +/* This scid is dead to us. */ +void add_to_txout_failures(struct routing_state *rstate, + const struct short_channel_id *scid); #endif /* LIGHTNING_GOSSIPD_ROUTING_H */ From d8c386a141beccc67fc9d30de70cf027331de089 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Nov 2019 13:24:50 +1030 Subject: [PATCH 5/5] sphinx: fix potential data leak. https://github.com/lightningnetwork/lightning-rfc/pull/697 https://lists.linuxfoundation.org/pipermail/lightning-dev/2019-November/002288.html We generate it from an hmac using the session secret. It's not clear that this will be useful for reproducing test vectors though, since we don't generate the first 66 bytes, which is what the spec says to do. Reported-by: @roasbeef Signed-off-by: Rusty Russell --- common/sphinx.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/common/sphinx.c b/common/sphinx.c index af2f96a76762..5fe79a2f8bfd 100644 --- a/common/sphinx.c +++ b/common/sphinx.c @@ -511,6 +511,7 @@ struct onionpacket *create_onionpacket( sphinx_hop_size(&sp->hops[num_hops - 1]); u8 filler[fillerSize]; struct keyset keys; + u8 padkey[KEY_LEN]; u8 nexthmac[HMAC_SIZE]; u8 stream[ROUTING_INFO_SIZE]; struct hop_params *params; @@ -529,7 +530,16 @@ struct onionpacket *create_onionpacket( } packet->version = 0; memset(nexthmac, 0, HMAC_SIZE); - memset(packet->routinginfo, 0, ROUTING_INFO_SIZE); + + /* BOLT-e116441ee836447ac3f24cdca62bac1e0f223d5f #4: + * + * The packet is initialized with 1366 _random_ bytes derived from a + * CSPRNG. + */ + /* Note that this is just hop_payloads: the rest of the packet is + * overwritten below or above anyway. */ + generate_key(padkey, "pad", 3, sp->session_key->data); + generate_cipher_stream(stream, padkey, ROUTING_INFO_SIZE); generate_header_padding(filler, sizeof(filler), sp, params);