Do not use third party services for IP detection. #5161

gmaxwell commented at 10:07 PM on October 28, 2014: contributor

This is a simplified re-do of closed pull #3088.

This patch eliminates the privacy and reliability problematic use of centralized web services for discovering the node's addresses for advertisement.

The Bitcoin protocol already allows your peers to tell you what IP they think you have, but this data isn't trustworthy since they could lie. So the challenge is using it without creating a DOS vector.

To accomplish this we adopt an approach similar to the one used by P2Pool: If we're announcing and don't have a better address discovered (e.g. via UPNP) or configured we just announce to each peer the address that peer told us. Since peers could already replace, forge, or drop our address messages this cannot create a new vulnerability... but if even one of our peers is giving us a good address we'll eventually make a useful advertisement.

We also may randomly use the peer-provided address for the daily rebroadcast even if we otherwise have a seemingly routable address, just in case we've been misconfigured (e.g. by UPNP).

To avoid privacy problems, we only do these things if discovery is enabled.

gmaxwell commented at 10:08 PM on October 28, 2014: contributor

I've tested this (and variations of it) for a while and I'm happy with it. I think its reasonably simple now and avoids moving much of anything around.

The testing I performed was two fold, I ran an instrumented copy that logged its announcements and saw it making both types of announcements.

I also ran a node behind nat (with a port forward) which hasn't had bitcoin running on it recently without UPNP or other ways of discovering an IP and confirmed that it eventually gained inbound connections.

gmaxwell added this to the milestone 0.10.0 on Oct 28, 2014

TheBlueMatt commented at 6:13 AM on October 29, 2014: member

Can we change the help text on -discover to say something like "Do not rumor your address except as provided by -localip"? (and, more broadly, do we really want a flag for this? It is some strange depend-on-my-peer-not-rumoring-my-address benchmark, when we really should just use fListen?)

in src/main.cpp:None in 9eee4d8b4c outdated

4349 | @@ -4344,8 +4350,9 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
4350 |  
4351 |          // Address refresh broadcast
4352 |          static int64_t nLastRebroadcast;
4353 | -        if (!IsInitialBlockDownload() && (GetTime() - nLastRebroadcast > 24 * 60 * 60))
4354 | +        if (fListen && !IsInitialBlockDownload() && (GetTime() - nLastRebroadcast > 24 * 60 * 60))

TheBlueMatt commented at 6:14 AM on October 29, 2014:

Because of this we only clear setAddrKnown when we're listening, which I dont think was intended?

gmaxwell commented at 6:49 AM on October 29, 2014:

gah. good catch. I added that at the last minute "oh why bother taking the vnodes lock when we're not going to do anything with it."

in src/main.cpp:None in 9eee4d8b4c outdated

