Merge #17754: net: Don't allow resolving of std::string with embedded NUL characters. Add tests.

7a046cdc1423963bdcbcf9bb98560af61fa90b37 tests: Avoid using C-style NUL-terminated strings as arguments (practicalswift)
fefb9165f23fe9d10ad092ec31715f906e0d2ee7 tests: Add tests to make sure lookup methods fail on std::string parameters with embedded NUL characters (practicalswift)
9574de86ad703ad942cdd0eca79f48c0d42b102b net: Avoid using C-style NUL-terminated strings as arguments in the netbase interface (practicalswift)

Pull request description:

  Don't allow resolving of `std::string`:s with embedded `NUL` characters.

  Avoid using C-style `NUL`-terminated strings as arguments in the `netbase` interface

  Add tests.

  The only place in where C-style `NUL`-terminated strings are actually needed is here:

  ```diff
  +    if (!ValidAsCString(name)) {
  +        return false;
  +    }
  ...
  -    int nErr = getaddrinfo(pszName, nullptr, &aiHint, &aiRes);
  +    int nErr = getaddrinfo(name.c_str(), nullptr, &aiHint, &aiRes);
       if (nErr)
           return false;
  ```

  Interface changes:

  ```diff
  -bool LookupHost(const char *pszName, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup);
  +bool LookupHost(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup);

  -bool LookupHost(const char *pszName, CNetAddr& addr, bool fAllowLookup);
  +bool LookupHost(const std::string& name, CNetAddr& addr, bool fAllowLookup);

  -bool Lookup(const char *pszName, CService& addr, int portDefault, bool fAllowLookup);
  +bool Lookup(const std::string& name, CService& addr, int portDefault, bool fAllowLookup);

  -bool Lookup(const char *pszName, std::vector<CService>& vAddr, int portDefault, bool fAllowLookup, unsigned int nMaxSolutions);
  +bool Lookup(const std::string& name, std::vector<CService>& vAddr, int portDefault, bool fAllowLookup, unsigned int nMaxSolutions);

  -bool LookupSubNet(const char *pszName, CSubNet& subnet);
  +bool LookupSubNet(const std::string& strSubnet, CSubNet& subnet);

  -CService LookupNumeric(const char *pszName, int portDefault = 0);
  +CService LookupNumeric(const std::string& name, int portDefault = 0);

  -bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, const SOCKET& hSocketRet, int nTimeout, bool *outProxyConnectionFailed);
  +bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, const SOCKET& hSocketRet, int nTimeout, bool& outProxyConnectionFailed);
  ```

  It should be noted that the `ConnectThroughProxy` change (from `bool *outProxyConnectionFailed` to `bool& outProxyConnectionFailed`) has nothing to do with `NUL` handling but I thought it was worth doing when touching this file :)

ACKs for top commit:
  EthanHeilman:
    ACK 7a046cdc1423963bdcbcf9bb98560af61fa90b37
  laanwj:
    ACK 7a046cdc1423963bdcbcf9bb98560af61fa90b37

Tree-SHA512: 66556e290db996917b54091acd591df221f72230f6b9f6b167b9195ee870ebef6e26f4cda2f6f54d00e1c362e1743bf56785d0de7cae854e6bf7d26f6caccaba

1 files changed, 48 insertions, 29 deletions

diff --git a/src/netbase.cpp b/src/netbase.cpp
index f0c91e0619..a70179cb16 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -7,8 +7,9 @@
 
 #include <sync.h>
 #include <tinyformat.h>
-#include <util/system.h>
 #include <util/strencodings.h>
+#include <util/string.h>
+#include <util/system.h>
 
 #include <atomic>
 
@@ -59,10 +60,14 @@ std::string GetNetworkName(enum Network net) {
     }
 }
 
