Merge bitcoin/bitcoin#22087: Validate port-options

04526787b5f6613d1f1ad78434e1dd24ab88dd76 Validate `port` options (amadeuszpawlik)
f8387c42343867779170a0f96ef64e6acff5c481 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 https://github.com/bitcoin/bitcoin/pull/24116 and https://github.com/bitcoin/bitcoin/pull/24344, port "0" is considered invalid too.
  Proposed in https://github.com/bitcoin/bitcoin/issues/21893#issuecomment-835784223

  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 04526787b5f6613d1f1ad78434e1dd24ab88dd76
  ryanofsky:
    Code review ACK 04526787b5f6613d1f1ad78434e1dd24ab88dd76. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding 'GetArgs[0]` problem.

Tree-SHA512: f1ac80bf98520b287a6413ceadb41bc3a93c491955de9b9319ee1298ac0ab982751905762a287e748997ead6198a8bb7a3bc8817ac9e3d2468e11ab4a0f8496d

8 files changed, 117 insertions, 14 deletions

diff --git a/doc/release-notes-22087.md b/doc/release-notes-22087.md
new file mode 100644
index 0000000000..8d7fd242b2
--- /dev/null
+++ b/doc/release-notes-22087.md
@@ -0,0 +1,4 @@
+Updated settings
+----------------
+
+- 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)
diff --git a/src/init.cpp b/src/init.cpp
index 25b40c6c6e..8ffab64622 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1255,6 +1255,51 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     // as they would never get updated.
     if (!ignores_incoming_txs) node.fee_estimator = std::make_unique<CBlockPolicyEstimator>(FeeestPath(args));
 
+    // Check port numbers
+    for (const std::string port_option : {
+        "-port",
+        "-rpcport",
+    }) {
+        if (args.IsArgSet(port_option)) {
+            const std::string port = args.GetArg(port_option, "");
+            uint16_t n;
+            if (!ParseUInt16(port, &n) || n == 0) {
+                return InitError(InvalidPortErrMsg(port_option, port));
+            }
+        }
+    }
+
+    for (const std::string port_option : {
+        "-i2psam",
+        "-onion",
+        "-proxy",
+        "-rpcbind",
+        "-torcontrol",
+        "-whitebind",
+        "-zmqpubhashblock",
+        "-zmqpubhashtx",
+        "-zmqpubrawblock",
+        "-zmqpubrawtx",
+        "-zmqpubsequence",
+    }) {
+        for (const std::string& socket_addr : args.GetArgs(port_option)) {
+            std::string host_out;
+            uint16_t port_out{0};
+            if (!SplitHostPort(socket_addr, port_out, host_out)) {
+                return InitError(InvalidPortErrMsg(port_option, socket_addr));
+            }
+        }
+    }
+
+    for (const std::string& socket_addr : args.GetArgs("-bind")) {
+        std::string host_out;
+        uint16_t port_out{0};
+        std::string bind_socket_addr = socket_addr.substr(0, socket_addr.rfind('='));
+        if (!SplitHostPort(bind_socket_addr, port_out, host_out)) {
+            return InitError(InvalidPortErrMsg("-bind", socket_addr));
+        }
+    }
+
     // sanitize comments per BIP-0014, format user agent and check total size
     std::vector<std::string> uacomments;
     for (const std::string& cmt : args.GetArgs("-uacomment")) {
diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
index c2d2fa37b4..0e1e9ae211 100644
--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -84,12 +84,12 @@ BOOST_AUTO_TEST_CASE(netbase_properties)
 
 }
 
-bool static TestSplitHost(const std::string& test, const std::string& host, uint16_t port)
+bool static TestSplitHost(const std::string& test, const std::string& host, uint16_t port, bool validPort=true)
 {
     std::string hostOut;
     uint16_t portOut{0};
-    SplitHostPort(test, portOut, hostOut);
-    return hostOut == host && port == portOut;
+    bool validPortOut = SplitHostPort(test, portOut, hostOut);
+    return hostOut == host && portOut == port && validPortOut == validPort;
 }
 
 BOOST_AUTO_TEST_CASE(netbase_splithost)