3482 | @@ -3483,7 +3483,13 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
3483 |              {
3484 |                  CAddress addr = GetLocalAddress(&pfrom->addr);
3485 |                  if (addr.IsRoutable())
3486 | +                {
3487 | +                    pfrom->PushAddress(addr);
3488 | +                } else if (IsPeerAddrLocalGood(pfrom)) {
3489 | +                    addr = CAddress(pfrom->addrLocal);

TheBlueMatt commented at 6:21 AM on October 29, 2014:

After this youve cleared the nServices and nTime fields set by GetLocalAddress.

luke-jr commented at 2:12 PM on November 4, 2014:

How about using addr.SetIP(pfrom->addrLocal); here, rather than re-setting nServices/nTime below? (this seems more future-proof if CAddress adds new stuff ever)

gmaxwell commented at 6:50 PM on November 4, 2014:

This cuts both ways, e.g. extra fields you didn't want kept. But I agree thats better, changed.

in src/net.cpp:None in 9eee4d8b4c outdated

 243 | -            if (addrLocal.IsRoutable() && (CService)addrLocal != (CService)pnode->addrLocal)
 244 | -            {
 245 | -                pnode->PushAddress(addrLocal);
 246 | -                pnode->addrLocal = addrLocal;
 247 | -            }
 248 | +            addrLocal = CAddress(pnode->addrLocal);

TheBlueMatt commented at 6:21 AM on October 29, 2014:

After this youve cleared the nServices and nTime fields set by GetLocalAddress.

gmaxwell commented at 7:21 AM on October 29, 2014: contributor

Can we change the help text on -discover to say something like "Do not rumor your address except as provided by -localip"? (and, more broadly, do we really want a flag for this? It is some strange depend-on-my-peer-not-rumoring-my-address benchmark, when we really should just use fListen?)

Consider, when you're listening to torHS you want listen=1 but you certantly don't want discovery of any type. (capturing that, one of the extra changes I made here was making proxy also imply no discovery but it still should be settable). Beyond hoping that your peers won't disrespect your wishes, it can avoid rumoring incorrect data when your outbound connections don't match your inbound. (p2pool has no way to control what addresses get rumored and this has broken things for at least a couple people).

ghost commented at 8:09 AM on October 29, 2014: none

I think there is some clown out there who is addr messaging for everyone who connects to him

There's a number of clients on the network that do absolutely stupid things with the IP address in the handshake and addr messages. One piece of software returns ::1 for everything, another returns a 0.0.0.0, another makes up random IP addresses for their peers. I don't know what software they are, because two pretend to be a /Satoshi/ client (Satoshi seems to be the new Mozilla/5.0), and the other returns null for the human readable version name.

It's likely not a problem for this patch, just be aware that even the big players are writing horribly broken code that doesn't even attempt to conform to the p2p network the core client does.

gmaxwell commented at 8:16 AM on October 29, 2014: contributor

Yea, this patch doesn't assume the peers are non-evil, much less assuming they aren't conventionally broken.

laanwj added the label P2P on Oct 29, 2014

gmaxwell commented at 8:34 PM on October 29, 2014: contributor

@TheBlueMatt see updates

in src/net.cpp:None in bf0f14f1c9 outdated

 224 | +           GetListenPort() == Params().GetDefaultPort() &&
 225 | +           !IsLimited(pnode->addrLocal.GetNetwork());
 226 | +}
 227 | +
 228 | +// pushes our own address to a peer
 229 | +void AdvertizeLocal(CNode *pnode)

kanzure commented at 9:50 PM on October 29, 2014:

Does AdvertizeLocal still qualify for the previous comment ("used when scores of local addresses may have changed")?

gmaxwell commented at 10:16 PM on October 29, 2014:

Only indirectly. Previously it invoked readvertisement more frequently, potentially triggered by external events. With this change we only advertise on connect and on a timer... if the scores change it'll influence what we advertise, but no longer directly triggers it.

luke-jr commented at 2:15 PM on November 4, 2014:

Is advertise intentionally misspelled?

gmaxwell commented at 6:24 PM on November 4, 2014:

It's not misspelled. advertize is EN_US and that was the name previously.

luke-jr commented at 6:58 PM on November 4, 2014:

Weird, I've never seen it spelled with a 'z' even in EN_US, but it appears Google agrees.

laanwj commented at 1:23 PM on November 8, 2014:

After this change do we ever advertize our Tor hidden service address? I've added logging to my test node that listens both on Tor and plainnet, both external addresses appear as SeenLocal

2014-11-08 12:56:48 SeenLocal: xx.xx.xx.xx:8333
2014-11-08 12:57:05 SeenLocal: xxx.onion:8333

However, it always advertizes the IPv4 address

2014-11-08 09:15:21 AdvertizeLocal: advertizing address xx.xx.xx.xx:8333

(maybe this can be avoided using discover=0, and providing externalips, but I explicitly disabled those to test this code)

gmaxwell commented at 6:21 PM on November 8, 2014:

The resason you're seen-localing it is becasue peers who already know it are connecting inbound to you. Since outbound HS peers can never seen your HS address it can't be discovered, and must be configured with externalips.

laanwj commented at 3:34 PM on November 10, 2014:

OK - so when providing externalips, it will advertise the HS address?

gmaxwell commented at 5:12 PM on November 10, 2014:

Yes; for two reasons: one is because externalips disables discover, the other is because if you enable discover it still mosty uses the traditional address source (what it would have used if discover were distabled) if it has one that it thinks is routable.

spinza commented at 1:16 PM on October 30, 2014: none

Does this pull request also resolve issue #48 ?

gmaxwell commented at 5:34 PM on October 30, 2014: contributor

@spinza the substance of #48 was mostly solved by the prior heartbeating changes. But this will improve the loss of inbound connections after your IP changes.

in src/init.cpp:None in bf0f14f1c9 outdated

 567 | @@ -568,6 +568,9 @@ bool AppInit2(boost::thread_group& threadGroup)
 568 |          // to protect privacy, do not listen by default if a default proxy server is specified
 569 |          if (SoftSetBoolArg("-listen", false))
 570 |              LogPrintf("AppInit2 : parameter interaction: -proxy set -> setting -listen=0\n");
 571 | +        // to protect privacy, do not discover addresses by default
 572 | +        if (SoftSetBoolArg("-discover", false))
 573 | +            LogPrintf("AppInit2 : parameter interaction: -proxy set -> setting -discover=0\n");

luke-jr commented at 2:04 PM on November 4, 2014:

I don't think this rationale fits. Proxies aren't always for privacy. More importantly: is there a reason not to default discover to false always?

gmaxwell commented at 6:21 PM on November 4, 2014:

If proxy is set complex things are happening with your networking and discovery is much more likely not useful.

Defaulting to discover false would break listening for every host behind NAT by default, precisely those users who are least likely to set things right.

Avoiding it when proxy is set helps minimize unwanted surprises without gratitious breakage (afer all, we already disabling listening in that case).

in src/main.cpp:None in bf0f14f1c9 outdated

4373 | -                    }
4374 | +                    AdvertizeLocal(pnode);
4375 |                  }
4376 |              }
4377 | -            nLastRebroadcast = GetTime();
4378 | +            if (rebroadcastRun)

