Skip to content

Commit 5fc3939

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#22087: Validate port-options
0452678 Validate `port` options (amadeuszpawlik) f8387c4 Validate port value in `SplitHostPort` (amadeuszpawlik) Pull request description: Validate `port`-options, so that invalid values are rejected early in the startup. Ports are `uint16_t`s, which effectively limits a port's value to <=65535. As discussed in bitcoin/bitcoin#24116 and bitcoin/bitcoin#24344, port "0" is considered invalid too. Proposed in bitcoin/bitcoin#21893 (comment) The `SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut)` now returns a bool that indicates whether the port value was set and within the allowed range. This is an improvement that can be used not only for port validation of options at startup, but also in rpc calls, etc, ACKs for top commit: luke-jr: utACK 0452678 ryanofsky: Code review ACK 0452678. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding 'GetArgs[0]` problem. Tree-SHA512: f1ac80bf98520b287a6413ceadb41bc3a93c491955de9b9319ee1298ac0ab982751905762a287e748997ead6198a8bb7a3bc8817ac9e3d2468e11ab4a0f8496d
2 parents 2e77dff + 0452678 commit 5fc3939

File tree

8 files changed

+117
-14
lines changed

8 files changed

+117
-14
lines changed

‎doc/release-notes-22087.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Updated settings
2+
----------------
3+
4+
- Ports specified in `-port` and `-rpcport` options are now validated at startup. Values that previously worked and were considered valid can now result in errors. (#22087)

‎src/init.cpp

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1255,6 +1255,51 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
12551255
// as they would never get updated.
12561256
if (!ignores_incoming_txs) node.fee_estimator = std::make_unique<CBlockPolicyEstimator>(FeeestPath(args));
12571257

1258+
// Check port numbers
1259+
for (const std::string port_option : {
1260+
"-port",
1261+
"-rpcport",
1262+
}) {
1263+
if (args.IsArgSet(port_option)) {
1264+
const std::string port = args.GetArg(port_option, "");
1265+
uint16_t n;
1266+
if (!ParseUInt16(port, &n) || n == 0) {
1267+
return InitError(InvalidPortErrMsg(port_option, port));
1268+
}
1269+
}
1270+
}
1271+
1272+
for (const std::string port_option : {
1273+
"-i2psam",
1274+
"-onion",
1275+
"-proxy",
1276+
"-rpcbind",
1277+
"-torcontrol",
1278+
"-whitebind",
1279+
"-zmqpubhashblock",
1280+
"-zmqpubhashtx",
1281+
"-zmqpubrawblock",
1282+
"-zmqpubrawtx",
1283+
"-zmqpubsequence",
1284+
}) {
1285+
for (const std::string& socket_addr : args.GetArgs(port_option)) {
1286+
std::string host_out;
1287+
uint16_t port_out{0};
1288+
if (!SplitHostPort(socket_addr, port_out, host_out)) {
1289+
return InitError(InvalidPortErrMsg(port_option, socket_addr));
1290+
}
1291+
}
1292+
}
1293+
1294+
for (const std::string& socket_addr : args.GetArgs("-bind")) {
1295+
std::string host_out;
1296+
uint16_t port_out{0};
1297+
std::string bind_socket_addr = socket_addr.substr(0, socket_addr.rfind('='));
1298+
if (!SplitHostPort(bind_socket_addr, port_out, host_out)) {
1299+
return InitError(InvalidPortErrMsg("-bind", socket_addr));
1300+
}
1301+
}
1302+
12581303
// sanitize comments per BIP-0014, format user agent and check total size
12591304
std::vector<std::string> uacomments;
12601305
for (const std::string& cmt : args.GetArgs("-uacomment")) {

‎src/test/netbase_tests.cpp

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,12 @@ BOOST_AUTO_TEST_CASE(netbase_properties)
8484

8585
}
8686

87-
bool static TestSplitHost(const std::string& test, const std::string& host, uint16_t port)
87+
bool static TestSplitHost(const std::string& test, const std::string& host, uint16_t port, bool validPort=true)
8888
{
8989
std::string hostOut;
9090
uint16_t portOut{0};
91-
SplitHostPort(test, portOut, hostOut);
92-
return hostOut == host && port == portOut;
91+
bool validPortOut = SplitHostPort(test, portOut, hostOut);
92+
return hostOut == host && portOut == port && validPortOut == validPort;
9393
}
9494

9595
BOOST_AUTO_TEST_CASE(netbase_splithost)
@@ -109,6 +109,23 @@ BOOST_AUTO_TEST_CASE(netbase_splithost)
109109
BOOST_CHECK(TestSplitHost(":8333", "", 8333));
110110
BOOST_CHECK(TestSplitHost("[]:8333", "", 8333));
111111
BOOST_CHECK(TestSplitHost("", "", 0));
112+
BOOST_CHECK(TestSplitHost(":65535", "", 65535));
113+
BOOST_CHECK(TestSplitHost(":65536", ":65536", 0, false));
114+
BOOST_CHECK(TestSplitHost(":-1", ":-1", 0, false));
115+
BOOST_CHECK(TestSplitHost("[]:70001", "[]:70001", 0, false));
116+
BOOST_CHECK(TestSplitHost("[]:-1", "[]:-1", 0, false));
117+
BOOST_CHECK(TestSplitHost("[]:-0", "[]:-0", 0, false));
118+
BOOST_CHECK(TestSplitHost("[]:0", "", 0, false));
119+
BOOST_CHECK(TestSplitHost("[]:1/2", "[]:1/2", 0, false));
120+
BOOST_CHECK(TestSplitHost("[]:1E2", "[]:1E2", 0, false));
121+
BOOST_CHECK(TestSplitHost("127.0.0.1:65536", "127.0.0.1:65536", 0, false));
122+
BOOST_CHECK(TestSplitHost("127.0.0.1:0", "127.0.0.1", 0, false));
123+
BOOST_CHECK(TestSplitHost("127.0.0.1:", "127.0.0.1:", 0, false));
124+
BOOST_CHECK(TestSplitHost("127.0.0.1:1/2", "127.0.0.1:1/2", 0, false));
125+
BOOST_CHECK(TestSplitHost("127.0.0.1:1E2", "127.0.0.1:1E2", 0, false));
126+
BOOST_CHECK(TestSplitHost("www.bitcoincore.org:65536", "www.bitcoincore.org:65536", 0, false));
127+
BOOST_CHECK(TestSplitHost("www.bitcoincore.org:0", "www.bitcoincore.org", 0, false));
128+
BOOST_CHECK(TestSplitHost("www.bitcoincore.org:", "www.bitcoincore.org:", 0, false));
112129
}
113130

114131
bool static TestParse(std::string src, std::string canon)

‎src/util/error.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,11 @@ bilingual_str ResolveErrMsg(const std::string& optname, const std::string& strBi
4949
return strprintf(_("Cannot resolve -%s address: '%s'"), optname, strBind);
5050
}
5151

52+
bilingual_str InvalidPortErrMsg(const std::string& optname, const std::string& invalid_value)
53+
{
54+
return strprintf(_("Invalid port specified in %s: '%s'"), optname, invalid_value);
55+
}
56+
5257
bilingual_str AmountHighWarn(const std::string& optname)
5358
{
5459
return strprintf(_("%s is set very high!"), optname);

‎src/util/error.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ bilingual_str TransactionErrorString(const TransactionError error);
3939

4040
bilingual_str ResolveErrMsg(const std::string& optname, const std::string& strBind);
4141

42+
bilingual_str InvalidPortErrMsg(const std::string& optname, const std::string& strPort);
43+
4244
bilingual_str AmountHighWarn(const std::string& optname);
4345

4446
bilingual_str AmountErrMsg(const std::string& optname, const std::string& strValue);

‎src/util/strencodings.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,9 @@ std::vector<Byte> ParseHex(std::string_view str)
9797
template std::vector<std::byte> ParseHex(std::string_view);
9898
template std::vector<uint8_t> ParseHex(std::string_view);
9999

100-
void SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut)
100+
bool SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut)
101101
{
102+
bool valid = false;
102103
size_t colon = in.find_last_of(':');
103104
// if a : is found, and it either follows a [...], or no other : is in the string, treat it as port separator
104105
bool fHaveColon = colon != in.npos;
@@ -109,13 +110,18 @@ void SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut)
109110
if (ParseUInt16(in.substr(colon + 1), &n)) {
110111
in = in.substr(0, colon);
111112
portOut = n;
113+
valid = (portOut != 0);
112114
}
115+
} else {
116+
valid = true;
113117
}
114118
if (in.size() > 0 && in[0] == '[' && in[in.size() - 1] == ']') {
115119
hostOut = in.substr(1, in.size() - 2);
116120
} else {
117121
hostOut = in;
118122
}
123+
124+
return valid;
119125
}
120126

121127
std::string EncodeBase64(Span<const unsigned char> input)

‎src/util/strencodings.h

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,16 @@ std::string EncodeBase32(Span<const unsigned char> input, bool pad = true);
8888
*/
8989
std::string EncodeBase32(std::string_view str, bool pad = true);
9090

91-
void SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut);
91+
/**
92+
* Splits socket address string into host string and port value.
93+
* Validates port value.
94+
*
95+
* @param[in] in The socket address string to split.
96+
* @param[out] portOut Port-portion of the input, if found and parsable.
97+
* @param[out] hostOut Host-portion of the input, if found.
98+
* @return true if port-portion is absent or within its allowed range, otherwise false
99+
*/
100+
bool SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut);
92101

93102
// LocaleIndependentAtoi is provided for backwards compatibility reasons.
94103
//

‎test/functional/feature_proxy.py

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -317,19 +317,34 @@ def networks_dict(d):
317317

318318
self.stop_node(1)
319319

320-
self.log.info("Test passing invalid -proxy raises expected init error")
321-
self.nodes[1].extra_args = ["-proxy=abc:def"]
322-
msg = "Error: Invalid -proxy address or hostname: 'abc:def'"
320+
self.log.info("Test passing invalid -proxy hostname raises expected init error")
321+
self.nodes[1].extra_args = ["-proxy=abc..abc:23456"]
322+
msg = "Error: Invalid -proxy address or hostname: 'abc..abc:23456'"
323323
self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
324324

325-
self.log.info("Test passing invalid -onion raises expected init error")
326-
self.nodes[1].extra_args = ["-onion=xyz:abc"]
327-
msg = "Error: Invalid -onion address or hostname: 'xyz:abc'"
325+
self.log.info("Test passing invalid -proxy port raises expected init error")
326+
self.nodes[1].extra_args = ["-proxy=192.0.0.1:def"]
327+
msg = "Error: Invalid port specified in -proxy: '192.0.0.1:def'"
328328
self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
329329

330-
self.log.info("Test passing invalid -i2psam raises expected init error")
331-
self.nodes[1].extra_args = ["-i2psam=def:xyz"]
332-
msg = "Error: Invalid -i2psam address or hostname: 'def:xyz'"
330+
self.log.info("Test passing invalid -onion hostname raises expected init error")
331+
self.nodes[1].extra_args = ["-onion=xyz..xyz:23456"]
332+
msg = "Error: Invalid -onion address or hostname: 'xyz..xyz:23456'"
333+
self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
334+
335+
self.log.info("Test passing invalid -onion port raises expected init error")
336+
self.nodes[1].extra_args = ["-onion=192.0.0.1:def"]
337+
msg = "Error: Invalid port specified in -onion: '192.0.0.1:def'"
338+
self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
339+
340+
self.log.info("Test passing invalid -i2psam hostname raises expected init error")
341+
self.nodes[1].extra_args = ["-i2psam=def..def:23456"]
342+
msg = "Error: Invalid -i2psam address or hostname: 'def..def:23456'"
343+
self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
344+
345+
self.log.info("Test passing invalid -i2psam port raises expected init error")
346+
self.nodes[1].extra_args = ["-i2psam=192.0.0.1:def"]
347+
msg = "Error: Invalid port specified in -i2psam: '192.0.0.1:def'"
333348
self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
334349

335350
self.log.info("Test passing invalid -onlynet=i2p without -i2psam raises expected init error")

0 commit comments

Comments
 (0)