Skip to content

Commit 4148e60

Browse files
achow101achow101
committed
Merge bitcoin/bitcoin#30679: fix: handle invalid -rpcbind port earlier
e6994ef fix: increase rpcbind check robustness (tdb3) d38e3ae fix: handle invalid rpcbind port earlier (tdb3) 83b67f2 refactor: move host/port checking (tdb3) 73c2439 test: add tests for invalid rpcbind ports (tdb3) Pull request description: Previously, when an invalid port was specified in `-rpcbind`, the `SplitHostPort()` return value in `HTTPBindAddresses()` was ignored and attempt would be made to bind to the default rpcbind port (with the host/port treated as a host). This rearranges port checking code in `AppInitMain()` to handle the invalid port before reaching `HTTPBindAddresses()`. Also adds a check in `HTTPBindAddresses()` as a defensive measure for future changes. Adds then updates associated functional tests as well. ACKs for top commit: achow101: ACK e6994ef ryanofsky: Code review ACK e6994ef zaidmstrr: Code review ACK [e6994ef](bitcoin/bitcoin@e6994ef) Tree-SHA512: bcc3e5ceef21963821cd16ce6ecb83d5c5657755882c05872a7cfe661a1492b1d631f54de22f41fdd173512d62dd15dc37e394fe1a7abe4de484b82cd2438b92
2 parents a8a2628 + e6994ef commit 4148e60

File tree

3 files changed

+74
-45
lines changed

3 files changed

+74
-45
lines changed

‎src/httpserver.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
#include <chainparamsbase.h>
1010
#include <common/args.h>
11+
#include <common/messages.h>
1112
#include <compat/compat.h>
1213
#include <logging.h>
1314
#include <netbase.h>
@@ -43,6 +44,8 @@
4344

4445
#include <support/events.h>
4546

47+
using common::InvalidPortErrMsg;
48+
4649
/** Maximum size of http request (request line + headers) */
4750
static const size_t MAX_HEADERS_SIZE = 8192;
4851

@@ -374,7 +377,10 @@ static bool HTTPBindAddresses(struct evhttp* http)
374377
for (const std::string& strRPCBind : gArgs.GetArgs("-rpcbind")) {
375378
uint16_t port{http_port};
376379
std::string host;
377-
SplitHostPort(strRPCBind, port, host);
380+
if (!SplitHostPort(strRPCBind, port, host)) {
381+
LogError("%s\n", InvalidPortErrMsg("-rpcbind", strRPCBind).original);
382+
return false;
383+
}
378384
endpoints.emplace_back(host, port);
379385
}
380386
}

‎src/init.cpp

Lines changed: 50 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1145,6 +1145,53 @@ bool AppInitInterfaces(NodeContext& node)
11451145
return true;
11461146
}
11471147