@@ -109,6 +109,23 @@ BOOST_AUTO_TEST_CASE(netbase_splithost)
     BOOST_CHECK(TestSplitHost(":8333", "", 8333));
     BOOST_CHECK(TestSplitHost("[]:8333", "", 8333));
     BOOST_CHECK(TestSplitHost("", "", 0));
+    BOOST_CHECK(TestSplitHost(":65535", "", 65535));
+    BOOST_CHECK(TestSplitHost(":65536", ":65536", 0, false));
+    BOOST_CHECK(TestSplitHost(":-1", ":-1", 0, false));
+    BOOST_CHECK(TestSplitHost("[]:70001", "[]:70001", 0, false));
+    BOOST_CHECK(TestSplitHost("[]:-1", "[]:-1", 0, false));
+    BOOST_CHECK(TestSplitHost("[]:-0", "[]:-0", 0, false));
+    BOOST_CHECK(TestSplitHost("[]:0", "", 0, false));
+    BOOST_CHECK(TestSplitHost("[]:1/2", "[]:1/2", 0, false));
+    BOOST_CHECK(TestSplitHost("[]:1E2", "[]:1E2", 0, false));
+    BOOST_CHECK(TestSplitHost("127.0.0.1:65536", "127.0.0.1:65536", 0, false));
+    BOOST_CHECK(TestSplitHost("127.0.0.1:0", "127.0.0.1", 0, false));
+    BOOST_CHECK(TestSplitHost("127.0.0.1:", "127.0.0.1:", 0, false));
+    BOOST_CHECK(TestSplitHost("127.0.0.1:1/2", "127.0.0.1:1/2", 0, false));
+    BOOST_CHECK(TestSplitHost("127.0.0.1:1E2", "127.0.0.1:1E2", 0, false));
+    BOOST_CHECK(TestSplitHost("www.bitcoincore.org:65536", "www.bitcoincore.org:65536", 0, false));
+    BOOST_CHECK(TestSplitHost("www.bitcoincore.org:0", "www.bitcoincore.org", 0, false));
+    BOOST_CHECK(TestSplitHost("www.bitcoincore.org:", "www.bitcoincore.org:", 0, false));
 }
 
 bool static TestParse(std::string src, std::string canon)
diff --git a/src/util/error.cpp b/src/util/error.cpp
index 33a35a6d59..390cb6c11b 100644
--- a/src/util/error.cpp
+++ b/src/util/error.cpp
@@ -49,6 +49,11 @@ bilingual_str ResolveErrMsg(const std::string& optname, const std::string& strBi
     return strprintf(_("Cannot resolve -%s address: '%s'"), optname, strBind);
 }
 
+bilingual_str InvalidPortErrMsg(const std::string& optname, const std::string& invalid_value)
+{
+    return strprintf(_("Invalid port specified in %s: '%s'"), optname, invalid_value);
+}
+
 bilingual_str AmountHighWarn(const std::string& optname)
 {
     return strprintf(_("%s is set very high!"), optname);
diff --git a/src/util/error.h b/src/util/error.h
index 0429de651a..27916501f0 100644
--- a/src/util/error.h
+++ b/src/util/error.h
@@ -39,6 +39,8 @@ bilingual_str TransactionErrorString(const TransactionError error);
 
 bilingual_str ResolveErrMsg(const std::string& optname, const std::string& strBind);
 
+bilingual_str InvalidPortErrMsg(const std::string& optname, const std::string& strPort);
+
 bilingual_str AmountHighWarn(const std::string& optname);
 
 bilingual_str AmountErrMsg(const std::string& optname, const std::string& strValue);
diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp
index b5ac151374..e28ca8e73a 100644
--- a/src/util/strencodings.cpp
+++ b/src/util/strencodings.cpp
@@ -97,8 +97,9 @@ std::vector<Byte> ParseHex(std::string_view str)
 template std::vector<std::byte> ParseHex(std::string_view);
 template std::vector<uint8_t> ParseHex(std::string_view);
 
-void SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut)
+bool SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut)
 {
+    bool valid = false;
     size_t colon = in.find_last_of(':');
     // if a : is found, and it either follows a [...], or no other : is in the string, treat it as port separator
     bool fHaveColon = colon != in.npos;
@@ -109,13 +110,18 @@ void SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut)
         if (ParseUInt16(in.substr(colon + 1), &n)) {
             in = in.substr(0, colon);
             portOut = n;
+            valid = (portOut != 0);
         }
