Skip to content

Commit 52c3b03

Browse files
kwvgkwvg
committed
merge bitcoin#23542: open p2p connections to nodes that listen on non-default ports
1 parent 8e2a12a commit 52c3b03

File tree

11 files changed

+293
-21
lines changed

11 files changed

+293
-21
lines changed

‎doc/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ The Dash Core repo's [root README](/README.md) contains relevant information on
7676
- [I2P Support](i2p.md)
7777
- [Init Scripts (systemd/upstart/openrc)](init.md)
7878
- [Managing Wallets](managing-wallets.md)
79+
- [P2P bad ports definition and list](p2p-bad-ports.md)
7980
- [PSBT support](psbt.md)
8081
- [Reduce Memory](reduce-memory.md)
8182
- [Reduce Traffic](reduce-traffic.md)

‎doc/p2p-bad-ports.md‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
When Dash Core automatically opens outgoing P2P connections it chooses
2+
a peer (address and port) from its list of potential peers. This list is
3+
populated with unchecked data, gossiped over the P2P network by other peers.
4+
5+
A malicious actor may gossip an address:port where no Dash node is listening,
6+
or one where a service is listening that is not related to the Dash network.
7+
As a result, this service may occasionally get connection attempts from Dash
8+
nodes.
9+
10+
"Bad" ports are ones used by services which are usually not open to the public
11+
and usually require authentication. A connection attempt (by Dash Core,
12+
trying to connect because it thinks there is a Dash node on that
13+
address:port) to such service may be considered a malicious action by an
14+
ultra-paranoid administrator. An example for such a port is 22 (ssh). On the
15+
other hand, connection attempts to public services that usually do not require
16+
authentication are unlikely to be considered a malicious action,
17+
e.g. port 80 (http).
18+
19+
Below is a list of "bad" ports which Dash Core avoids when choosing a peer to
20+
connect to. If a node is listening on such a port, it will likely receive less
21+
incoming connections.
22+
23+
1: tcpmux
24+
7: echo
25+
9: discard
26+
11: systat
27+
13: daytime
28+
15: netstat
29+
17: qotd
30+
19: chargen
31+
20: ftp data
32+
21: ftp access
33+
22: ssh
34+
23: telnet
35+
25: smtp
36+
37: time
37+
42: name
38+
43: nicname
39+
53: domain
40+
69: tftp
41+
77: priv-rjs
42+
79: finger
43+
87: ttylink
44+
95: supdup
45+
101: hostname
46+
102: iso-tsap
47+
103: gppitnp
48+
104: acr-nema
49+
109: pop2
50+
110: pop3
51+
111: sunrpc
52+
113: auth
53+
115: sftp
54+
117: uucp-path
55+
119: nntp
56+
123: NTP
57+
135: loc-srv /epmap
58+
137: netbios
59+
139: netbios
60+
143: imap2
61+
161: snmp
62+
179: BGP
63+
389: ldap
64+
427: SLP (Also used by Apple Filing Protocol)
65+
465: smtp+ssl
66+
512: print / exec
67+
513: login
68+
514: shell
69+
515: printer
70+
526: tempo
71+
530: courier
72+
531: chat
73+
532: netnews
74+
540: uucp
75+
548: AFP (Apple Filing Protocol)
76+
554: rtsp
77+
556: remotefs
78+
563: nntp+ssl
79+
587: smtp (rfc6409)
80+
601: syslog-conn (rfc3195)
81+
636: ldap+ssl
82+
989: ftps-data
83+
990: ftps
84+
993: ldap+ssl
85+
995: pop3+ssl
86+
1719: h323gatestat
87+
1720: h323hostcall
88+
1723: pptp
89+
2049: nfs
90+
3659: apple-sasl / PasswordServer
91+
4045: lockd
92+
5060: sip
93+
5061: sips
94+
6000: X11
95+
6566: sane-port
96+
6665: Alternate IRC
97+
6666: Alternate IRC
98+
6667: Standard IRC
99+
6668: Alternate IRC
100+
6669: Alternate IRC
101+
6697: IRC + TLS
102+
10080: Amanda
103+
104+
For further information see:
105+
106+
[pull/23306](https://github.com/bitcoin/bitcoin/pull/23306#issuecomment-947516736)
107+
108+
[pull/23542](https://github.com/bitcoin/bitcoin/pull/23542)
109+
110+
[fetch.spec.whatwg.org](https://fetch.spec.whatwg.org/#port-blocking)
111+
112+
[chromium.googlesource.com](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc)
113+
114+
[hg.mozilla.org](https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsIOService.cpp)

‎src/init.cpp‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -566,6 +566,8 @@ void SetupServerArgs(ArgsManager& argsman)
566566
argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
567567
argsman.AddArg("-peertimeout=<n>", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
568568
argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
569+
// TODO: remove the sentence "Nodes not using ... incoming connections." once the changes from
570+
// https://github.com/bitcoin/bitcoin/pull/23542 have become widespread.
569571
argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, regtest: %u) are unlikely to get incoming connections. Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
570572
argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_ELISION, OptionsCategory::CONNECTION);
571573
argsman.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -2270,13 +2272,24 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
22702272
const uint16_t default_bind_port =
22712273
static_cast<uint16_t>(args.GetArg("-port", Params().GetDefaultPort()));
22722274

2275+
const auto BadPortWarning = [](const char* prefix, uint16_t port) {
2276+
return strprintf(_("%s request to listen on port %u. This port is considered \"bad\" and "
2277+
"thus it is unlikely that any Dash Core peers connect to it. See "
2278+
"doc/p2p-bad-ports.md for details and a full list."),
2279+
prefix,
2280+
port);
2281+
};
2282+
22732283
for (const std::string& bind_arg : args.GetArgs("-bind")) {
22742284
std::optional<CService> bind_addr;
22752285
const size_t index = bind_arg.rfind('=');
22762286
if (index == std::string::npos) {
22772287
bind_addr = Lookup(bind_arg, default_bind_port, /*fAllowLookup=*/false);
22782288
if (bind_addr.has_value()) {
22792289
connOptions.vBinds.push_back(bind_addr.value());
2290+
if (IsBadPort(bind_addr.value().GetPort())) {
2291+
InitWarning(BadPortWarning("-bind", bind_addr.value().GetPort()));
2292+
}
22802293
continue;
22812294
}
22822295
} else {
@@ -2304,6 +2317,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
23042317
// on any address - 0.0.0.0 (IPv4) and :: (IPv6).
23052318
connOptions.bind_on_any = args.GetArgs("-bind").empty() && args.GetArgs("-whitebind").empty();
23062319

2320+
// Emit a warning if a bad port is given to -port= but only if -bind and -whitebind are not
2321+
// given, because if they are, then -port= is ignored.
2322+
if (connOptions.bind_on_any && args.IsArgSet("-port")) {
2323+
const uint16_t port_arg = args.GetArg("-port", 0);
2324+
if (IsBadPort(port_arg)) {
2325+
InitWarning(BadPortWarning("-port", port_arg));
2326+
}
2327+
}
2328+
23072329
CService onion_service_target;
23082330
if (!connOptions.onion_binds.empty()) {
23092331
onion_service_target = connOptions.onion_binds.front();

‎src/net.cpp‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3543,12 +3543,26 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
35433543
continue;
35443544
}
35453545

3546-
// Do not allow non-default ports, unless after 50 invalid
3547-
// addresses selected already. This is to prevent malicious peers
3548-
// from advertising themselves as a service on another host and
3549-
// port, causing a DoS attack as nodes around the network attempt
3550-
// to connect to it fruitlessly.
3551-
if ((!isMasternode || !Params().AllowMultiplePorts()) && addr.GetPort() != Params().GetDefaultPort(addr.GetNetwork()) && addr.GetPort() != GetListenPort() && nTries < 50) {
3546+
// Port validation in Dash has additional rules. Some networks are prohibited
3547+
// from using a non-default port while others allow any arbitary port so long
3548+
// it isn't a bad port (and in the case of masternodes, it matches its listen
3549+
// port)
3550+
const bool is_prohibited_port = [this, &addr, &isMasternode](){
3551+
if (!Params().AllowMultiplePorts()) {
3552+
const uint16_t default_port{Params().GetDefaultPort(addr.GetNetwork())};
3553+
assert(!IsBadPort(default_port)); // Make sure we never set the default port to a bad port
3554+
return addr.GetPort() != default_port;
3555+
}
3556+
const bool is_bad_port{IsBadPort(addr.GetPort())};
3557+
if (isMasternode) {
3558+
return addr.GetPort() != GetListenPort() || is_bad_port;
3559+
} else {
3560+
return is_bad_port;
3561+
}
3562+
}();
3563+
3564+
// Do not connect to prohibited ports, unless 50 invalid addresses have been selected already.
3565+
if (nTries < 50 && is_prohibited_port) {
35523566
continue;
35533567
}
35543568

‎src/net_processing.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2474,11 +2474,13 @@ void PeerManagerImpl::RelayAddress(NodeId originator,
24742474
// Relay to a limited number of other nodes
24752475
// Use deterministic randomness to send to the same nodes for 24 hours
24762476
// at a time so the m_addr_knowns of the chosen nodes prevent repeats
2477-
const uint64_t hashAddr{addr.GetHash()};
2477+
const uint64_t hash_addr{CServiceHash(0, 0)(addr)};
24782478
const auto current_time{GetTime<std::chrono::seconds>()};
24792479
// Adding address hash makes exact rotation time different per address, while preserving periodicity.
2480-
const uint64_t time_addr{(static_cast<uint64_t>(count_seconds(current_time)) + hashAddr) / count_seconds(ROTATE_ADDR_RELAY_DEST_INTERVAL)};
2481-
const CSipHasher hasher{m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr).Write(time_addr)};
2480+
const uint64_t time_addr{(static_cast<uint64_t>(count_seconds(current_time)) + hash_addr) / count_seconds(ROTATE_ADDR_RELAY_DEST_INTERVAL)};
2481+
const CSipHasher hasher{m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY)
2482+
.Write(hash_addr)
2483+
.Write(time_addr)};
24822484
FastRandomContext insecure_rand;
24832485

24842486
// Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers.

‎src/netaddress.cpp‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -732,14 +732,6 @@ std::vector<unsigned char> CNetAddr::GetAddrBytes() const
732732
return std::vector<unsigned char>(m_addr.begin(), m_addr.end());
733733
}
734734

735-
uint64_t CNetAddr::GetHash() const
736-
{
737-
uint256 hash = Hash(m_addr);
738-
uint64_t nRet;
739-
memcpy(&nRet, &hash, sizeof(nRet));
740-
return nRet;
741-
}
742-
743735
// private extensions to enum Network, only returned by GetExtNetwork,
744736
// and only used in GetReachabilityFrom
745737
static const int NET_TEREDO = NET_MAX;

‎src/netaddress.h‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,6 @@ class CNetAddr
203203

204204
enum Network GetNetwork() const;
205205
std::string ToStringAddr() const;
206-
uint64_t GetHash() const;
207206
bool GetInAddr(struct in_addr* pipv4Addr) const;
208207
Network GetNetClass() const;
209208

@@ -564,6 +563,14 @@ class CService : public CNetAddr
564563
class CServiceHash
565564
{
566565
public:
566+
CServiceHash()
567+
: m_salt_k0{GetRand(std::numeric_limits<uint64_t>::max())},
568+
m_salt_k1{GetRand(std::numeric_limits<uint64_t>::max())}
569+
{
570+
}
571+
572+
CServiceHash(uint64_t salt_k0, uint64_t salt_k1) : m_salt_k0{salt_k0}, m_salt_k1{salt_k1} {}
573+
567574
size_t operator()(const CService& a) const noexcept
568575
{
569576
CSipHasher hasher(m_salt_k0, m_salt_k1);
@@ -574,8 +581,8 @@ class CServiceHash
574581
}
575582

576583
private:
577-
const uint64_t m_salt_k0 = GetRand(std::numeric_limits<uint64_t>::max());
578-
const uint64_t m_salt_k1 = GetRand(std::numeric_limits<uint64_t>::max());
584+
const uint64_t m_salt_k0;
585+
const uint64_t m_salt_k1;
579586
};
580587

581588
#endif // BITCOIN_NETADDRESS_H

‎src/netbase.cpp‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -720,3 +720,93 @@ void InterruptSocks5(bool interrupt)
720720
{
721721
interruptSocks5Recv = interrupt;
722722
}
723+
724+
bool IsBadPort(uint16_t port)
725+
{
726+
/* Don't forget to update doc/p2p-bad-ports.md if you change this list. */
727+
728+
switch (port) {
729+
case 1: // tcpmux
730+
case 7: // echo
731+
case 9: // discard
732+
case 11: // systat
733+
case 13: // daytime
734+
case 15: // netstat
735+
case 17: // qotd
736+
case 19: // chargen
737+
case 20: // ftp data
738+
case 21: // ftp access
739+
case 22: // ssh
740+
case 23: // telnet
741+
case 25: // smtp
742+
case 37: // time
743+
case 42: // name
744+
case 43: // nicname
745+
case 53: // domain
746+
case 69: // tftp
747+
case 77: // priv-rjs
748+
case 79: // finger
749+
case 87: // ttylink
750+
case 95: // supdup
751+
case 101: // hostname
752+
case 102: // iso-tsap
753+
case 103: // gppitnp
754+
case 104: // acr-nema
755+
case 109: // pop2
756+
case 110: // pop3
757+
case 111: // sunrpc
758+
case 113: // auth
759+
case 115: // sftp
760+
case 117: // uucp-path
761+
case 119: // nntp
762+
case 123: // NTP
763+
case 135: // loc-srv /epmap
764+
case 137: // netbios
765+
case 139: // netbios
766+
case 143: // imap2
767+
case 161: // snmp
768+
case 179: // BGP
769+
case 389: // ldap
770+
case 427: // SLP (Also used by Apple Filing Protocol)
771+
case 465: // smtp+ssl
772+
case 512: // print / exec
773+
case 513: // login
774+
case 514: // shell
775+
case 515: // printer
776+
case 526: // tempo
777+
case 530: // courier
778+
case 531: // chat
779+
case 532: // netnews
780+
case 540: // uucp
781+
case 548: // AFP (Apple Filing Protocol)
782+
case 554: // rtsp
783+
case 556: // remotefs
784+
case 563: // nntp+ssl
785+
case 587: // smtp (rfc6409)
786+
case 601: // syslog-conn (rfc3195)
787+
case 636: // ldap+ssl
788+
case 989: // ftps-data
789+
case 990: // ftps
790+
case 993: // ldap+ssl
791+
case 995: // pop3+ssl
792+
case 1719: // h323gatestat
793+
case 1720: // h323hostcall
794+
case 1723: // pptp
795+
case 2049: // nfs
796+
case 3659: // apple-sasl / PasswordServer
797+
case 4045: // lockd
798+
case 5060: // sip
799+
case 5061: // sips
800+
case 6000: // X11
801+
case 6566: // sane-port
802+
case 6665: // Alternate IRC
803+
case 6666: // Alternate IRC
804+
case 6667: // Standard IRC
805+
case 6668: // Alternate IRC
806+
case 6669: // Alternate IRC
807+
case 6697: // IRC + TLS
808+
case 10080: // Amanda
809+
return true;
810+
}
811+
return false;
812+
}

‎src/netbase.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,4 +248,13 @@ void InterruptSocks5(bool interrupt);
248248
*/
249249
bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* auth, const Sock& socket);
250250

251+
/**
252+
* Determine if a port is "bad" from the perspective of attempting to connect
253+
* to a node on that port.
254+
* @see doc/p2p-bad-ports.md
255+
* @param[in] port Port to check.
256+
* @returns whether the port is bad
257+
*/
258+
bool IsBadPort(uint16_t port);
259+
251260
#endif // BITCOIN_NETBASE_H

‎src/test/fuzz/netaddress.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ FUZZ_TARGET(netaddress)
1616
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
1717

1818
const CNetAddr net_addr = ConsumeNetAddr(fuzzed_data_provider);
19-
(void)net_addr.GetHash();
2019
(void)net_addr.GetNetClass();
2120
if (net_addr.GetNetwork() == Network::NET_IPV4) {
2221
assert(net_addr.IsIPv4());
@@ -81,6 +80,8 @@ FUZZ_TARGET(netaddress)
8180
(void)service.GetKey();
8281
(void)service.GetPort();
8382
(void)service.ToStringAddrPort();
83+
(void)CServiceHash()(service);
84+
(void)CServiceHash(0, 0)(service);
8485

8586
const CNetAddr other_net_addr = ConsumeNetAddr(fuzzed_data_provider);
8687
(void)net_addr.GetReachabilityFrom(other_net_addr);

0 commit comments

Comments
 (0)