From e208fb5d3bea4c1fb750cb0028819635ecdeb415 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sat, 27 Apr 2024 18:09:12 -0400 Subject: cli: Sanitize ports in rpcconnect and rpcport Adds error handling of invalid ports to rpcconnect and rpcport, with associated functional tests. --- src/bitcoin-cli.cpp | 31 ++++++++++++++++++-- test/functional/interface_bitcoin_cli.py | 49 ++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index c7ba2204c3..eb1ee6134a 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -743,8 +743,35 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co // 2. port in -rpcconnect (ie following : in ipv4 or ]: in ipv6) // 3. default port for chain uint16_t port{BaseParams().RPCPort()}; - SplitHostPort(gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT), port, host); - port = static_cast(gArgs.GetIntArg("-rpcport", port)); + { + uint16_t rpcconnect_port{0}; + const std::string rpcconnect_str = gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT); + if (!SplitHostPort(rpcconnect_str, rpcconnect_port, host)) { + // Uses argument provided as-is + // (rather than value parsed) + // to aid the user in troubleshooting + throw std::runtime_error(strprintf("Invalid port provided in -rpcconnect: %s", rpcconnect_str)); + } else { + if (rpcconnect_port != 0) { + // Use the valid port provided in rpcconnect + port = rpcconnect_port; + } // else, no port was provided in rpcconnect (continue using default one) + } + + if (std::optional rpcport_arg = gArgs.GetArg("-rpcport")) { + // -rpcport was specified + const uint16_t rpcport_int{ToIntegral(rpcport_arg.value()).value_or(0)}; + if (rpcport_int == 0) { + // Uses argument provided as-is + // (rather than value parsed) + // to aid the user in troubleshooting + throw std::runtime_error(strprintf("Invalid port provided in -rpcport: %s", rpcport_arg.value())); + } + + // Use the valid port provided + port = rpcport_int; + } + } // Obtain event base raii_event_base base = obtain_event_base(); diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index 83bb5121e5..a6628dcbf3 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -8,6 +8,7 @@ from decimal import Decimal import re from test_framework.blocktools import COINBASE_MATURITY +from test_framework.netutil import test_ipv6_local from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -15,6 +16,7 @@ from test_framework.util import ( assert_raises_process_error, assert_raises_rpc_error, get_auth_cookie, + rpc_port, ) import time @@ -107,6 +109,53 @@ class TestBitcoinCli(BitcoinTestFramework): self.log.info("Test connecting to a non-existing server") assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('-rpcport=1').echo) + self.log.info("Test handling of invalid ports in rpcconnect") + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:notaport", self.nodes[0].cli("-rpcconnect=127.0.0.1:notaport").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:-1", self.nodes[0].cli("-rpcconnect=127.0.0.1:-1").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:0", self.nodes[0].cli("-rpcconnect=127.0.0.1:0").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:65536", self.nodes[0].cli("-rpcconnect=127.0.0.1:65536").echo) + + self.log.info("Checking for IPv6") + have_ipv6 = test_ipv6_local() + if not have_ipv6: + self.log.info("Skipping IPv6 tests") + + if have_ipv6: + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:notaport", self.nodes[0].cli("-rpcconnect=[::1]:notaport").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:-1", self.nodes[0].cli("-rpcconnect=[::1]:-1").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:0", self.nodes[0].cli("-rpcconnect=[::1]:0").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:65536", self.nodes[0].cli("-rpcconnect=[::1]:65536").echo) + + self.log.info("Test handling of invalid ports in rpcport") + assert_raises_process_error(1, "Invalid port provided in -rpcport: notaport", self.nodes[0].cli("-rpcport=notaport").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcport: -1", self.nodes[0].cli("-rpcport=-1").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcport: 0", self.nodes[0].cli("-rpcport=0").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcport: 65536", self.nodes[0].cli("-rpcport=65536").echo) + + self.log.info("Test port usage preferences") + node_rpc_port = rpc_port(self.nodes[0].index) + # Prevent bitcoin-cli from using existing rpcport in conf + conf_rpcport = "rpcport=" + str(node_rpc_port) + self.nodes[0].replace_in_config([(conf_rpcport, "#" + conf_rpcport)]) + # prefer rpcport over rpcconnect + assert_raises_process_error(1, "Could not connect to the server 127.0.0.1:1", self.nodes[0].cli(f"-rpcconnect=127.0.0.1:{node_rpc_port}", "-rpcport=1").echo) + if have_ipv6: + assert_raises_process_error(1, "Could not connect to the server ::1:1", self.nodes[0].cli(f"-rpcconnect=[::1]:{node_rpc_port}", "-rpcport=1").echo) + + assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=127.0.0.1:18999", f'-rpcport={node_rpc_port}').getblockcount()) + if have_ipv6: + assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=[::1]:18999", f'-rpcport={node_rpc_port}').getblockcount()) + + # prefer rpcconnect port over default + assert_equal(BLOCKS, self.nodes[0].cli(f"-rpcconnect=127.0.0.1:{node_rpc_port}").getblockcount()) + if have_ipv6: + assert_equal(BLOCKS, self.nodes[0].cli(f"-rpcconnect=[::1]:{node_rpc_port}").getblockcount()) + + # prefer rpcport over default + assert_equal(BLOCKS, self.nodes[0].cli(f'-rpcport={node_rpc_port}').getblockcount()) + # Re-enable rpcport in conf if present + self.nodes[0].replace_in_config([("#" + conf_rpcport, conf_rpcport)]) + self.log.info("Test connecting with non-existing RPC cookie file") assert_raises_process_error(1, "Could not locate RPC credentials", self.nodes[0].cli('-rpccookiefile=does-not-exist', '-rpcpassword=').echo) -- cgit v1.2.3 From 24bc46c83b39149f4845a575a82337eb46d91bdb Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sat, 27 Apr 2024 18:10:57 -0400 Subject: cli: Add warning for duplicate port definition Adds a warning when both -rpcconnect and -rpcport define a port to be used. --- src/bitcoin-cli.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index eb1ee6134a..6352044ba7 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -770,6 +770,12 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co // Use the valid port provided port = rpcport_int; + + // If there was a valid port provided in rpcconnect, + // rpcconnect_port is non-zero. + if (rpcconnect_port != 0) { + tfm::format(std::cerr, "Warning: Port specified in both -rpcconnect and -rpcport. Using -rpcport %u\n", port); + } } } -- cgit v1.2.3