From d40d22b68e338c13d7f05e7466c41c5251d86e7d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 7 May 2018 13:59:22 +0930 Subject: [PATCH] gossipd: don't try to connect to non-routable addresses. Someone could try to announce an internal address, and we might probe it. This breaks tests, so we add '--dev-allow-localhost' for our tests, so we don't eliminate that one. Of course, now we need to skip some more tests in non-developer mode. Signed-off-by: Rusty Russell --- common/test/run-ip_port_parsing.c | 6 ---- gossipd/gossip.c | 41 +++++++++++++++----------- gossipd/gossip_wire.csv | 1 + gossipd/netaddress.c | 4 ++- gossipd/netaddress.h | 3 +- gossipd/routing.c | 4 ++- gossipd/routing.h | 6 +++- gossipd/test/run-bench-find_route.c | 2 +- gossipd/test/run-find_route-specific.c | 2 +- gossipd/test/run-find_route.c | 2 +- lightningd/gossip_control.c | 8 ++++- lightningd/lightningd.c | 1 + lightningd/lightningd.h | 3 ++ lightningd/options.c | 3 ++ tests/test_lightningd.py | 12 +++++--- tests/utils.py | 6 ++-- 16 files changed, 66 insertions(+), 38 deletions(-) diff --git a/common/test/run-ip_port_parsing.c b/common/test/run-ip_port_parsing.c index d99811c16..0773e3a3c 100644 --- a/common/test/run-ip_port_parsing.c +++ b/common/test/run-ip_port_parsing.c @@ -8,9 +8,6 @@ /* Generated stub for fromwire */ const u8 *fromwire(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, void *copy UNNEEDED, size_t n UNNEEDED) { fprintf(stderr, "fromwire called!\n"); abort(); } -/* Generated stub for fromwire_bool */ -bool fromwire_bool(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) -{ fprintf(stderr, "fromwire_bool called!\n"); abort(); } /* Generated stub for fromwire_fail */ const void *fromwire_fail(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) { fprintf(stderr, "fromwire_fail called!\n"); abort(); } @@ -26,9 +23,6 @@ void fromwire_u8_array(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, u8 *arr /* Generated stub for towire */ void towire(u8 **pptr UNNEEDED, const void *data UNNEEDED, size_t len UNNEEDED) { fprintf(stderr, "towire called!\n"); abort(); } -/* Generated stub for towire_bool */ -void towire_bool(u8 **pptr UNNEEDED, bool v UNNEEDED) -{ fprintf(stderr, "towire_bool called!\n"); abort(); } /* Generated stub for towire_u16 */ void towire_u16(u8 **pptr UNNEEDED, u16 v UNNEEDED) { fprintf(stderr, "towire_u16 called!\n"); abort(); } diff --git a/gossipd/gossip.c b/gossipd/gossip.c index beece573e..4afa2fda3 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -1606,14 +1606,14 @@ static bool handle_wireaddr_listen(struct daemon *daemon, } /* If it's a wildcard, turns it into a real address pointing to internet */ -static bool public_address(struct wireaddr *wireaddr) +static bool public_address(struct daemon *daemon, struct wireaddr *wireaddr) { if (wireaddr_is_wildcard(wireaddr)) { if (!guess_address(wireaddr)) return false; } - return address_routable(wireaddr); + return address_routable(wireaddr, daemon->rstate->dev_allow_localhost); } static void add_announcable(struct daemon *daemon, const struct wireaddr *addr) @@ -1684,7 +1684,7 @@ static struct wireaddr_internal *setup_listeners(const tal_t *ctx, true); if (ipv6_ok) { add_binding(&binding, &wa); - if (public_address(&wa.u.wireaddr)) + if (public_address(daemon, &wa.u.wireaddr)) add_announcable(daemon, &wa.u.wireaddr); } wa.u.wireaddr.type = ADDR_TYPE_IPV4; @@ -1693,7 +1693,7 @@ static struct wireaddr_internal *setup_listeners(const tal_t *ctx, if (handle_wireaddr_listen(daemon, &wa.u.wireaddr, ipv6_ok)) { add_binding(&binding, &wa); - if (public_address(&wa.u.wireaddr)) + if (public_address(daemon, &wa.u.wireaddr)) add_announcable(daemon, &wa.u.wireaddr); } continue; @@ -1701,7 +1701,7 @@ static struct wireaddr_internal *setup_listeners(const tal_t *ctx, case ADDR_INTERNAL_WIREADDR: handle_wireaddr_listen(daemon, &wa.u.wireaddr, false); add_binding(&binding, &wa); - if (public_address(&wa.u.wireaddr)) + if (public_address(daemon, &wa.u.wireaddr)) add_announcable(daemon, &wa.u.wireaddr); continue; } @@ -1723,18 +1723,21 @@ static struct io_plan *gossip_init(struct daemon_conn *master, { struct bitcoin_blkid chain_hash; u32 update_channel_interval; + bool dev_allow_localhost; if (!fromwire_gossipctl_init( daemon, msg, &daemon->broadcast_interval, &chain_hash, &daemon->id, &daemon->globalfeatures, &daemon->localfeatures, &daemon->proposed_wireaddr, &daemon->proposed_listen_announce, daemon->rgb, - daemon->alias, &update_channel_interval, &daemon->reconnect)) { + daemon->alias, &update_channel_interval, &daemon->reconnect, + &dev_allow_localhost)) { master_badmsg(WIRE_GOSSIPCTL_INIT, msg); } /* Prune time is twice update time */ daemon->rstate = new_routing_state(daemon, &chain_hash, &daemon->id, - update_channel_interval * 2); + update_channel_interval * 2, + dev_allow_localhost); /* Load stored gossip messages */ gossip_store_load(daemon->rstate, daemon->rstate->store); @@ -1944,7 +1947,6 @@ gossip_resolve_addr(const tal_t *ctx, const struct pubkey *id) { struct node *node; - struct addrhint *a; /* Get from routing state. */ node = get_node(rstate, id); @@ -1952,18 +1954,23 @@ gossip_resolve_addr(const tal_t *ctx, /* No matching node? */ if (!node) return NULL; - /* Node has no addresses? */ - if (tal_count(node->addresses) == 0) - return NULL; /* FIXME: When struct addrhint can contain more than one address, - * we should copy all addresses. - * For now getting first address should be fine. */ - a = tal(ctx, struct addrhint); - a->addr.itype = ADDR_INTERNAL_WIREADDR; - a->addr.u.wireaddr = node->addresses[0]; + * we should copy all routable addresses. */ + for (size_t i = 0; i < tal_count(node->addresses); i++) { + struct addrhint *a; - return a; + if (!address_routable(&node->addresses[i], + rstate->dev_allow_localhost)) + continue; + + a = tal(ctx, struct addrhint); + a->addr.itype = ADDR_INTERNAL_WIREADDR; + a->addr.u.wireaddr = node->addresses[i]; + return a; + } + + return NULL; } static void try_reach_peer(struct daemon *daemon, const struct pubkey *id, diff --git a/gossipd/gossip_wire.csv b/gossipd/gossip_wire.csv index c65115115..d7023b924 100644 --- a/gossipd/gossip_wire.csv +++ b/gossipd/gossip_wire.csv @@ -18,6 +18,7 @@ gossipctl_init,,rgb,3*u8 gossipctl_init,,alias,32*u8 gossipctl_init,,update_channel_interval,u32 gossipctl_init,,reconnect,bool +gossipctl_init,,dev_allow_localhost,bool # Activate the gossip daemon, so others can connect. gossipctl_activate,3025 diff --git a/gossipd/netaddress.c b/gossipd/netaddress.c index 22cb85df4..7d3d4b41f 100644 --- a/gossipd/netaddress.c +++ b/gossipd/netaddress.c @@ -265,7 +265,9 @@ bool guess_address(struct wireaddr *addr) abort(); } -bool address_routable(const struct wireaddr *wireaddr) +bool address_routable(const struct wireaddr *wireaddr, bool allow_localhost) { + if (allow_localhost && IsLocal(wireaddr)) + return true; return IsRoutable(wireaddr); } diff --git a/gossipd/netaddress.h b/gossipd/netaddress.h index 664b10ad0..27ce71853 100644 --- a/gossipd/netaddress.h +++ b/gossipd/netaddress.h @@ -8,6 +8,7 @@ bool guess_address(struct wireaddr *wireaddr); /* Is this address public? */ -bool address_routable(const struct wireaddr *wireaddr); +bool address_routable(const struct wireaddr *wireaddr, + bool allow_localhost); #endif /* LIGHTNING_GOSSIPD_NETADDRESS_H */ diff --git a/gossipd/routing.c b/gossipd/routing.c index d7930588e..81cc03b84 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -86,7 +86,8 @@ static struct node_map *empty_node_map(const tal_t *ctx) struct routing_state *new_routing_state(const tal_t *ctx, const struct bitcoin_blkid *chain_hash, const struct pubkey *local_id, - u32 prune_timeout) + u32 prune_timeout, + bool dev_allow_localhost) { struct routing_state *rstate = tal(ctx, struct routing_state); rstate->nodes = empty_node_map(rstate); @@ -95,6 +96,7 @@ struct routing_state *new_routing_state(const tal_t *ctx, rstate->local_id = *local_id; rstate->prune_timeout = prune_timeout; rstate->store = gossip_store_new(rstate); + rstate->dev_allow_localhost = dev_allow_localhost; list_head_init(&rstate->pending_cannouncement); uintmap_init(&rstate->chanmap); diff --git a/gossipd/routing.h b/gossipd/routing.h index 6c1312074..45867b32a 100644 --- a/gossipd/routing.h +++ b/gossipd/routing.h @@ -162,6 +162,9 @@ struct routing_state { * restarts */ struct gossip_store *store; + /* For testing, we announce and accept localhost */ + bool dev_allow_localhost; + /* A map of channels indexed by short_channel_ids */ UINTMAP(struct chan *) chanmap; }; @@ -183,7 +186,8 @@ struct route_hop { struct routing_state *new_routing_state(const tal_t *ctx, const struct bitcoin_blkid *chain_hash, const struct pubkey *local_id, - u32 prune_timeout); + u32 prune_timeout, + bool dev_allow_localhost); struct chan *new_chan(struct routing_state *rstate, const struct short_channel_id *scid, diff --git a/gossipd/test/run-bench-find_route.c b/gossipd/test/run-bench-find_route.c index 6a7f85c5a..2b24a12b0 100644 --- a/gossipd/test/run-bench-find_route.c +++ b/gossipd/test/run-bench-find_route.c @@ -219,7 +219,7 @@ int main(int argc, char *argv[]) | SECP256K1_CONTEXT_SIGN); setup_tmpctx(); - rstate = new_routing_state(tmpctx, &zerohash, &me, 0); + rstate = new_routing_state(tmpctx, &zerohash, &me, 0, false); opt_register_noarg("--perfme", opt_set_bool, &perfme, "Run perfme-start and perfme-stop around benchmark"); diff --git a/gossipd/test/run-find_route-specific.c b/gossipd/test/run-find_route-specific.c index 9a980327a..c1d5fd1ab 100644 --- a/gossipd/test/run-find_route-specific.c +++ b/gossipd/test/run-find_route-specific.c @@ -152,7 +152,7 @@ int main(void) strlen("02ea622d5c8d6143f15ed3ce1d501dd0d3d09d3b1c83a44d0034949f8a9ab60f06"), &c); - rstate = new_routing_state(tmpctx, &zerohash, &a, 0); + rstate = new_routing_state(tmpctx, &zerohash, &a, 0, false); /* [{'active': True, 'short_id': '6990:2:1/1', 'fee_per_kw': 10, 'delay': 5, 'flags': 1, 'destination': '0230ad0e74ea03976b28fda587bb75bdd357a1938af4424156a18265167f5e40ae', 'source': '02ea622d5c8d6143f15ed3ce1d501dd0d3d09d3b1c83a44d0034949f8a9ab60f06', 'last_update': 1504064344}, */ nc = get_or_make_connection(rstate, &c, &b, "6990:2:1"); diff --git a/gossipd/test/run-find_route.c b/gossipd/test/run-find_route.c index 2eefc9eff..e166c0418 100644 --- a/gossipd/test/run-find_route.c +++ b/gossipd/test/run-find_route.c @@ -188,7 +188,7 @@ int main(void) memset(&tmp, 'a', sizeof(tmp)); pubkey_from_privkey(&tmp, &a); - rstate = new_routing_state(tmpctx, &zerohash, &a, 0); + rstate = new_routing_state(tmpctx, &zerohash, &a, 0, false); new_node(rstate, &a); diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index c0dfa08d6..ba895748d 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -185,6 +185,11 @@ void gossip_init(struct lightningd *ld) u64 capabilities = HSM_CAP_ECDH | HSM_CAP_SIGN_GOSSIP; struct wireaddr_internal *wireaddrs = ld->proposed_wireaddr; enum addr_listen_announce *listen_announce = ld->proposed_listen_announce; + bool allow_localhost = false; +#if DEVELOPER + if (ld->dev_allow_localhost) + allow_localhost = true; +#endif msg = towire_hsm_client_hsmfd(tmpctx, &ld->id, capabilities); if (!wire_sync_write(ld->hsm_fd, msg)) @@ -219,7 +224,8 @@ void gossip_init(struct lightningd *ld) get_offered_global_features(tmpctx), get_offered_local_features(tmpctx), wireaddrs, listen_announce, ld->rgb, - ld->alias, ld->config.channel_update_interval, ld->reconnect); + ld->alias, ld->config.channel_update_interval, ld->reconnect, + allow_localhost); subd_send_msg(ld->gossip, msg); } diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 2ce639c4e..5a297563b 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -51,6 +51,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx) ld->dev_debug_subdaemon = NULL; ld->dev_disconnect_fd = -1; ld->dev_subdaemon_fail = false; + ld->dev_allow_localhost = false; if (getenv("LIGHTNINGD_DEV_MEMLEAK")) memleak_init(ld, backtrace_state); diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 311726222..d711a58b6 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -189,6 +189,9 @@ struct lightningd { /* If we have --dev-fail-on-subdaemon-fail */ bool dev_subdaemon_fail; + /* Allow and accept localhost node_announcement addresses */ + bool dev_allow_localhost; + /* Things we've marked as not leaking. */ const void **notleaks; #endif /* DEVELOPER */ diff --git a/lightningd/options.c b/lightningd/options.c index a9ec6fc9e..0f83e1cb1 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -415,6 +415,9 @@ static void dev_register_opts(struct lightningd *ld) "Time between gossip broadcasts in milliseconds"); opt_register_arg("--dev-disconnect=", opt_subd_dev_disconnect, NULL, ld, "File containing disconnection points"); + opt_register_noarg("--dev-allow-localhost", opt_set_bool, + &ld->dev_allow_localhost, + "Announce and allow announcments for localhost address"); } #endif diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index a0807632c..12db1fd5c 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -664,6 +664,7 @@ class LightningDTests(BaseLightningDTests): "Cryptographic handshake: ", l1.rpc.connect, '032cf15d1ad9c4a08d26eab1918f732d8ef8fdc6abb9640bf3db174372c491304e', 'localhost', l2.port) + @unittest.skipIf(not DEVELOPER, "needs --dev-allow-localhost") def test_connect_by_gossip(self): """Test connecting to an unknown peer using node gossip """ @@ -4284,10 +4285,13 @@ class LightningDTests(BaseLightningDTests): def test_address(self): l1 = self.node_factory.get_node() addr = l1.rpc.getinfo()['address'] - assert len(addr) == 1 - assert addr[0]['type'] == 'ipv4' - assert addr[0]['address'] == '127.0.0.1' - assert int(addr[0]['port']) == l1.port + if 'dev-allow-localhost' in l1.daemon.opts: + assert len(addr) == 1 + assert addr[0]['type'] == 'ipv4' + assert addr[0]['address'] == '127.0.0.1' + assert int(addr[0]['port']) == l1.port + else: + assert len(addr) == 0 bind = l1.rpc.getinfo()['binding'] assert len(bind) == 1 diff --git a/tests/utils.py b/tests/utils.py index 29a9fde29..d4441de13 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -261,9 +261,7 @@ class LightningD(TailableProc): opts = { 'bitcoin-datadir': bitcoin_dir, 'lightning-dir': lightning_dir, - 'bind-addr': '127.0.0.1:{}'.format(port), - # lightningd won't announce non-routable addresses by default. - 'announce-addr': '127.0.0.1:{}'.format(port), + 'addr': '127.0.0.1:{}'.format(port), 'allow-deprecated-apis': 'false', 'override-fee-rates': '15000/7500/1000', 'network': 'regtest', @@ -283,6 +281,8 @@ class LightningD(TailableProc): f.write(seed) if DEVELOPER: self.opts['dev-broadcast-interval'] = 1000 + # lightningd won't announce non-routable addresses by default. + self.opts['dev-allow-localhost'] = None self.prefix = 'lightningd-%d' % (node_id) @property