luke-jr commented at 2:14 PM on November 4, 2014:

This would be clearer to just say if (!vNodes.empty())

gmaxwell commented at 6:46 PM on November 4, 2014:

Hm. Originally I'd had more complex logic; skipping it if it didn't successfully broadcast but that interacted poorly with Addrknown clearing, okay, fair enough it can indeed be simplified now.

gmaxwell commented at 6:51 PM on November 4, 2014: contributor

@luke-jr updated

luke-jr commented at 8:21 PM on November 4, 2014: member

laanwj commented at 10:25 AM on November 5, 2014: member

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Tested ACK 5fd28b94e57d14f81bc63717c99e3e7ca7316b75
I've tested w/ a host behind NAT, upnp=0, discover=1, it advertizes the right IP
Also tested with discover=0 and no externalip, no advertizing of local addresses happens (as expected)
Reviewed the code and the logic makes sense to me
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCgAGBQJUWfrPAAoJEHSBCwEjRsmm0pYH/3BcQvSe93wEyh0AgRnjyQVy
W2qoTeC9orNA6sr709S1xOmgjKbAEgPDXScqAdDnYkYWuL4CuXEjn6lrQ+O/5Vl5
AM9DG6jlrTz7RVkbXt1knPsWNTs7nHAwZWFHWeq2XwrPdQH5QG//EJys8wb6DJ2O
0AsXpqer/GbHOUGusye4tCQSL+kNSD10fMfG581VVXMVzgjc1zMsDm386N5qPjWJ
PMoD7fmqZe5xDiMa2tJZ0tXcNSl10/Iz4jNwO7zZaNQMDDs131YWLcc7Cz7LVTVG
z51kBFy8Wt25+1qhLsjsRs4gmMTxaKuu/6XL0dUtaGwxSDf2Iz2YQ9bPjJaQx50=
=UWK5
-----END PGP SIGNATURE-----

gmaxwell commented at 4:45 PM on November 7, 2014: contributor

Does anyone feel this is waiting on anything else?

in src/net.cpp:None in 5fd28b94e5 outdated

 223 | +
 224 | +// Is our peer's addrLocal potentially useful as an external IP source?
 225 | +bool IsPeerAddrLocalGood(CNode *pnode)
 226 | +{
 227 | +    return fDiscover && pnode->addr.IsRoutable() && pnode->addrLocal.IsRoutable() &&
 228 | +           GetListenPort() == Params().GetDefaultPort() &&

sipa commented at 5:29 PM on November 7, 2014:

Why would the address a peer tells us be not good if we're running on a non-standard port?

gmaxwell commented at 7:42 PM on November 7, 2014:

The peer can't tell us anything about the port we're listning on, (E.g. you connect out to some peer and they see the source port as some random number).... we're already behind address translation. Seemed more conservative to skip in this case.

in src/main.cpp:None in 5fd28b94e5 outdated

4396 | -                        if (addr.IsRoutable())
4397 | -                            pnode->PushAddress(addr);
4398 | -                    }
4399 | -                }
4400 | +                // Rebroadcast our address
4401 | +                AdvertizeLocal(pnode);