+    } else {
+        valid = true;
     }
     if (in.size() > 0 && in[0] == '[' && in[in.size() - 1] == ']') {
         hostOut = in.substr(1, in.size() - 2);
     } else {
         hostOut = in;
     }
+
+    return valid;
 }
 
 std::string EncodeBase64(Span<const unsigned char> input)
diff --git a/src/util/strencodings.h b/src/util/strencodings.h
index 14867b21b2..94bc6cc2f3 100644
--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -88,7 +88,16 @@ std::string EncodeBase32(Span<const unsigned char> input, bool pad = true);
  */
 std::string EncodeBase32(std::string_view str, bool pad = true);
 
-void SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut);
+/**
+ * Splits socket address string into host string and port value.
+ * Validates port value.
+ *
+ * @param[in] in        The socket address string to split.
+ * @param[out] portOut  Port-portion of the input, if found and parsable.
+ * @param[out] hostOut  Host-portion of the input, if found.
+ * @return              true if port-portion is absent or within its allowed range, otherwise false
+ */
+bool SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut);
 
 // LocaleIndependentAtoi is provided for backwards compatibility reasons.
 //
diff --git a/test/functional/feature_proxy.py b/test/functional/feature_proxy.py
index c90852562e..18b079cd71 100755
--- a/test/functional/feature_proxy.py
+++ b/test/functional/feature_proxy.py
@@ -317,19 +317,34 @@ class ProxyTest(BitcoinTestFramework):
 
         self.stop_node(1)
 
-        self.log.info("Test passing invalid -proxy raises expected init error")
-        self.nodes[1].extra_args = ["-proxy=abc:def"]
-        msg = "Error: Invalid -proxy address or hostname: 'abc:def'"
+        self.log.info("Test passing invalid -proxy hostname raises expected init error")
+        self.nodes[1].extra_args = ["-proxy=abc..abc:23456"]
+        msg = "Error: Invalid -proxy address or hostname: 'abc..abc:23456'"
         self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
 
-        self.log.info("Test passing invalid -onion raises expected init error")
-        self.nodes[1].extra_args = ["-onion=xyz:abc"]
-        msg = "Error: Invalid -onion address or hostname: 'xyz:abc'"
+        self.log.info("Test passing invalid -proxy port raises expected init error")
+        self.nodes[1].extra_args = ["-proxy=192.0.0.1:def"]
+        msg = "Error: Invalid port specified in -proxy: '192.0.0.1:def'"
         self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
 
-        self.log.info("Test passing invalid -i2psam raises expected init error")
-        self.nodes[1].extra_args = ["-i2psam=def:xyz"]
-        msg = "Error: Invalid -i2psam address or hostname: 'def:xyz'"
+        self.log.info("Test passing invalid -onion hostname raises expected init error")
+        self.nodes[1].extra_args = ["-onion=xyz..xyz:23456"]
+        msg = "Error: Invalid -onion address or hostname: 'xyz..xyz:23456'"
+        self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
+
+        self.log.info("Test passing invalid -onion port raises expected init error")
+        self.nodes[1].extra_args = ["-onion=192.0.0.1:def"]
+        msg = "Error: Invalid port specified in -onion: '192.0.0.1:def'"
+        self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
+
+        self.log.info("Test passing invalid -i2psam hostname raises expected init error")
+        self.nodes[1].extra_args = ["-i2psam=def..def:23456"]
+        msg = "Error: Invalid -i2psam address or hostname: 'def..def:23456'"
+        self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
+
+        self.log.info("Test passing invalid -i2psam port raises expected init error")
+        self.nodes[1].extra_args = ["-i2psam=192.0.0.1:def"]
+        msg = "Error: Invalid port specified in -i2psam: '192.0.0.1:def'"
         self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
 
         self.log.info("Test passing invalid -onlynet=i2p without -i2psam raises expected init error")