1148+
bool CheckHostPortOptions(const ArgsManager& args) {
1149+
for (const std::string port_option : {
1150+
"-port",
1151+
"-rpcport",
1152+
}) {
1153+
if (args.IsArgSet(port_option)) {
1154+
const std::string port = args.GetArg(port_option, "");
1155+
uint16_t n;
1156+
if (!ParseUInt16(port, &n) || n == 0) {
1157+
return InitError(InvalidPortErrMsg(port_option, port));
1158+
}
1159+
}
1160+
}
1161+
1162+
for ([[maybe_unused]] const auto& [arg, unix] : std::vector<std::pair<std::string, bool>>{
1163+
// arg name UNIX socket support
1164+
{"-i2psam", false},
1165+
{"-onion", true},
1166+
{"-proxy", true},
1167+
{"-rpcbind", false},
1168+
{"-torcontrol", false},
1169+
{"-whitebind", false},
1170+
{"-zmqpubhashblock", true},
1171+
{"-zmqpubhashtx", true},
1172+
{"-zmqpubrawblock", true},
1173+
{"-zmqpubrawtx", true},
1174+
{"-zmqpubsequence", true},
1175+
}) {
1176+
for (const std::string& socket_addr : args.GetArgs(arg)) {
1177+
std::string host_out;
1178+
uint16_t port_out{0};
1179+
if (!SplitHostPort(socket_addr, port_out, host_out)) {
1180+
#ifdef HAVE_SOCKADDR_UN
1181+
// Allow unix domain sockets for some options e.g. unix:/some/file/path
1182+
if (!unix || !socket_addr.starts_with(ADDR_PREFIX_UNIX)) {
1183+
return InitError(InvalidPortErrMsg(arg, socket_addr));
1184+
}
1185+
#else
1186+
return InitError(InvalidPortErrMsg(arg, socket_addr));
1187+
#endif
1188+
}
1189+
}
1190+
}
1191+
1192+
return true;
1193+
}
1194+
11481195
bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
11491196
{
11501197
const ArgsManager& args = *Assert(node.args);
@@ -1232,6 +1279,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
12321279
RegisterZMQRPCCommands(tableRPC);
12331280
#endif
12341281

1282+
// Check port numbers
1283+
if (!CheckHostPortOptions(args)) return false;
1284+
12351285
/* Start the RPC server already. It will be started in "warmup" mode
12361286
* and not really process calls already (but it will signify connections
12371287
* that the server is there and will be ready later). Warmup mode will
@@ -1322,50 +1372,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
13221372
validation_signals.RegisterValidationInterface(fee_estimator);
13231373
}
13241374

1325-
// Check port numbers
1326-
for (const std::string port_option : {
1327-
"-port",
1328-
"-rpcport",
1329-
}) {
1330-
if (args.IsArgSet(port_option)) {
1331-
const std::string port = args.GetArg(port_option, "");
1332-
uint16_t n;
1333-
if (!ParseUInt16(port, &n) || n == 0) {
1334-
return InitError(InvalidPortErrMsg(port_option, port));
1335-
}
1336-
}
1337-
}
1338-
1339-
for ([[maybe_unused]] const auto& [arg, unix] : std::vector<std::pair<std::string, bool>>{
1340-
// arg name UNIX socket support
1341-
{"-i2psam", false},
1342-
{"-onion", true},
1343-
{"-proxy", true},
1344-
{"-rpcbind", false},
1345-
{"-torcontrol", false},
1346-
{"-whitebind", false},
1347-
{"-zmqpubhashblock", true},
1348-
{"-zmqpubhashtx", true},
1349-
{"-zmqpubrawblock", true},
1350-
{"-zmqpubrawtx", true},
1351-
{"-zmqpubsequence", true},
1352-
}) {
1353-
for (const std::string& socket_addr : args.GetArgs(arg)) {
1354-
std::string host_out;
1355-
uint16_t port_out{0};
1356-
if (!SplitHostPort(socket_addr, port_out, host_out)) {
1357-
#ifdef HAVE_SOCKADDR_UN
1358-
// Allow unix domain sockets for some options e.g. unix:/some/file/path
1359-
if (!unix || !socket_addr.starts_with(ADDR_PREFIX_UNIX)) {
1360-
return InitError(InvalidPortErrMsg(arg, socket_addr));
1361-
}
1362-
#else
1363-
return InitError(InvalidPortErrMsg(arg, socket_addr));
1364-
#endif
1365-
}
1366-
}
1367-
}
1368-
13691375
for (const std::string& socket_addr : args.GetArgs("-bind")) {
13701376
std::string host_out;
13711377
uint16_t port_out{0};

‎test/functional/rpc_bind.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,19 @@ def run_bind_test(self, allow_ips, connect_to, addresses, expected):
4545
assert_equal(set(get_bind_addrs(pid)), set(expected))
4646
self.stop_nodes()
4747

48+
def run_invalid_bind_test(self, allow_ips, addresses):
49+
'''
50+
Attempt to start a node with requested rpcallowip and rpcbind
51+
parameters, expecting that the node will fail.
52+
'''
53+
self.log.info(f'Invalid bind test for {addresses}')
54+
base_args = ['-disablewallet', '-nolisten']
55+
if allow_ips:
56+
base_args += ['-rpcallowip=' + x for x in allow_ips]
57+
init_error = 'Error: Invalid port specified in -rpcbind: '
58+
for addr in addresses:
59+
self.nodes[0].assert_start_raises_init_error(base_args + [f'-rpcbind={addr}'], init_error + f"'{addr}'")
60+
4861
def run_allowip_test(self, allow_ips, rpchost, rpcport):
4962
'''
5063
Start a node with rpcallow IP, and request getnetworkinfo
@@ -84,6 +97,10 @@ def run_test(self):
8497

8598
if not self.options.run_nonloopback:
8699
self._run_loopback_tests()
100+
if self.options.run_ipv4:
101+
self.run_invalid_bind_test(['127.0.0.1'], ['127.0.0.1:notaport', '127.0.0.1:-18443', '127.0.0.1:0', '127.0.0.1:65536'])
102+
if self.options.run_ipv6:
103+
self.run_invalid_bind_test(['[::1]'], ['[::1]:notaport', '[::1]:-18443', '[::1]:0', '[::1]:65536'])
87104
if not self.options.run_ipv4 and not self.options.run_ipv6:
88105
self._run_nonloopback_tests()
89106

0 commit comments

Comments
 (0)