Merge bitcoin/bitcoin#22087: Validate port-options

04526787b5 Validate `port` options (amadeuszpawlik)
f8387c4234 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 04526787b5
  ryanofsky:
    Code review ACK 04526787b5. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding 'GetArgs[0]` problem.

Tree-SHA512: f1ac80bf98520b287a6413ceadb41bc3a93c491955de9b9319ee1298ac0ab982751905762a287e748997ead6198a8bb7a3bc8817ac9e3d2468e11ab4a0f8496d

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)

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")) {

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)

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);

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);

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)

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.
 //

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")