-bool static LookupIntern(const char *pszName, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup)
+bool static LookupIntern(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup)
 {
     vIP.clear();
 
+    if (!ValidAsCString(name)) {
+        return false;
+    }
+
     {
         CNetAddr addr;
         // From our perspective, onion addresses are not hostnames but rather
@@ -71,7 +76,7 @@ bool static LookupIntern(const char *pszName, std::vector<CNetAddr>& vIP, unsign
         // getaddrinfo to decode them and it wouldn't make sense to resolve
         // them, we return a network address representing it instead. See
         // CNetAddr::SetSpecial(const std::string&) for more details.
-        if (addr.SetSpecial(std::string(pszName))) {
+        if (addr.SetSpecial(name)) {
             vIP.push_back(addr);
             return true;
         }
@@ -93,7 +98,7 @@ bool static LookupIntern(const char *pszName, std::vector<CNetAddr>& vIP, unsign
     // hostname lookups.
     aiHint.ai_flags = fAllowLookup ? AI_ADDRCONFIG : AI_NUMERICHOST;
     struct addrinfo *aiRes = nullptr;
-    int nErr = getaddrinfo(pszName, nullptr, &aiHint, &aiRes);
+    int nErr = getaddrinfo(name.c_str(), nullptr, &aiHint, &aiRes);
     if (nErr)
         return false;
 
@@ -131,7 +136,7 @@ bool static LookupIntern(const char *pszName, std::vector<CNetAddr>& vIP, unsign
 /**
  * Resolve a host string to its corresponding network addresses.
  *
- * @param pszName The string representing a host. Could be a name or a numerical
+ * @param name    The string representing a host. Could be a name or a numerical
  *                IP address (IPv6 addresses in their bracketed form are
  *                allowed).
  * @param[out] vIP The resulting network addresses to which the specified host
@@ -143,28 +148,34 @@ bool static LookupIntern(const char *pszName, std::vector<CNetAddr>& vIP, unsign
  * @see Lookup(const char *, std::vector<CService>&, int, bool, unsigned int)
  *      for additional parameter descriptions.
  */
-bool LookupHost(const char *pszName, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup)
+bool LookupHost(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup)
 {
-    std::string strHost(pszName);
+    if (!ValidAsCString(name)) {
+        return false;
+    }
+    std::string strHost = name;
     if (strHost.empty())
         return false;
     if (strHost.front() == '[' && strHost.back() == ']') {
         strHost = strHost.substr(1, strHost.size() - 2);
     }
 
-    return LookupIntern(strHost.c_str(), vIP, nMaxSolutions, fAllowLookup);
+    return LookupIntern(strHost, vIP, nMaxSolutions, fAllowLookup);
 }
 
  /**
  * Resolve a host string to its first corresponding network address.
  *
- * @see LookupHost(const char *, std::vector<CNetAddr>&, unsigned int, bool) for
+ * @see LookupHost(const std::string&, std::vector<CNetAddr>&, unsigned int, bool) for
  *      additional parameter descriptions.
  */
-bool LookupHost(const char *pszName, CNetAddr& addr, bool fAllowLookup)
+bool LookupHost(const std::string& name, CNetAddr& addr, bool fAllowLookup)
 {
+    if (!ValidAsCString(name)) {
+        return false;
+    }
     std::vector<CNetAddr> vIP;
-    LookupHost(pszName, vIP, 1, fAllowLookup);
+    LookupHost(name, vIP, 1, fAllowLookup);
     if(vIP.empty())
         return false;
     addr = vIP.front();
@@ -174,7 +185,7 @@ bool LookupHost(const char *pszName, CNetAddr& addr, bool fAllowLookup)
 /**
  * Resolve a service string to its corresponding service.
  *
- * @param pszName The string representing a service. Could be a name or a
+ * @param name    The string representing a service. Could be a name or a
  *                numerical IP address (IPv6 addresses should be in their
  *                disambiguated bracketed form), optionally followed by a port
  *                number. (e.g. example.com:8333 or
@@ -191,16 +202,17 @@ bool LookupHost(const char *pszName, CNetAddr& addr, bool fAllowLookup)
  * @returns Whether or not the service string successfully resolved to any
  *          resulting services.
  */
-bool Lookup(const char *pszName, std::vector<CService>& vAddr, int portDefault, bool fAllowLookup, unsigned int nMaxSolutions)
+bool Lookup(const std::string& name, std::vector<CService>& vAddr, int portDefault, bool fAllowLookup, unsigned int nMaxSolutions)
 {
-    if (pszName[0] == 0)
+    if (name.empty() || !ValidAsCString(name)) {
         return false;
+    }
     int port = portDefault;
     std::string hostname;
-    SplitHostPort(std::string(pszName), port, hostname);
+    SplitHostPort(name, port, hostname);
 
     std::vector<CNetAddr> vIP;
-    bool fRet = LookupIntern(hostname.c_str(), vIP, nMaxSolutions, fAllowLookup);
+    bool fRet = LookupIntern(hostname, vIP, nMaxSolutions, fAllowLookup);
     if (!fRet)
         return false;
     vAddr.resize(vIP.size());
@@ -215,10 +227,13 @@ bool Lookup(const char *pszName, std::vector<CService>& vAddr, int portDefault,
  * @see Lookup(const char *, std::vector<CService>&, int, bool, unsigned int)
  *      for additional parameter descriptions.
  */
-bool Lookup(const char *pszName, CService& addr, int portDefault, bool fAllowLookup)
+bool Lookup(const std::string& name, CService& addr, int portDefault, bool fAllowLookup)
 {
+    if (!ValidAsCString(name)) {
+        return false;
+    }
     std::vector<CService> vService;
-    bool fRet = Lookup(pszName, vService, portDefault, fAllowLookup, 1);
+    bool fRet = Lookup(name, vService, portDefault, fAllowLookup, 1);
     if (!fRet)
         return false;
     addr = vService[0];
@@ -235,12 +250,15 @@ bool Lookup(const char *pszName, CService& addr, int portDefault, bool fAllowLoo
  * @see Lookup(const char *, CService&, int, bool) for additional parameter
  *      descriptions.
  */
-CService LookupNumeric(const char *pszName, int portDefault)
+CService LookupNumeric(const std::string& name, int portDefault)
 {
+    if (!ValidAsCString(name)) {
+        return {};
+    }
     CService addr;
     // "1.2:345" will fail to resolve the ip, but will still set the port.
     // If the ip fails to resolve, re-init the result.
-    if(!Lookup(pszName, addr, portDefault, false))
+    if(!Lookup(name, addr, portDefault, false))
         addr = CService();
     return addr;
 }
@@ -768,12 +786,11 @@ bool IsProxy(const CNetAddr &addr) {
  *
  * @returns Whether or not the operation succeeded.
  */
-bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, const SOCKET& hSocket, int nTimeout, bool *outProxyConnectionFailed)
+bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, const SOCKET& hSocket, int nTimeout, bool& outProxyConnectionFailed)
 {
     // first connect to proxy server
     if (!ConnectSocketDirectly(proxy.proxy, hSocket, nTimeout, true)) {
-        if (outProxyConnectionFailed)
-            *outProxyConnectionFailed = true;
+        outProxyConnectionFailed = true;
         return false;
     }
     // do socks negotiation
@@ -796,23 +813,25 @@ bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int
  * Parse and resolve a specified subnet string into the appropriate internal
  * representation.
  *
- * @param pszName A string representation of a subnet of the form `network
+ * @param strSubnet A string representation of a subnet of the form `network
  *                address [ "/", ( CIDR-style suffix | netmask ) ]`(e.g.
  *                `2001:db8::/32`, `192.0.2.0/255.255.255.0`, or `8.8.8.8`).
  * @param ret The resulting internal representation of a subnet.
  *
  * @returns Whether the operation succeeded or not.
  */
-bool LookupSubNet(const char* pszName, CSubNet& ret)
+bool LookupSubNet(const std::string& strSubnet, CSubNet& ret)
 {
-    std::string strSubnet(pszName);
+    if (!ValidAsCString(strSubnet)) {
+        return false;
+    }
     size_t slash = strSubnet.find_last_of('/');
     std::vector<CNetAddr> vIP;
 
     std::string strAddress = strSubnet.substr(0, slash);
-    // TODO: Use LookupHost(const char *, CNetAddr&, bool) instead to just get
+    // TODO: Use LookupHost(const std::string&, CNetAddr&, bool) instead to just get
     //       one CNetAddr.
-    if (LookupHost(strAddress.c_str(), vIP, 1, false))
+    if (LookupHost(strAddress, vIP, 1, false))
     {
         CNetAddr network = vIP[0];
         if (slash != strSubnet.npos)
@@ -827,7 +846,7 @@ bool LookupSubNet(const char* pszName, CSubNet& ret)
             else // If not a valid number, try full netmask syntax
             {
                 // Never allow lookup for netmask
-                if (LookupHost(strNetmask.c_str(), vIP, 1, false)) {
+                if (LookupHost(strNetmask, vIP, 1, false)) {
                     ret = CSubNet(network, vIP[0]);
                     return ret.IsValid();
                 }