sipa commented at 5:30 PM on November 7, 2014:

This probably runs much more than needed. Would it make sense to move it into the above if block?

gmaxwell commented at 7:49 PM on November 7, 2014:

The prior if is always true except the first time through, we do actually want this to fire the first time through. I think? the test that keeps it from running all the time is two blocks up (on the line with the initialblockdownload check).

sipa commented at 1:54 PM on November 8, 2014:

ok

in src/net.cpp:None in 5fd28b94e5 outdated

 215 |  {
 216 | -    LOCK(cs_vNodes);
 217 | -    BOOST_FOREACH(CNode* pnode, vNodes)
 218 | +    LOCK(cs_mapLocalHost);
 219 | +    if (mapLocalHost.count(addr) == 0)
 220 | +        return 0;

sipa commented at 5:31 PM on November 7, 2014:

Use LOCAL_NONE constant instead?

in src/net.cpp:None in 5fd28b94e5 outdated

 238 | +        CAddress addrLocal = GetLocalAddress(&pnode->addr);
 239 | +        // If discovery is enabled, sometimes give our peer the address it
 240 | +        // tells us that it sees us as in case it has a better idea of our
 241 | +        // address than we do.
 242 | +        if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() ||
 243 | +             GetRand(GetnScore(addrLocal)>LOCAL_MANUAL?8:2) == 0))

sipa commented at 5:33 PM on November 7, 2014:

Coding style nit: a few more spaces please? (I actually misread the > as a -> initially)

sipa commented at 5:34 PM on November 7, 2014: member

utACK, with a few nits.

Do not use third party services for IP detection.

This is a simplified re-do of closed pull #3088.

This patch eliminates the privacy and reliability problematic use
of centralized web services for discovering the node's addresses
for advertisement.

The Bitcoin protocol already allows your peers to tell you what
IP they think you have, but this data isn't trustworthy since
they could lie. So the challenge is using it without creating a
DOS vector.

To accomplish this we adopt an approach similar to the one used
by P2Pool: If we're announcing and don't have a better address
discovered (e.g. via UPNP) or configured we just announce to
each peer the address that peer told us. Since peers could
already replace, forge, or drop our address messages this cannot
create a new vulnerability... but if even one of our peers is
giving us a good address we'll eventually make a useful
advertisement.

We also may randomly use the peer-provided address for the
daily rebroadcast even if we otherwise have a seemingly routable
address, just in case we've been misconfigured (e.g. by UPNP).

To avoid privacy problems, we only do these things if discovery
is enabled.

845c86d128

gmaxwell commented at 8:15 PM on November 7, 2014: contributor

@sipa adjusted except for the nLastRebroadcast. I think you were misreading it there. Can you confirm?

On the disabling it when using a non-default port, any of the other ACKers have an opinion? I think it could go either way. Avoiding running it seemed more conservative to me, but there are cases where if we require the default port it would fail to discover an address that would be useful... when you're behind nat on a non-standard port and managed to map the ports 1:1, and where the old discovery code would have worked. So I've changed it.

laanwj commented at 6:31 AM on November 11, 2014: member

@gmaxwell I'd say it's still useful to be able to discover external IP changes even if not using the default port. Throughout the code, bitcoind assumes that it knows the port where it's listening on.

The edge case here would be a) behind NAT b) manually forwarded port c) external port is different from internal port. But in that case bitcoind has no way of discovering what the external port is for advertising, so this code won't make a difference. I don't think we can handle that case at all right now (even -externalip can't take a port).

gmaxwell commented at 9:10 AM on November 11, 2014: contributor

@laanwj Okay, great. (I did change it so that it doesn't require the default port anymore).

laanwj merged this on Nov 12, 2014

laanwj closed this on Nov 12, 2014

laanwj referenced this in commit 0c7862e968 on Nov 12, 2014

unknown referenced this in commit 845e59f0ee on Nov 27, 2014

laanwj referenced this in commit 66b473457b on Jan 20, 2015

rnicoll referenced this in commit 88377f5fe7 on Feb 15, 2015

jameshilliard referenced this in commit 3bfb1f1ca9 on Apr 15, 2015

rnicoll referenced this in commit b03e0049df on Jun 11, 2015

rnicoll referenced this in commit 582ec9f242 on Jun 21, 2015

reddink referenced this in commit a6e79d1fb7 on May 27, 2020

MarcoFalke locked this on Sep 8, 2021